Skip to content

Conversation

@decryp2kanon
Copy link
Contributor

@decryp2kanon decryp2kanon commented Sep 30, 2020

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

before: 😕

bTc/kB
btc/kb
BTc/Kb

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

after: 😄

BTC/kB
sat/B

reference:

BTC_KB, //!< Use explicit BTC/kB fee given in coin control
SAT_B, //!< Use explicit sat/B fee given in coin control

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

@DrahtBot DrahtBot added the Tests label Sep 30, 2020
@fanquake fanquake changed the title test: unit case sensitive in a functional test test: consistently use BTC/kB in wallet_basic test Sep 30, 2020
@decryp2kanon
Copy link
Contributor Author

decryp2kanon commented Sep 30, 2020

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

@sipa
Copy link
Member

sipa commented Sep 30, 2020

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

@decryp2kanon
Copy link
Contributor Author

decryp2kanon commented Sep 30, 2020

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. 05227a3

BTC_KB, //!< Use explicit BTC/kB fee given in coin control
SAT_B, //!< Use explicit sat/B fee given in coin control

what do you think? @kallewoof

@decryp2kanon decryp2kanon changed the title test: consistently use BTC/kB in wallet_basic test test: consistently use BTC/kB and sat/B in wallet_basic test Sep 30, 2020
@kallewoof
Copy link
Contributor

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

@decryp2kanon
Copy link
Contributor Author

decryp2kanon commented Sep 30, 2020

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?

@kallewoof
Copy link
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.

@jonatack
Copy link
Member

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

@decryp2kanon
Copy link
Contributor Author

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

@murchandamus
Copy link
Contributor

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.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants