test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option #19014

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2005-testPrevReleases changing 4 files +12 −17

MarcoFalke commented at 11:09 AM on May 19, 2020: member
The "auto-detection" feature is kept in place, but making it an option allows to properly document it. For example, on my machine I get:
```
$ ./test/functional/wallet_disable.py --help | grep previous-releases
  --previous-releases   Force test of previous releases (default: False)
```
fanquake added the label Tests on May 19, 2020
fanquake cross-referenced this on May 19, 2020 from issue test: fix TEST_PREVIOUS_RELEASES check by fanquake
fanquake commented at 11:11 AM on May 19, 2020: member

Concept ACK
Sjors commented at 1:15 PM on May 19, 2020: member

Concept ACK. You have to explicitly opt out of --previous-releases on Travis, because PREVIOUS_RELEASES_DIR is created and cached for all machines.

in test/functional/test_framework/test_framework.py:159 in fa55deb2ab outdated

 154 | @@ -154,6 +155,9 @@ def parse_args(self):
 155 |                              help="Print out all RPC calls as they are made")
 156 |          parser.add_argument("--portseed", dest="port_seed", default=os.getpid(), type=int,
 157 |                              help="The seed to use for assigning port numbers (default: current process id)")
 158 | +        parser.add_argument("--previous-releases", dest="prev_releases", action="store_true",
 159 | +                            default=os.path.isdir(previous_releases_path) and bool(os.path.listdir(previous_releases_path)),

Sjors commented at 3:21 PM on May 19, 2020:

That's a Python 3.6 syntax I think https://docs.python.org/3/library/os.html#os.listdir

MarcoFalke commented at 3:36 PM on May 19, 2020:

Nah, just me being stupid. I forgot to test this locally before pushing

MarcoFalke force-pushed on May 19, 2020
MarcoFalke force-pushed on May 19, 2020
Sjors commented at 3:56 PM on May 19, 2020: member

tACK fab56c1a6e95b75f0dacb2d7301ac0fec9a310d5

It would be nice if you could still opt-out without having to move the releases folder. This can be useful when a new version is added and I can't be bothered to download it.
MarcoFalke commented at 5:10 PM on May 19, 2020: member

It would be nice if you could still opt-out without having to move the releases folder. This can be useful when a new version is added and I can't be bothered to download it.

If this auto-detection behavior is too brittle, we should simply remove it. It seems odd to have default values depend on the presence of directories or files on the hard disk.
MarcoFalke force-pushed on May 19, 2020
Sjors commented at 5:34 PM on May 19, 2020: member

If this auto-detection behavior is too brittle, we should simply remove it. It seems odd to have default values depend on the presence of directories or files on the hard disk.

That's annoying. Now you have to add --previous-releases every time to run those tests. Checking the presence of /releases for the default was better. We can always improve the error messages a bit, e.g. show which specific version is missing (you can already tell from the exceptions).
MarcoFalke force-pushed on May 19, 2020
Sjors commented at 5:39 PM on May 19, 2020: member

re-tACK fab56c1 thanks for restoring
DrahtBot commented at 8:18 PM on May 19, 2020: 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:
- #19013 (test: add v0.20.0 to backwards compatibility test by Sjors)
- #18434 (tests: add a test-security target and run it in CI by fanquake)
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.
DrahtBot cross-referenced this on May 19, 2020 from issue test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test by Sjors
DrahtBot cross-referenced this on May 19, 2020 from issue Multiprocess build support by ryanofsky
DrahtBot cross-referenced this on May 20, 2020 from issue tests: add a test-security target and run it in CI by fanquake
DrahtBot cross-referenced this on May 20, 2020 from issue net: Add NAT-PMP port forwarding support by hebasto
DrahtBot added the label Needs rebase on May 21, 2020
test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option faf1c3cc58
test: Default --previous-releases to false if dir is empty fad798be76
MarcoFalke force-pushed on May 21, 2020
DrahtBot removed the label Needs rebase on May 21, 2020
Sjors commented at 8:58 AM on May 22, 2020: member

re-tACK fad798b
MarcoFalke merged this on May 22, 2020
MarcoFalke closed this on May 22, 2020
MarcoFalke deleted the branch on May 22, 2020
sidhujag referenced this in commit 49d9fc4fe6 on May 22, 2020
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18903 test: fix TEST_PREVIOUS_RELEASES check

test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option #19014

Conflicts