-
Notifications
You must be signed in to change notification settings - Fork 38.7k
tests: Skip --descriptor tests if sqlite is not compiled #20262
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
Conversation
|
Concept ACK Getting test failures on another PR because of this (I think) |
|
This PR successfully skips the I'm still getting failures on feature_pruning.py, interface_rpc.py, p2p_addrv2_relay.py but not related to this PR I don't think. |
|
ACK 7411876: patch looks correct |
ryanofsky
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.
Code review ACK 7411876
|
|
||
| def skip_test_if_missing_module(self): | ||
| self.skip_if_no_wallet() | ||
| self.skip_if_no_sqlite() |
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.
Is it necessary to call self.skip_if_no_sqlite() explicitly here, since it is already called in skip_if_no_wallet(self) if --descriptors is set (and looking at the additions to test_runner.py this seems to now be the case)?
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.
It's being done for people who just run it on the command line without --descriptor. This test always needs this check.
|
|
||
| def skip_test_if_missing_module(self): | ||
| self.skip_if_no_wallet() | ||
| self.skip_if_no_sqlite() |
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.
same as my comment in wallet_descriptor.py
| self.setup_clean_chain = True | ||
| self.num_nodes = 1 | ||
| self.extra_args = [['-keypool=100']] | ||
| self.wallet_names = [] |
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.
Why did this need to be added? I'm not seeing it used anywhere
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.
To ensure that no default wallets are created.
hebasto
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.
ACK 7411876, tested on Linux Mint 20 (x86_64), tests pass for binaries compiled with:
--without-sqlite--disable-wallet
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
tACK 7411876 You may want to swap the commits, so none of them break the tests. |
#20156 allows sqlite to not be compiled by configuring
--without-sqlite. However doing so and then running the test runner will result in all of the--descriptortests to fail. We should be skipping those tests if sqlite was not compiled.