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.
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
RPC Python2 Test Runner #3780
RPC Python2 Test Runner #3780
Conversation
#3781 can be used to test these on Windows. |
qa/rpc-tests/test.py
Outdated
def print_test_results(test_list): | ||
i =0 | ||
for test in test_list: | ||
logging.info("%s : %s", test[i], test[i+1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this printing by twos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is trying to adjust for the fact that stats
is being appended for each test (so there is excess data in each test_list
entry). test[i]
will contain the filename; test[i+1]
will contain the elapsed time.
This will break (in that it will print incorrect information) if a test fails, because pass_list
won't contain an entry for every test. Say there are four tests, A through D, and test C fails. Then pass_list
will contain:
[
['fileA', 'elapsedA'],
['fileA', 'elapsedA', 'fileB', 'elapsedB'],
['fileA', 'elapsedA', 'fileB', 'elapsedB', 'fileC', 'elapsedC', 'fileD', 'elapsedD'],
]
The current logic, when given pass_list
, will print the information for tests [A, B, C]
instead of [A, B, D]
. With more failed tests, the offset will increase correspondingly. fail_list
will likely only just print passing test information.
test_list
should instead contain 2-tuples (thus only containing pass or failure information, not both):
[
('fileA', 'elapsedA'),
('fileB', 'elapsedB'),
('fileD', 'elapsedD'),
]
This could then be printed with:
logging.info("%s : %s" % test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Str4d is correct about the counting by twos, and the issues with failed/interrupted test scenarios. Looking to address this in next round of commits.
if p1.returncode != 0: | ||
fail_list.append(stats) | ||
else: | ||
pass_list.append(stats) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this -- stats
is always the same object within this function, so we're appending that object multiple times?
qa/rpc-tests/test.py
Outdated
def print_test_results(test_list): | ||
i =0 | ||
for test in test_list: | ||
logging.info("%s : %s", test[i], test[i+1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is trying to adjust for the fact that stats
is being appended for each test (so there is excess data in each test_list
entry). test[i]
will contain the filename; test[i+1]
will contain the elapsed time.
This will break (in that it will print incorrect information) if a test fails, because pass_list
won't contain an entry for every test. Say there are four tests, A through D, and test C fails. Then pass_list
will contain:
[
['fileA', 'elapsedA'],
['fileA', 'elapsedA', 'fileB', 'elapsedB'],
['fileA', 'elapsedA', 'fileB', 'elapsedB', 'fileC', 'elapsedC', 'fileD', 'elapsedD'],
]
The current logic, when given pass_list
, will print the information for tests [A, B, C]
instead of [A, B, D]
. With more failed tests, the offset will increase correspondingly. fail_list
will likely only just print passing test information.
test_list
should instead contain 2-tuples (thus only containing pass or failure information, not both):
[
('fileA', 'elapsedA'),
('fileB', 'elapsedB'),
('fileD', 'elapsedD'),
]
This could then be printed with:
logging.info("%s : %s" % test)
qa/rpc-tests/test.py
Outdated
def run_test_scripts(test_list, args): | ||
fail_list = [] | ||
pass_list = [] | ||
stats = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this, and instead construct a fresh 2-tuple for each test containing the desired information. Then simplify the printing logic per my comment there.
if args.listextended: | ||
opt_listextended() | ||
if args.individual: | ||
opt_individual(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the -i
argument, and instead check if args.args
is not empty, to decide whether to run provided tests or all tests.
#might want to add routine to properly clean SIGTERM tests so they don't conga line the rest | ||
try: | ||
start_time = time.time() | ||
p1 = Popen(['python', file]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaves off various arguments and flags that are supposed to be provided to the individual RPC tests. See our current test runner, as well as upstream's Python test runner as of 0.12.0.
Additionally, executing with python
ignores the environment command at the top of each file. This might be useful for a CI system, but probably isn't what is indended for general usage, so if the intent is the latter then this should probably be turned on with a CLI flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working through some of the upstream nuances, but it appears the defaults are redundant to the env BITCOIND
and BITCOINCLI
. Perhaps there are other binaries upstream uses inside of what they user for srcdir
, but from what I have tinkered with it doesn't seem we use other binaries in the src dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also appears that the other major upgrade is the coverage python file. I'd have to go look at what it does in the field. At a glance it seems to just dump pathways of control flow from RPC, with a given set of params.
Addressed a couple comments, and will now check out some of the timeout/ctrl-c items mentioned. |
This new script is intended to allow the RPC stack to run on a given platform (e.g Windows, Linux, MacOS, ARM?). There will be a separate PR associated with this, which will allow the RPC stack to gracefully run on Windows. (Update: #3781 is separate PR )
I have a few cleanup/graceful state maintenance functions to add. I am currently testing these per platform, but it should be done soon.