Bump test framework to v30.0#758
Conversation
pinheadmz
left a comment
There was a problem hiding this comment.
concept ACK, did quick review in the browser. I'll dig in to the CI failures next.
THANKS!
resources/scenarios/ln_init.py
Outdated
|
|
||
| def main(): | ||
| LNInit().main() | ||
| LNInit(__file__).main() |
There was a problem hiding this comment.
Instead of adding this to every scenario, and since we don't use the value anyway, why not just add a dummy to Commander constructor:
class Commander(BitcoinTestFramework):
def __init__(self):
super().__init__(__file__) # or NoneThere was a problem hiding this comment.
Right, that's much better, I've taken this.
There was a problem hiding this comment.
Update: Ah, I've just remembered why I did it this way: https://github.com/bitcoin/bitcoin/blob/v30.0/test/functional/test_framework/test_framework.py#L134-L136:
class BitcoinTestMetaClass(type):
"""Metaclass for BitcoinTestFramework.
Ensures that any attempt to register a subclass of `BitcoinTestFramework`
adheres to a standard whereby the subclass overrides `set_test_params` and
`run_test` but DOES NOT override either `__init__` or `main`. If any of
those standards are violated, a ``TypeError`` is raised."""
def __new__(cls, clsname, bases, dct):
if not clsname == 'BitcoinTestFramework':
if '__init__' in dct or 'main' in dct:
raise TypeError("BitcoinTestFramework subclasses may not override "
"'__init__' or 'main'")b2127dc to
6fde7ae
Compare
6fde7ae to
be0d127
Compare
8d76317 to
2b062f2
Compare
Can be reproduced or verified as follows: ```bash git clone git@github.com:bitcoin/bitcoin.git --depth 1 --branch 30.x tmp rm -rf resources/scenarios/test_framework/ mv tmp/test/functional/test_framework/ resources/scenarios/ rm -rf tmp ```
The repetitive part of this diff can reproduced with:
```bash
sed -i -e 's/\s*().main\s*()/("").main()/' $(git ls-files 'resources/*.py')
```
Which mostly comes from this scripted diff from bitcoin/bitcoin:
bitcoin/bitcoin@a047344
send_version, added in bitcoin/bitcoin#28782 supports_v2_p2p, added in bitcoin/bitcoin#24748 * Note that in the reconnaissance scenario v2 is false because we are connecting to older versions of bitcoind
No reason to maintain this code in two places after bitcoin-dev-project#739
2b062f2 to
34e9c18
Compare
|
I fixed the CI failures on main and rebased. Then added a few more commits to accomdate upstream changes to the test framework. This is a breaking change and Galen Erso and Wrath of Nalo will need to either be rebased or pin specific older versions of warnet (see #763) |
pinheadmz
left a comment
There was a problem hiding this comment.
--> diff ~/Desktop/work/bitcoin/test/functional/test_framework/ ~/Desktop/work/warnet/resources/scenarios/test_framework/
Diff is a match, CI passes. LFG, worry about breaking changes later. Main should be considered unstable now
Bumps the included test framework to Bitcoin Core 30.0, and update example scenarios. Presently, bumping and fixing the tests happen in separate commits for ease of review, but I'm happy to squash if commits being atomic is preferred.
This PR also includes an unrelated change I found helpful: Enabling CI tests on all branches so contributors to warnet will have CI run on their branches without having to open a PR against
main.Opening as a draft as this is still failing CI at the moment.