tests: Run both descriptor and legacy tests within a single test invocation #20892
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
This was referenced Jan 9, 2021
857c61e to
b827c13
Compare
This was referenced Jan 13, 2021
b827c13 to
76fd40d
Compare
This was referenced Jan 26, 2021
c54bcd0 to
7dbd3fd
Compare
7dbd3fd to
bed643b
Compare
This was referenced Feb 9, 2021
62b7f30 to
f19e4e6
Compare
f19e4e6 to
ce7fa01
Compare
48 tasks
|
I have the following error when running the test_runner with BDB compiled using depends: Details1695642351381,"4/288 - wallet_backup.py --legacy-wallet failed, Duration: 43 s"
1695642351381,stdout:
1695642351381,2023-09-25T11:39:01.368000Z TestFramework (INFO): PRNG seed is: 8911868130033750645
1695642351381,2023-09-25T11:39:01.381000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb
1695642351381,2023-09-25T11:39:05.172000Z TestFramework (INFO): Generating initial blockchain
1695642351381,2023-09-25T11:39:09.767000Z TestFramework (INFO): Creating transactions
1695642351381,2023-09-25T11:39:25.216000Z TestFramework (INFO): Backing up
1695642351381,2023-09-25T11:39:25.622000Z TestFramework (INFO): More transactions
1695642351381,2023-09-25T11:39:39.536000Z TestFramework (INFO): Restoring wallets on node 3 using backup files
1695642351381,2023-09-25T11:39:40.434000Z TestFramework (INFO): Restoring using dumped wallet
1695642351381,2023-09-25T11:39:43.078000Z TestFramework (ERROR): Unexpected exception caught during testing
1695642351381,Traceback (most recent call last):
1695642351381," File ""/tmp/bitcoin/test/functional/test_framework/test_framework.py"", line 148, in main"
1695642351381, self.run_test()
1695642351381," File ""/tmp/bitcoin/test/functional/wallet_backup.py"", line 231, in run_test"
1695642351381," self.nodes[node_num].createwallet(wallet_name=self.default_wallet_name, descriptors=self.use_descriptors, load_on_startup=True)"
1695642351381," File ""/tmp/bitcoin/test/functional/test_framework/test_node.py"", line 818, in createwallet"
1695642351381," return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)"
1695642351381," File ""/tmp/bitcoin/test/functional/test_framework/coverage.py"", line 50, in __call__"
1695642351381," return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)"
1695642351381," File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 126, in __call__"
1695642351381," postdata = json.dumps(self.get_request(*args, **argsn), default=serialization_fallback, ensure_ascii=self.ensure_ascii)"
1695642351381," File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 114, in get_request"
1695642351381," json.dumps(args or argsn, default=serialization_fallback, ensure_ascii=self.ensure_ascii),"
1695642351381," File ""/usr/lib/python3.10/json/__init__.py"", line 238, in dumps"
1695642351381, **kw).encode(obj)
1695642351381," File ""/usr/lib/python3.10/json/encoder.py"", line 199, in encode"
1695642351381," chunks = self.iterencode(o, _one_shot=True)"
1695642351381," File ""/usr/lib/python3.10/json/encoder.py"", line 257, in iterencode"
1695642351381," return _iterencode(o, 0)"
1695642351381," File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 68, in serialization_fallback"
1695642351381," raise TypeError(repr(o) + "" is not JSON serializable"")"
1695642351381,TypeError: <test_framework.test_framework.OptionalBool object at 0x7fbcf6e055a0> is not JSON serializable
1695642351381,2023-09-25T11:39:43.130000Z TestFramework (INFO): Stopping nodes
1695642351381,2023-09-25T11:39:43.507000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb
1695642351381,2023-09-25T11:39:43.507000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb/test_framework.log
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR):
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): Hint: Call /tmp/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb' to consolidate all logs
1695642351381,2023-09-25T11:39:4
8000
3.508000Z TestFramework (ERROR):
1695642351381,"2023-09-25T11:39:43.508000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log."
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): |
|
Not sure if this is worth it at this point. If there are independent test framework improvements, they can be submitted and reviewed and tested independently. However, I don't think the whole test framework should be rewritten for a temporary need. IIRC it should be possible to just remove the feature to create new bdb wallets after the branch-off. It bdb wallet support is going to be removed in the future, there shouldn't be any need for new wallets to be created in this format, especially with the 27.0 release next year. If someone (or the tests) really need to create a bdb wallet, they can just use the previous release (for example 25.0). Happy to review a pull request that removes the feature to create bdb wallets. |
Instead of manually making the tmpdir depending on whether --tmpdir is provided, we can always us tempfile.mkdtemp and provide the dir option with self.options.tmpdir. This behaves the same as previously except that the tmpdirs created when --tmpdir is passed will have additional randomness on the directory names. In order to get the correct tmpdir prefixes from the test runner, a --tmpdir-prefix option is added. Lastly, the tmpdir location is stored in a dedicated self.tmpdir variable. We wll remove usage of self.options.tmpdir for that path in later commits.
--tmpdirprefix is now unnecessary and potentially confusing.
Instead of retrieving the actual tmpdir path from self.options.tmpdir, retrieve it from self.tmpdir. -BEGIN VERIFY SCRIPT- sed -i "s/self.options.tmpdir/self.tmpdir/" $(git grep -l "self.options.tmpdir" -- ":!test/functional/test_framework/test_framework.py") sed -i "264,271b;s/self.options.tmpdir/self.tmpdir/" test/functional/test_framework/test_framework.py -END VERIFY SCRIPT-
Some new wallet functional tests were added, these need to --legacy-wallet and --descriptors variants in test_runner.py
self.use_descriptors tracks whether the test should be using a descriptor. However, to avoid the fact that None evaluates to False, it is a newly introduced OptionalBool class that will assert if it is None when used as a bool rather than returning False. test_framework.py is updated to use self.use_descriptors throughout rather than self.options.descriptors. The next commit will update the rest of the tests.
Instead of using the options parser to hold whether we are doing descriptor wallets, have a member in the test framework class to handle it. -BEGIN VERIFY SCRIPT- sed -i -E "s/self.options.descriptors/self.use_descriptors/" $(git grep -l "self.options.descriptors" -- ":(exclude)*/test_framework.py") -END VERIFY SCRIPT-
self.options.descriptors is no longer used by the tests directly, so we don't need to be assigning it.
Determining the default wallet name based on the wallet type the test is running with requires being in `setup()`, so move the setting of `self.default_wallet_name` there. This requires changing a couple of tests in the way that they can specify that the default wallet name should be used in `self.wallet_names`.
Be able to run tests that can run in both descriptor and legacy wallet modes with a single invocation of the test script. In order to facilitate this, self.options.descriptors is changed to be a list. When neither option is provided, but both are available, it is set to [False, True] which will run the test twice, the first in legacy wallet mode, the second in descriptor wallet mode. For the tests that can only have one mode, add_wallet_options will set self.options.descriptors to either [False] or [True] which will make the correct type run.
Most wallet tests need to be run with both wallet types. Instead of requiring developers to manually specify this, the test_runner can find the wallet tests that do not have a wallet type specified and automatically run the test with both types.
ce7fa01 to
99d7e2c
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
The intention is to remove the legacy wallet soon(tm), so I guess this isn't worth merging and then reverting in a few months. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of having the tests only do one type or the other, depending on command line arguments, these tests should be running with both types of wallets. Any test that calls
add_wallet_optionswill be run for each wallet type that it specifies a wallet option for if none are give in the command line. If a particular wallet is specified with--legacy-walletor--descriptors, the options are still respected.