test: simplify test_runner.py #29497
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
I tried this with |
Thank you for the test |
|
I tested with this diff e82b342 and ran |
| @@ -613,14 +613,12 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage= | |||
| max_len_name = len(max(test_list, key=len)) | |||
| test_count = len(test_list) | |||
There was a problem hiding this comment.
Maybe we can remove test_count? See below.
There was a problem hiding this comment.
test_count being assigned outside of the loop enables reporting of the correct number of total tests on the right side of the /.
test/functional/test_runner.py
Outdated
| if failfast and not all_passed: | ||
| break | ||
| for test_result, testdir, stdout, stderr, skip_reason in job_queue.get_next(): | ||
| test_results.append(test_result) | ||
| i += 1 | ||
| done_str = "{}/{} - {}{}{}".format(i, test_count, BOLD[1], test_result.name, BOLD[0]) | ||
| done_str = "{}/{} - {}{}{}".format(len(test_results), test_count, BOLD[1], test_result.name, BOLD[0]) |
There was a problem hiding this comment.
Nit: We could use an f-string here and replace test_count with len(test_list)
| done_str = "{}/{} - {}{}{}".format(len(test_results), test_count, BOLD[1], test_result.name, BOLD[0]) | |
| done_str = f"{len(test_results)}/{len(test_list)} - {BOLD[1]}{test_result.name}{BOLD[0]}" |
There was a problem hiding this comment.
Thanks, updating to an f-string better aligns with the style guidelines (and that line is being changed anyway).
Leaving other format strings untouched for now (those could be updated in another refactor PR).
tdb3
force-pushed
the
20240227_testrunnersimplification
branch
from
0dfd4c2
to
8f2d814
Compare
|
Thanks for the review @davidgumberg! |
There was a problem hiding this comment.
Tested ACK 8f2d814.
Reviewed the code changes and I think they make the program a bit more readable. I ran test_runner.py and there were no issues. Also tested with --failfast and it did indeed fail fast at the expected test.
|
reACK 8f2d814 |
Thank you for the review @marcofleon! |
Thank you for the review @mzumsande! |
Remove the num_running variable as it can be implied by the length of the jobs list. Remove the i variable as it can be implied by the length of the test_results list. Instead of counting results to determine if finished, make the queue object itself responsible (by looking at running jobs and jobs left). Originally proposed by @sipa in PR bitcoin#23995. Co-authored-by: Pieter Wuille <pieter@wuille.net>
tdb3
force-pushed
the
20240227_testrunnersimplification
branch
from
8f2d814
to
0831b54
Compare
|
rebased |
|
re-ACK 0831b54
Why? This looked ready for merge with 3 ACKs. A rebase invalidates existing ACKs, so once these exist it should only be done if there is a conflict (Drahtbot will complain) or a specific reason (such as. a silent conflict). The CI will always rebase on master anyway for its runs (which could be restarted without rebasing if really necessary). |
Thank you for the insight, didn't realize that. I'll avoid that moving forward. The rationale was to bring it to head while reinitiating CI (MacOS brew issue). |
|
reACK 0831b54 |
Implements the simplifications to test_runner.py proposed by sipa in PR #23995.
Remove the num_running variable as it can be implied by the length of the jobs list.
Remove the i variable as it can be implied by the length of the test_results list.
Instead of counting results to determine if finished, make the queue object itself
responsible (by looking at running jobs and jobs left).