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

1. 
   decryp2kanon commented at 2:45 am on September 30, 2020: contributor

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

   before: :confused:

   0bTc/kB
   1btc/kb
   2BTc/Kb
   3
   4sat/B
   5sAT/b
   6SAT/b
   7SAT/B
   

   after: :smile:

   0BTC/kB
   1sat/B
   

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

   test result:

    0$ ./test/functional/test_runner.py --jobs=4 --loglevel=DEBUG --previous-releases --coverage --extended wallet_basic.py
    1
    2Temporary test directory at /tmp/test_runner_₿_🏃_20200929_224115
    3Running Unit Tests for Test Framework Modules
    4.......
    5----------------------------------------------------------------------
    6Ran 7 tests in 0.008s
    7
    8OK
    9Initializing coverage directory at /tmp/coveragevbx7unjc
   10Remaining jobs: [wallet_basic.py]
   111/1 - wallet_basic.py passed, Duration: 32 s                   
   12
   13TEST            | STATUS    | DURATION
   14
   15wallet_basic.py | ✓ Passed  | 32 s
   16
   17ALL             | ✓ Passed  | 32 s (accumulated) 
   18Runtime: 32 s
   

2. DrahtBot added the label Tests on Sep 30, 2020
3. fanquake renamed this:
   test: unit case sensitive in a functional test
   test: consistently use BTC/kB in wallet_basic test
   on Sep 30, 2020
4. test: consistently use BTC/kB in wallet_basic test 476675d279
5. decryp2kanon force-pushed on Sep 30, 2020
6. 
   decryp2kanon commented at 7:07 am on September 30, 2020: contributor

   i just changed the commit title as suggested by fanquake. thanks!
7. 
   sipa commented at 7:09 am on September 30, 2020: member

   I assume this is intentional? To test that units are parsed in a case-insenstive manner?
8. 
   decryp2kanon commented at 7:17 am on September 30, 2020: contributor

   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

9. test: consistently use sat/B in wallet_basic test 18934af65e
10. decryp2kanon renamed this:
    test: consistently use BTC/kB in wallet_basic test
    test: consistently use BTC/kB and sat/B in wallet_basic test
    on Sep 30, 2020
11. 
    kallewoof commented at 7:59 am on September 30, 2020: member

    It’s intentional, and it should work, so if it doesn’t work, then I would prefer fixing it over changing the values.
12. 
    decryp2kanon commented at 8:03 am on September 30, 2020: contributor

    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?

13. 
    kallewoof commented at 8:12 am on September 30, 2020: member

    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.
14. 
    jonatack commented at 8:20 am on September 30, 2020: member

    Agree with the other reviewers, these tests are intentional and the proposed change would reduce the coverage.
15. 
    decryp2kanon commented at 8:25 am on September 30, 2020: contributor

    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

16. decryp2kanon closed this on Sep 30, 2020

    ---

17. decryp2kanon deleted the branch on Sep 30, 2020
18. 
    Xekyo commented at 1:51 am on October 29, 2020: member

    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.

19. DrahtBot locked this on Feb 15, 2022

Contributors
decryp2kanon sipa kallewoof jonatack Xekyo

Labels
Tests