Improve rpc-tests.py #9657
Improve rpc-tests.py #9657
Conversation
|
Great! Concept ACK on all of those. I will take a detailed look later.
|
qa/pull-tester/rpc-tests.py
Outdated
| for t in testScripts + testScriptsExt: | ||
| if t in opts or re.sub(".py$", "", t) in opts: | ||
| test_list = [] | ||
| for t in test_scripts: |
There was a problem hiding this comment.
One shouldn't be required to pass --extended to be able to run specific tests from the extended set. You can leave the following here:
for t in BASE_SCRIPTS + EXTENDED_SCRIPTS:
Mhmm, that would still exclude zmq_test.py because of the other change you made...
There was a problem hiding this comment.
Agree. I've fixed this by adding a new ALL_SCRIPTS variable which is the concatenation of the three other lists.
qa/pull-tester/rpc-tests.py
Outdated
| parser.add_argument('--coverage', action='store_true') | ||
| parser.add_argument('-extended', action='store_true') | ||
| parser.add_argument('--help', '-h', '-?', action='store_true') | ||
| parser.add_argument('--parallel', type=int, default=4) |
There was a problem hiding this comment.
If we are considering changing this around, my vote would be for renaming --parallel to be --jobs and also giving it a '-j' alias. The advantage is that it matches make where most people know what -j3 means at a glance.
qa/pull-tester/rpc-tests.py
Outdated
| if arg[:2] == "--": | ||
| passon_args.append(arg) | ||
| else: | ||
| tests.add(arg) |
There was a problem hiding this comment.
Suggest doing this chunk with list/iterator comprehension like so:
tests = set(arg for arg in unknown_args if arg[:2] != "--")
passon_args = [arg for arg in unknown_args if arg[:2] == "--"]
There was a problem hiding this comment.
much nicer. Thanks!
qa/pull-tester/rpc-tests.py
Outdated
| if t in opts or re.sub(".py$", "", t) in opts: | ||
| test_list = [] | ||
| for t in test_scripts: | ||
| if t in tests or re.sub(".py$", "", t) in tests: | ||
| test_list.append(t) |
There was a problem hiding this comment.
Suggest building test_list with list comprehension:
test_list = [t for t in test_scripts if
(t in tests or re.sub(".py$", "", t) in tests)]
qa/pull-tester/rpc-tests.py
Outdated
| parser.add_argument('--help', '-h', '-?', action='store_true', help='print help text and exit') | ||
| parser.add_argument('--nozmq', action='store_true', help='do not run the zmq tests') | ||
| parser.add_argument('--parallel', type=int, default=4, help='how many test scripts to run in parallel. Default=4.') | ||
| parser.add_argument('--win', action='store_true', help='signal that this is running in a Windows environment and that we should run the tests') |
There was a problem hiding this comment.
" and that we should run the tests" sounds weird to me. Are there special tests only for windows? Otherwise we just should run the tests, as always, just taking into account the windows environment.
If there's windows specific tests, what about "signal that this is running in a Windows environment and the windows specific tests sould be run"?
There was a problem hiding this comment.
I haven't changed the text in this argument (the text was previously in the docstring for the module). There are no windows specific tests - this argument simply means "I know I'm running on windows and I definitely want to run the tests." I haven't dug into why this argument was added originally, but feel free to open a new PR if you think it can be removed.
qa/pull-tester/rpc-tests.py
Outdated
| raise | ||
|
|
||
| BASE_SCRIPTS= [ | ||
| # longest test should go first, to favor running tests in parallel |
qa/pull-tester/rpc-tests.py
Outdated
| sys.exit(0) | ||
|
|
||
| coverage = None | ||
| runtests(test_list, config["environment"]["SRCDIR"], config["environment"]["BUILDDIR"], config["environment"]["EXEEXT"], args.parallel, args.coverage, passon_args) |
There was a problem hiding this comment.
Maybe stupid question...not sure how main() can call runtests() without having declared it yet.
There was a problem hiding this comment.
Not a stupid question :)
main() is called in the final line of the module:
if __name__ == '__main__':
main()
by which point all the functions have been declared. I believe this is a fairly common python module structure.
|
Wouldn't f1e8e33 be clearer if https://github.com/bitcoin/bitcoin/blob/master/qa/pull-tester/tests_config.py.in was removed in the same commit? utACK modulo comments. |
Remove the use of wildcard imports in rpc-tests.py and replace with configparser.
b90946a to
6538402
Compare
qa/pull-tester/rpc-tests.py
Outdated
| if t in tests or re.sub(".py$", "", t) in tests: | ||
| test_list.append(t) | ||
| test_list = [t for t in test_scripts if | ||
| (t in ALL_SCRIPTS or re.sub(".py$", "", t) in ALL_SCRIPTS)] |
There was a problem hiding this comment.
This is incorrect and it prevents you from running a single test at a time e.g. $ qa/pull-tester/rpc-tests.py segwit
test_scripts can only be made up of BASE_SCRIPTS, ZMQ_SCRIPTS, and EXTENDED_SCRIPTS. ALL_SCRIPTS is all of those, so test_list will then always be set to all of test_scripts.
I believe it should be:
test_list = [t for t in test_scripts if (t in tests or re.sub(".py$", "", t) in tests)]
There was a problem hiding this comment.
You're completely right. I put the ALL_SCRIPTS in the wrong place.
Should be fixed now.
958bbc7 to
117e0f2
Compare
|
ACK 117e0f2 Tested on Debian 8 environment with normal
Did not test:
|
|
Bike-shedding history, I would have preferred that ALL_SCRIPTS is declared and used in 0eeb09e otherwise the commit is broken. The comprehensive lists in 117e0f2 could also be squashed there. It makes sense to keep s/parallel/jobs separated. |
|
thanks @jtimon - you're right about that historic commit being broken. I did try to merge back and rebase, but it caused merge conflicts, so I didn't bother continuing. If the maintainers really think this is important, I can go back and fix history, but these commits should be merged as a set, so I don't think it matters. |
|
Apologies for the review churn. I've just realised that my last commit introduced a redundant check on the names of the tests to run. I've removed that in commit 1bd87a6 . Once review is complete I'll squash that into the previous commit. |
qa/pull-tester/rpc-tests.py
Outdated
| if config["environment"]["EXEEXT"] == ".exe" and not args.win: | ||
| # https://github.com/bitcoin/bitcoin/commit/d52802551752140cf41f0d9a225a43e84404d3e9 | ||
| # https://github.com/bitcoin/bitcoin/pull/5677#issuecomment-136646964 | ||
| print("Win tests currently disabled by default. Use -win option to enable") |
There was a problem hiding this comment.
Nit: This should use double dash, if you change it above.
It will break git bisect. Maybe you squash all commits that are problematic? Though, I won't block the pull based on this. |
This commit replaces the roll-your-own argument parsing in rpc_tests.py with Python's standard library argparse.
A few miscellaneous improvements to rpc-tests.py command line arguments: - make all arguments start with double dash for consistency - improve help text and output - add nozmq argument to explicitly exclude the ZMQ tests - change 'parallel' to 'jobs'
- add main() - remove global variables
1bd87a6 to
3de3ccd
Compare
Good point. I've now fixed up the history so rpc-tests.py should run at all commits. Branch is now at 3de3ccd . The only difference from the previous branch pointer 117e0f2 (ACK'ed by @jtimon and @isle2983 ) is that I've removed a redundant if test, changed a variable name from |
|
re-utACK 3de3ccd |
qa/pull-tester/tests_config.ini.in
Outdated
|
|
||
| [components] | ||
| # Which components are enabled. These are commented out by `configure` if they were disabled when running config. | ||
| @ENABLE_WALLET_TRUE@ENABLE_WALLET=True |
There was a problem hiding this comment.
Capitalized "True"/"False" seems out of place in an .ini file. Maybe switch to lowercase "yes"/"no" as shown in the documentation: https://docs.python.org/3/library/configparser.html. "yes"/"no" are also understood by the git config tool (https://git-scm.com/docs/git-config#git-config-true), though the canonical form in git is "true"/"false".
There was a problem hiding this comment.
Good point. I'll change these to true and use the getboolean() method
qa/pull-tester/tests_config.ini.in
Outdated
| # These environment variables are set by the build process and read by | ||
| # rpc-tests.py | ||
|
|
||
| [DEFAULT] |
There was a problem hiding this comment.
This section is redundant and could mislead someone who is reading this file into thinking these options are false when they aren't. Suggest removing this section and replacing
ENABLE_WALLET = config["components"]["ENABLE_WALLET"] == "True"
with
ENABLE_WALLET = config["components"].get("ENABLE_WALLET") == "True"
or similar.
There was a problem hiding this comment.
Great! I wasn't aware of the get() method. Agree that removing the defaults section is much better.
qa/pull-tester/rpc-tests.py
Outdated
| parser.add_argument('--help', '-h', '-?', action='store_true') | ||
| parser.add_argument('--parallel', type=int, default=4) | ||
| parser.add_argument('-win', action='store_true') | ||
| (args, unknown_args) = parser.parse_known_args() |
There was a problem hiding this comment.
Parentheses unnecessary.
qa/pull-tester/rpc-tests.py
Outdated
| run_parallel = int(arg.split(sep='=', maxsplit=1)[1]) | ||
| else: | ||
| opts.add(arg) | ||
| print_help = args.help |
There was a problem hiding this comment.
Get rid of these? Why have two names for the same thing?
qa/pull-tester/rpc-tests.py
Outdated
| elif len(opts) == 0 or (len(opts) == 1 and "-win" in opts): | ||
| test_list = testScripts | ||
| # Build list of tests | ||
| if len(tests) != 0: |
There was a problem hiding this comment.
Should avoid if len in python, https://www.python.org/dev/peps/pep-0008/
Yes: if not seq:
if seq:
No: if len(seq):
if not len(seq):
More occurrences a few places below, too.
qa/pull-tester/rpc-tests.py
Outdated
| # Individual tests have been specified. Run specified tests that exist | ||
| # in the ALL_SCRIPTS list. Accept the name with or without .py extension. | ||
| test_list = [t for t in ALL_SCRIPTS if | ||
| (t in tests or re.sub(".py$", "", t) in tests)] |
There was a problem hiding this comment.
Seems like "t in tests or " is redundant.
There was a problem hiding this comment.
This is required if the user specifies a test with the .py suffix, eg:
rpc-tests.py rpcnamedargs.py
qa/pull-tester/rpc-tests.py
Outdated
| config = configparser.ConfigParser() | ||
| config.read_file(open(os.path.dirname(__file__) + "/tests_config.ini")) | ||
|
|
||
| ENABLE_WALLET = config["components"]["ENABLE_WALLET"] == "True" |
There was a problem hiding this comment.
Maybe just use the config object directly below instead of defining these variables, since they are only used once. This will cut down on the number of global variables in this script, and make it clearer from the context where the settings are used where the settings come from.
There was a problem hiding this comment.
yep - I set this as a global variable to match the existing layout. It's been moved to be a local variable in the subsequent refactor commit.
I could remove the variable entirely and access the config directly in the if test, but I don't think it makes much difference at this point.
qa/pull-tester/rpc-tests.py
Outdated
| parser.add_argument('--help', '-h', '-?', action='store_true', help='print help text and exit') | ||
| parser.add_argument('--jobs', '-j', type=int, default=4, help='how many test scripts to run in parallel. Default=4.') | ||
| parser.add_argument('--nozmq', action='store_true', help='do not run the zmq tests') | ||
| parser.add_argument('--win', action='store_true', help='signal that this is running in a Windows environment and that we should run the tests') |
There was a problem hiding this comment.
Since you're adding a dash to this argument maybe consider renaming it something comprehensible like --force. And changing help text to "Run tests even on platforms where they are disabled by default (e.g. windows)."
The "signal that this is running in a windows environment" just comes off as strange, since the script obviously knows what environment it is running in.
There was a problem hiding this comment.
Yes. @jtimon made the same point that the help text was nonsense. Good point about changing the name of the parameter since I'm already breaking backward compatibility :)
I'll change this to --force and update the help text.
qa/pull-tester/rpc-tests.py
Outdated
| BASE_SCRIPTS= [ | ||
| # Scripts that are run by the travis build process |
There was a problem hiding this comment.
These two lines look like one sentence, would add punctuation.
qa/pull-tester/rpc-tests.py
Outdated
| coverage = None | ||
| runtests(test_list, config["environment"]["SRCDIR"], config["environment"]["BUILDDIR"], config["environment"]["EXEEXT"], args.jobs, args.coverage, passon_args) | ||
|
|
||
| def runtests(test_list, src_dir, build_dir, exeext, jobs=1, enable_coverage=False, args=[]): |
There was a problem hiding this comment.
Maybe call it run_tests for pep8.
|
utACK 3de3ccd |
|
Thanks for the thorough review @ryanofsky . I've applied most of those markups in a new commit. I haven't squashed them into the separate commits since there was quite a lot of code churn within those commits and it'd be tedious to get the history right. |
There was a problem hiding this comment.
utACK a6a3e58
There might be some minor issues (see feedback), but I think this is ready for merge nonetheless, as it comes with some great features that make future changes to the test framework a lot easier.
The feedback may be addressed in a separate pull, otherwise it would slow down progress in other test related pulls.
| REFERENCE_FILENAME = 'rpc_interface.txt' | ||
| COVERAGE_FILE_PREFIX = 'coverage.' | ||
| reference_filename = 'rpc_interface.txt' | ||
| coverage_file_prefix = 'coverage.' |
There was a problem hiding this comment.
Even though of local scope, those still denote global constants, so should be ALL_UPPER_CASE?
|
|
||
| if ENABLE_COVERAGE: | ||
| flags = ["--srcdir=" + src_dir] + args |
There was a problem hiding this comment.
I know this has no effect, but this was build_dir previously and should not be changed in this commit.
| parser.add_argument('--extended', action='store_true', help='run the extended test suite in addition to the basic tests') | ||
| parser.add_argument('--force', '-f', action='store_true', help='run tests even on platforms where they are disabled by default (e.g. windows).') | ||
| parser.add_argument('--help', '-h', '-?', action='store_true', help='print help text and exit') | ||
| parser.add_argument('--jobs', '-j', type=int, default=4, help='how many test scripts to run in parallel. Default=4.') |
There was a problem hiding this comment.
If you change the name of the arguments, make sure to update the docs https://github.com/bitcoin/bitcoin/tree/master/qa#running-tests and maybe even put a tiny notice in the release notes that says that some of the arguments to rpc-test.py have changed. Alternatively you could provide aliases for compatibility.
|
Thanks @ryanofsky for suggesting the |
|
Thanks @MarcoFalke . I'll make sure to track that feedback and address it early next week. |
* Merge bitcoin#9744: Remove unused module from rpc-tests a432aa0 Remove unused module from rpc-tests (Takashi Mitsuta) * Merge bitcoin#9696: [trivial] Fix recently introduced typos in comments 0c9b9b7 [trivial] Fix recently introduced typos in comments (practicalswift) * Merge bitcoin#9657: Improve rpc-tests.py a6a3e58 Various review markups for rpc-tests.py improvements (John Newbery) 3de3ccd Refactor rpc-tests.py (John Newbery) afd38e7 Improve rpc-tests.py arguments (John Newbery) 91bffff Use argparse in rpc_tests.py (John Newbery) 1581ecb Use configparser in rpc-tests.py (John Newbery) * Merge bitcoin#9724: Qt/Intro: Add explanation of IBD process f6d18f5 Qt/Intro: Explain a bit more what will happen first time (Luke Dashjr) 50c5657 Qt/Intro: Storage shouldn't grow significantly with pruning enabled (Luke Dashjr) 9adb694 Qt/Intro: Move sizeWarningLabel text into C++ code (Luke Dashjr) * Merge bitcoin#9794: Minor update to qrencode package builder 1bfe6b4 Use package name variable inside $(package)_file_name variable (Mitchell Cash) * Merge bitcoin#9726: netbase: Do not print an error on connection timeouts through proxy 3ddfe29 netbase: Do not print an error on connection timeouts through proxy (Wladimir J. van der Laan) 13f6085 netbase: Make InterruptibleRecv return an error code instead of bool (Wladimir J. van der Laan) * Merge bitcoin#9727: Remove fallbacks for boost_filesystem < v3 056aba2 Remove fallbacks for boost_filesystem < v3 (Wladimir J. van der Laan) * Merge bitcoin#9485: ZMQ example using python3 and asyncio b471daf Adddress nits, use asyncio signal handling, create_task (Bob McElrath) 4bb7d1b Add python version checks and 3.4 example (Bob McElrath) 5406d51 Rewrite to not use Polling wrapper for asyncio, link to python2.7 example (Bob McElrath) 5ea5368 ZMQ example using python3 and asyncio (Bob McElrath) * Merge bitcoin#9807: RPC doc fix-ups. 851f6a3 [qa][doc] Correct rpc test options in readme (fanquake) 41e7219 [trivial] Add tests_config.ini to .gitignore (fanquake) * Dashify Co-Authored-By: PastaPastaPasta <pasta@dashboost.org> * Change file permissions * update travis.yml -parallel -> --jobs
* Merge bitcoin#9744: Remove unused module from rpc-tests a432aa0 Remove unused module from rpc-tests (Takashi Mitsuta) * Merge bitcoin#9696: [trivial] Fix recently introduced typos in comments 0c9b9b7 [trivial] Fix recently introduced typos in comments (practicalswift) * Merge bitcoin#9657: Improve rpc-tests.py a6a3e58 Various review markups for rpc-tests.py improvements (John Newbery) 3de3ccd Refactor rpc-tests.py (John Newbery) afd38e7 Improve rpc-tests.py arguments (John Newbery) 91bffff Use argparse in rpc_tests.py (John Newbery) 1581ecb Use configparser in rpc-tests.py (John Newbery) * Merge bitcoin#9724: Qt/Intro: Add explanation of IBD process f6d18f5 Qt/Intro: Explain a bit more what will happen first time (Luke Dashjr) 50c5657 Qt/Intro: Storage shouldn't grow significantly with pruning enabled (Luke Dashjr) 9adb694 Qt/Intro: Move sizeWarningLabel text into C++ code (Luke Dashjr) * Merge bitcoin#9794: Minor update to qrencode package builder 1bfe6b4 Use package name variable inside $(package)_file_name variable (Mitchell Cash) * Merge bitcoin#9726: netbase: Do not print an error on connection timeouts through proxy 3ddfe29 netbase: Do not print an error on connection timeouts through proxy (Wladimir J. van der Laan) 13f6085 netbase: Make InterruptibleRecv return an error code instead of bool (Wladimir J. van der Laan) * Merge bitcoin#9727: Remove fallbacks for boost_filesystem < v3 056aba2 Remove fallbacks for boost_filesystem < v3 (Wladimir J. van der Laan) * Merge bitcoin#9485: ZMQ example using python3 and asyncio b471daf Adddress nits, use asyncio signal handling, create_task (Bob McElrath) 4bb7d1b Add python version checks and 3.4 example (Bob McElrath) 5406d51 Rewrite to not use Polling wrapper for asyncio, link to python2.7 example (Bob McElrath) 5ea5368 ZMQ example using python3 and asyncio (Bob McElrath) * Merge bitcoin#9807: RPC doc fix-ups. 851f6a3 [qa][doc] Correct rpc test options in readme (fanquake) 41e7219 [trivial] Add tests_config.ini to .gitignore (fanquake) * Dashify Co-Authored-By: PastaPastaPasta <pasta@dashboost.org> * Change file permissions * update travis.yml -parallel -> --jobs
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
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
rpc-tests.py was converted from a shell script into python in #6616 and it shows. There are several idioms which make sense for a shell script, but are considered bad practice in a python module. This PR cleans up some of the quirks.
This pull request combines several commits which improve the code style of rpc-tests.py and move away from bad practices. Reviews/comments for any or all of the commits are welcomed:
from tests_config import *wildcard import with a .ini file and the Python standard library configparser module. Benefits are:[EDIT: removed commit hashes since I've now rebased]