-
Notifications
You must be signed in to change notification settings - Fork 54
Bump test framework to v30.0 #758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Bump test framework to v30.0 #758
Conversation
pinheadmz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's much better, I've taken this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'")Can be reproduced or verified as follows: ```bash git clone [email protected]: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 ```
88acd58 to
b2127dc
Compare
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
b2127dc to
6fde7ae
Compare
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.