test: consistently use BTC/kB and sat/B in wallet_basic test #20041

I found some confused case sensitive in BTC/kB and sat/B. It doesn't change the test result.

before: :confused:

bTc/kB
btc/kb
BTc/Kb

sat/B
sAT/b
SAT/b
SAT/B

after: :smile:

BTC/kB
sat/B

reference: https://github.com/bitcoin/bitcoin/blob/1769828684f16b53e5fbf65173f508b9ea1b4b9c/src/policy/feerate.h#L22-L23

test result:

$ ./test/functional/test_runner.py --jobs=4 --loglevel=DEBUG --previous-releases --coverage --extended wallet_basic.py

Temporary test directory at /tmp/test_runner_₿_🏃_20200929_224115
Running Unit Tests for Test Framework Modules
.......
----------------------------------------------------------------------
Ran 7 tests in 0.008s

OK
Initializing coverage directory at /tmp/coveragevbx7unjc
Remaining jobs: [wallet_basic.py]
1/1 - wallet_basic.py passed, Duration: 32 s                   

TEST            | STATUS    | DURATION

wallet_basic.py | ✓ Passed  | 32 s

ALL             | ✓ Passed  | 32 s (accumulated) 
Runtime: 32 s

i just changed the commit title as suggested by fanquake. thanks!

I assume this is intentional? To test that units are parsed in a case-insenstive manner?

I assume this is intentional? To test that units are parsed in a case-insenstive manner?

you are right. it was intentional by the author, but it doesn't work correctly IMHO. so i suggest to use consistently. https://github.com/bitcoin/bitcoin/commit/05227a35545d7656450874b3668bf418c73813fb

https://github.com/bitcoin/bitcoin/blob/1769828684f16b53e5fbf65173f508b9ea1b4b9c/src/policy/feerate.h#L22-L23

what do you think? @kallewoof

It's intentional, and it should work, so if it doesn't work, then I would prefer fixing it over changing the values.

It's intentional, and it should work, so if it doesn't work, then I would prefer fixing it over changing the values.

thanks to confirm! however in my case, after changed its still passing. it should be failure?

Why should it be a failure? Any variant is allowed, including the ones you are using. You're just removing variants in the test cases.

Agree with the other reviewers, these tests are intentional and the proposed change would reduce the coverage.

Why should it be a failure? Any variant is allowed, including the ones you are using. You're just removing variants in the test cases.

i understood. thanks to make it clear! PR closing

I assume this is intentional? To test that units are parsed in a case-insenstive manner?

I was just remarking on the variety in capitalization in this test for the review of #20220. I think the change proposed by @decryp2kanon here would be great, if supplemented by an explicit test for the case insensitivity of the parameter value.