Skip to content

test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option #19014

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 2 commits into from
May 22, 2020
Merged

test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option #19014

maflcko merged 2 commits into from
May 22, 2020

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 19, 2020 •
edited
Loading

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 fanquake added the Tests label May 19, 2020
@fanquake fanquake mentioned this pull request May 19, 2020
Closed
@fanquake
Copy link
Member

fanquake commented May 19, 2020

Concept ACK

@Sjors
Copy link
Member

Sjors commented May 19, 2020

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

Sjors
Sjors reviewed May 19, 2020
View reviewed changes
test/functional/test_framework/test_framework.py Outdated
Copy link
Member

@Sjors Sjors May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@maflcko maflcko May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko maflcko force-pushed the 2005-testPrevReleases branch 2 times, most recently from fa1daae to fab56c1 Compare May 19, 2020 15:49
@Sjors
Copy link
Member

Sjors commented May 19, 2020

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.

@maflcko
Copy link
Member Author

maflcko commented May 19, 2020

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.

@maflcko maflcko force-pushed the 2005-testPrevReleases branch from fab56c1 to faecf28 Compare May 19, 2020 17:17
@Sjors
Copy link
Member

Sjors commented May 19, 2020

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

@maflcko maflcko force-pushed the 2005-testPrevReleases branch from faecf28 to fab56c1 Compare May 19, 2020 17:35
@Sjors
Copy link
Member

Sjors commented May 19, 2020

re-tACK fab56c1 thanks for restoring

@DrahtBot
Copy link
Contributor

DrahtBot commented May 19, 2020 •
edited
Loading

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.

This was referenced May 19, 2020
Merged
Merged
Merged
Merged
@maflcko maflcko force-pushed the 2005-testPrevReleases branch from fab56c1 to fad798b Compare May 21, 2020 11:50
@Sjors
Copy link
Member

Sjors commented May 22, 2020

re-tACK fad798b

@maflcko maflcko merged commit b5c423c into bitcoin:master May 22, 2020
@maflcko maflcko deleted the 2005-testPrevReleases branch May 22, 2020 10:44
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 22, 2020
Merge bitcoin#19014: test: Replace TEST_PREVIOUS_RELEASES env var wit…
49d9fc4 
…h test_framework option

fad798b test: Default --previous-releases to false if dir is empty (MarcoFalke)
faf1c3c test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option (MarcoFalke)

Pull request description:

  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)

ACKs for top commit:
  Sjors:
    re-tACK fad798b

Tree-SHA512: a7377d0d5378be0a50be278d76396cc403583617b5fc43467773eee706df698acf3f4e67651491183b9b43a8e1816b052e4c17b90272b7ec4b6ac134ad811400
@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.

Reviewers

1 more reviewer

@Sjors Sjors Sjors left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maflcko @fanquake @Sjors @DrahtBot