Skip to content

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Oct 29, 2020

#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 --descriptor tests to fail. We should be skipping those tests if sqlite was not compiled.

@michaelfolkson
Copy link

michaelfolkson commented Oct 29, 2020

Concept ACK

Getting test failures on another PR because of this (I think)

@maflcko maflcko added this to the 0.21.0 milestone Oct 29, 2020
@michaelfolkson
Copy link

This PR successfully skips the --descriptor tests on MacOS Catalina.

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.

@practicalswift
Copy link
Contributor

ACK 7411876: patch looks correct

Copy link
Contributor

@ryanofsky ryanofsky left a 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()
Copy link
Contributor

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)?

Copy link
Member Author

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()
Copy link
Contributor

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 = []
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member

@hebasto hebasto left a 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

@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

@Sjors
Copy link
Member

Sjors commented Oct 30, 2020

tACK 7411876

You may want to swap the commits, so none of them break the tests.

@meshcollider meshcollider merged commit 0c2eb7f into bitcoin:master Nov 1, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 2, 2020
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants