Skip to content

[qa] Refactor test_framework and pull tester #7971

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 4 commits into from
May 9, 2016
Merged

[qa] Refactor test_framework and pull tester #7971

maflcko merged 4 commits into from
May 9, 2016

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 29, 2016

  • log to stdout
  • increase range for p2p and rpc ports
  • UPPERCASE_CONSTANTS
  • Stop nodes on CTRL+C

@maflcko maflcko force-pushed the Mf1604-qaRefactor branch 3 times, most recently from 5f95266 to fa056ba Compare April 30, 2016 13:14
@maflcko
Copy link
Member Author

maflcko commented Apr 30, 2016

  • Added a commit to properly stop other nodes, even when one fails to stop
  • Added a commit to update the Readme

@maflcko maflcko force-pushed the Mf1604-qaRefactor branch from fa056ba to fa6cd63 Compare May 1, 2016 07:52
@maflcko maflcko mentioned this pull request May 1, 2016
Merged
@maflcko
Copy link
Member Author

maflcko commented May 6, 2016

Anyone interested in looking at the diff here? (It is mostly move-only and cleaning up the logic/ifelses)

It is already tested as part of #7972 but I'd prefer to have at least one read the patch.

@maflcko maflcko mentioned this pull request May 6, 2016
Closed
maflcko
maflcko reviewed May 6, 2016
View reviewed changes
qa/rpc-tests/test_framework/util.py Outdated
Copy link
Member Author

@maflcko maflcko May 6, 2016

Choose a reason for hiding this comment

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

Needs bump: #7972 (comment)

Copy link
Member Author

@maflcko maflcko May 6, 2016

Choose a reason for hiding this comment

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

@fanquake @paveljanik done in another commit.

Copy link
Member Author

@maflcko maflcko May 6, 2016

Choose a reason for hiding this comment

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

Rebased instead 😜

@paveljanik
Copy link
Contributor

paveljanik commented May 6, 2016

README.md should state install python3-zmq.

MarcoFalke added 3 commits May 6, 2016 12:43
[qa] Refactor test_framework and pull tester
fabbf6b 
* log to stdout
* increase range for p2p and rpc ports
* UPPERCASE_CONSTANTS
* Stop nodes on CTRL+C
[qa] Update README.md
2222dae
[qa] Stop other nodes, even when one fails to stop
fafb33c
@maflcko maflcko force-pushed the Mf1604-qaRefactor branch from fa1c650 to fafb33c Compare May 6, 2016 10:44
@maflcko
Copy link
Member Author

maflcko commented May 6, 2016

@paveljanik Rebased, so the other doc changes are included as well.

@paveljanik
Copy link
Contributor

paveljanik commented May 6, 2016

I have a lot of these during tests, but not only in the PR - also applies to current master:

Running testscript wallet.py ...
Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/testbv4onukz
Unexpected exception caught during testing: timeout('timed out',)
...
Running testscript abandonconflict.py ...
Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/test4arfwesr
Unexpected exception caught during testing: timeout('timed out',)

@maflcko
Copy link
Member Author

maflcko commented May 6, 2016 •
edited
Loading

It's just refactoring. Ideally, behavior should stay the same.

Also, I can't reproduce. Mind to open a issue for the timeouts?

sdaftuar
sdaftuar reviewed May 6, 2016
View reviewed changes
qa/pull-tester/rpc-tests.py Outdated

if print_help:
# Help should be the same for all scripts, so just
# call the first and exit
Copy link
Member

@sdaftuar sdaftuar May 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

comment ultranit: the help is not actually the same for all scripts, as some tests take custom (optional) arguments.

Copy link
Member Author

@maflcko maflcko May 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is true, but the previous behavior was to only show the help text of the first script and then exit(0). If someone wants to change the behavior, I'd propose a separate pull. I am happy to change the comment in this pull, though.

@sdaftuar
Copy link
Member

sdaftuar commented May 6, 2016

utACK (feel free to ignore my ultranit)

Not sure this needs to be addressed in this PR, but for supporting multiple tests running in parallel, I think it might make sense to add support to the test framework for specifying a range of ports to use for the p2p and rpc ports on the command line, which the rpc_tests.py script could use. The logic now of hoping that pid's don't collide modulo some big number (which is now bigger after this PR) is not sufficiently robust in my opinion. Doesn't need to be fixed here, just mentioning it since this PR is touching those functions already.

@fanquake fanquake mentioned this pull request May 8, 2016
Closed
@maflcko
Copy link
Member Author

maflcko commented May 8, 2016

Added a commit to fix @sdaftuar's nit, hopefully. In regard to the ports: I am wondering if it is sufficient to pass in a int seed (default=pid) instead of trying to parse ranges from the command line.

@sdaftuar
Copy link
Member

sdaftuar commented May 8, 2016

@MarcoFalke Yeah that approach of a single int that defaults to the pid sounds reasonable to me. Comment looks better too, thanks.

