[tests] Change feature_csv_activation.py to use BitcoinTestFramework #11817
Conversation
jnewbery
force-pushed
the
refactor_bip68-112-113-p2p
branch
from
c036491 to
117f261
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Reviewed following non-#11771 commits:
- utACK 117f261bfcff1249be96d66f48350338e16580e3 [tests] Change bip68-112-113-p2p.py to use BitcoinTestFramework
- utACK 7424bb122997da17276526d074e6380f2e112e3f [tests] Move utility functions in bip68-112-113-p2p.py out of class.
- utACK e78b600be47bf34c4271b217b616a77f8957e531 [tests] Remove nested loops from bip68-112-113-p2p.py
- utACK 44db6e78cf6acefe07ff9f5edb5f9995c7b45d7a [tests] improve logging in bip68-112-113-p2p.py
- utACK 9c513676921f93ff3f3886e8c6072fd4c3314249 [tests] fix flake8 nits in bip68-112-113-p2p.py
test/functional/bip68-112-113-p2p.py
Outdated
There was a problem hiding this comment.
In commit "Remove nested loops from bip68-112-113-p2p.py"
Should be able to avoid a lookup with [tx['tx'] for tx in txs.values()]
There was a problem hiding this comment.
Yes, that's better. Fixed
test/functional/bip68-112-113-p2p.py
Outdated
There was a problem hiding this comment.
In commit "Remove nested loops from bip68-112-113-p2p.py"
It seems better to continue returning a list than a dictionary. A dictionary is just slower and introduces nondeterminism in the test for no reason.
There was a problem hiding this comment.
yes - returning a list is much better. Changed. Thanks
jnewbery
force-pushed
the
refactor_bip68-112-113-p2p
branch
from
117f261 to
6bb4850
Compare
|
Rebased now that #11771 has been merged. edit: Just realised that I have't addressed Russ's comment here: #11817 (comment). Will do that later |
jnewbery
force-pushed
the
refactor_bip68-112-113-p2p
branch
from
6bb4850 to
96b02a0
Compare
|
ok, properly rebased and Russ's comment addressed. |
There was a problem hiding this comment.
To go along with @ryanofsky earlier comments in related PRs. This can be simplified to int(hash,16)
There was a problem hiding this comment.
Same here. Can be int(hash,16)
|
Added small nits, but ACK. |
|
utACK 96b02a0f62a64819c0048357b9e8b9da91af195f |
jnewbery
changed the title
|
@conscott thanks for the review. I agree that |
|
Needs rebase in the commit that moves the methods. Sorry for letting this sit for so long. |
Not a problem. This is low priority, will rebase next week. |
|
Rebased. Also addressed @conscott's comments: #11817 (comment) in the first commit. |
jnewbery
force-pushed
the
refactor_bip68-112-113-p2p
branch
from
96b02a0 to
cb5b047
Compare
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
utACK cb5b047a6f6fb58afee, just some bike-shedding about formatting and comments. Feel free to ignore.
There was a problem hiding this comment.
In commit c5427d1f4e3652a1b55968c752fb362b31996186
The "flags" to indicate if the random high- or low-bit was set are never read. Might as well not include it in the object.
There was a problem hiding this comment.
Yep, good spot. I've removed the 'srhb' and 'srlb' keys.
There was a problem hiding this comment.
In commit c5427d1f4e3652a1b55968c752fb362b31996186:
Any reason you remove the trailing comments in all lines but not in this one?
There was a problem hiding this comment.
I missed this one. Now removed.
There was a problem hiding this comment.
In commit cb5b047a6f6fb58afee:
Nit: Sometimes you use named args for the bool, and sometimes you don't. Might as well use the success=False everywhere.
There was a problem hiding this comment.
sounds good. Updated.
|
Concept ACK, and very weak utACK (read through the changes, not a Python expert). |
jnewbery
force-pushed
the
refactor_bip68-112-113-p2p
branch
from
cb5b047 to
1298268
Compare
|
re-utACK 1298268 only changes were style changes that shouldn't change behaviour. |
…TestFramework 1298268 [tests] Change feature_csv_activation.py to use BitcoinTestFramework (John Newbery) db7ffb9 [tests] Move utility functions in feature_csv_activation.py out of class. (John Newbery) 0842edf [tests] Remove nested loops from feature_csv_activation.py (John Newbery) 2e511d5 [tests] improve logging in feature_csv_activation.py (John Newbery) 6f7f5bc [tests] fix flake8 nits in feature_csv_activation.py (John Newbery) Pull request description: Next step in #10603. - first four commits tidy up bip68-112-113-p2p.py - fifth commit removes usage of ComparisonTestFramework Tree-SHA512: 34466be12280096ad92ac842f9c594ae40c19be9b4edc73c1e37964d03d55f4e75b80cea50c9940404096effc23705671503a883a7b7773b5866a29f653ba710
…TestFramework] 9c92c8c [tests] Remove Comparison Test Framework (John Newbery) e80c640 [tests] Remove bip9-softforks.py (John Newbery) Pull request description: Builds on #11772, #11773 and #11817. Please review those PRs first. Final step in #10603. - First commit removes bip9-softforks.py. bip9-sofforks.py was intended to be a generic test for versionbits deployments. However, it only tests CSV activation and was not updated to test segwit activation. CSV activation is tested by bip68-112-113-p2p.py, so this test is duplicated effort. Rather than try to update it to use the BitcoinTestFramework, just remove it. (see btcdrak#8 for previous discussion around the redundancy of bip9-softforks.py) - Second commit removes the now unused BitcoinComparisonFramework class and the comptool and blockstore modules. Tree-SHA512: 4bb7196d521048b3b8ba95c87dde73005a1ac73d29ccbb869f11ce9a71089686e7eacd7335337853041dfbd3a5b110172b105adbada58779814d4db22b1376f5
…se BitcoinTestFramework (#3278) * [tests] fix flake8 nits in feature_csv_activation.py * [tests] improve logging in feature_csv_activation.py * [tests] Remove nested loops from feature_csv_activation.py Makes the test a lot clearer. * [tests] Move utility functions in feature_csv_activation.py out of class. * [tests] Change feature_csv_activation.py to use BitcoinTestFramework * Few import fixes Co-authored-by: John Newbery <[email protected]>
7 participants
Next step in #10603.