@maflcko maflcko merged commit fad3366 into bitcoin:master May 9, 2016
maflcko pushed a commit that referenced this pull request May 9, 2016
Merge #7971: [qa] Refactor test_framework and pull tester
4e14afe 
fad3366 [qa] pull-tester: Adjust comment (MarcoFalke)
fafb33c [qa] Stop other nodes, even when one fails to stop (MarcoFalke)
2222dae [qa] Update README.md (MarcoFalke)
fabbf6b [qa] Refactor test_framework and pull tester (MarcoFalke)
@maflcko maflcko deleted the Mf1604-qaRefactor branch May 9, 2016 15:07
@dagurval dagurval mentioned this pull request Sep 3, 2017
Merged
codablock pushed a commit to codablock/dash that referenced this pull request Dec 21, 2017
@codablock
Merge bitcoin#7971: [qa] Refactor test_framework and pull tester
8bafa06 
fad3366 [qa] pull-tester: Adjust comment (MarcoFalke)
fafb33c [qa] Stop other nodes, even when one fails to stop (MarcoFalke)
2222dae [qa] Update README.md (MarcoFalke)
fabbf6b [qa] Refactor test_framework and pull tester (MarcoFalke)
@str4d str4d mentioned this pull request Nov 20, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Dec 2, 2020
@zkbot
Auto merge of #4874 - str4d:rpc-tests-py, r=str4d
7a0a268 
Backport migration from rpc-tests.sh to rpc-tests.py

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6567
- bitcoin/bitcoin#6523
- bitcoin/bitcoin#6616
- bitcoin/bitcoin#6788
  - Only the commit fixing `rpc-tests.py`
- bitcoin/bitcoin#6791
  - Only the fix to `qa/rpc-tests/README.md`
- bitcoin/bitcoin#6827
- bitcoin/bitcoin#6930
- bitcoin/bitcoin#6804
- bitcoin/bitcoin#7029
- bitcoin/bitcoin#7028
- bitcoin/bitcoin#7027
- bitcoin/bitcoin#7135
- bitcoin/bitcoin#7209
- bitcoin/bitcoin#7635
- bitcoin/bitcoin#7778
- bitcoin/bitcoin#7851
- bitcoin/bitcoin#7814
  - Only the changes to the new .py files in this PR.
- bitcoin/bitcoin#7971
- bitcoin/bitcoin#7972
- bitcoin/bitcoin#8056
  - Only the first commit.
- bitcoin/bitcoin#8098
- bitcoin/bitcoin#8104
- bitcoin/bitcoin#8133
  - Only the `rpc-tests.py` commit.
- bitcoin/bitcoin#8066
- bitcoin/bitcoin#8216
  - Only the last two commits.
- bitcoin/bitcoin#8254
- bitcoin/bitcoin#8400
- bitcoin/bitcoin#8482
  - Excluding the first commit (only affects RPC tests we don't have).
- bitcoin/bitcoin#8551
- bitcoin/bitcoin#8607
  - Only the pull-tester commit, for conflict removal.
- bitcoin/bitcoin#8625
- bitcoin/bitcoin#8713
- bitcoin/bitcoin#8750
- bitcoin/bitcoin#8789
- bitcoin/bitcoin#9098
- bitcoin/bitcoin#9276
  - Excluding the second commit (we don't have the changes it requires).
- bitcoin/bitcoin#9657
- bitcoin/bitcoin#9807
- bitcoin/bitcoin#9766
- bitcoin/bitcoin#9823
zkbot added a commit to zcash/zcash that referenced this pull request Dec 2, 2020
@zkbot
Auto merge of #4874 - str4d:rpc-tests-py, r=str4d
c8896f9 
Backport migration from rpc-tests.sh to rpc-tests.py

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6567
- bitcoin/bitcoin#6523
- bitcoin/bitcoin#6616
- bitcoin/bitcoin#6788
  - Only the commit fixing `rpc-tests.py`
- bitcoin/bitcoin#6791
  - Only the fix to `qa/rpc-tests/README.md`
- bitcoin/bitcoin#6827
- bitcoin/bitcoin#6930
- bitcoin/bitcoin#6804
- bitcoin/bitcoin#7029
- bitcoin/bitcoin#7028
- bitcoin/bitcoin#7027
- bitcoin/bitcoin#7135
- bitcoin/bitcoin#7209
- bitcoin/bitcoin#7635
- bitcoin/bitcoin#7778
- bitcoin/bitcoin#7851
- bitcoin/bitcoin#7814
  - Only the changes to the new .py files in this PR.
- bitcoin/bitcoin#7971
- bitcoin/bitcoin#7972
- bitcoin/bitcoin#8056
  - Only the first commit.
- bitcoin/bitcoin#8098
- bitcoin/bitcoin#8104
- bitcoin/bitcoin#8133
  - Only the `rpc-tests.py` commit.
- bitcoin/bitcoin#8066
- bitcoin/bitcoin#8216
  - Only the last two commits.
- bitcoin/bitcoin#8254
- bitcoin/bitcoin#8400
- bitcoin/bitcoin#8482
  - Excluding the first commit (only affects RPC tests we don't have).
- bitcoin/bitcoin#8551
- bitcoin/bitcoin#8607
  - Only the pull-tester commit, for conflict removal.
- bitcoin/bitcoin#8625
- bitcoin/bitcoin#8713
- bitcoin/bitcoin#8750
- bitcoin/bitcoin#8789
- bitcoin/bitcoin#9098
- bitcoin/bitcoin#9276
  - Excluding the second commit (we don't have the changes it requires).
- bitcoin/bitcoin#9657
- bitcoin/bitcoin#9807
- bitcoin/bitcoin#9766
- bitcoin/bitcoin#9823
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maflcko @paveljanik @sdaftuar