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).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | mzumsande, davidgumberg, marcofleon |
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 --failfast and it seemed to properly fail fast
I tried this with
--failfastand it seemed to properly fail fast
Thank you for the test
I tested with this diff e82b3421a8d5fb82fe20e670857abbbe220ae09e
and ran ./test/functional/test_runner.py --failfast
612 | @@ -613,14 +613,12 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage=
613 | max_len_name = len(max(test_list, key=len))
614 | test_count = len(test_list)
Maybe we can remove test_count? See below.
test_count being assigned outside of the loop enables reporting of the correct number of total tests on the right side of the /.
620 | break 621 | for test_result, testdir, stdout, stderr, skip_reason in job_queue.get_next(): 622 | test_results.append(test_result) 623 | - i += 1 624 | - done_str = "{}/{} - {}{}{}".format(i, test_count, BOLD[1], test_result.name, BOLD[0]) 625 | + done_str = "{}/{} - {}{}{}".format(len(test_results), test_count, BOLD[1], test_result.name, BOLD[0])
Nit: We could use an f-string here and replace test_count with len(test_list)
done_str = f"{len(test_results)}/{len(test_list)} - {BOLD[1]}{test_result.name}{BOLD[0]}"
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).
Thanks for the review @davidgumberg! Force pushed. Responses inline.
Tested ACK 8f2d814407c280f863be0303b4c2c1efdc0b8f36.
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.
Code Review ACK 8f2d814407c280f863be0303b4c2c1efdc0b8f36
Tested ACK 8f2d814407c280f863be0303b4c2c1efdc0b8f36.
Reviewed the code changes and I think they make the program a bit more readable. I ran
test_runner.pyand there were no issues. Also tested with--failfastand it did indeed fail fast at the expected test.
Thank you for the review @marcofleon!
Code Review ACK 8f2d814407c280f863be0303b4c2c1efdc0b8f36
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 #23995.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
rebased
re-ACK 0831b54
rebased
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).
re-ACK 0831b54
rebased
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 https://github.com/bitcoin/bitcoin/commit/0831b54dfca1b9e728295fff500215da14589fc0
Verified that the commit is unchanged with range-diff, and re-ran make check:
$ git range-diff a945f09...8f2d814 1105aa4..0831b54
1: 8f2d814407 = 1: 0831b54dfc test: simplify test_runner.py
re-ACK 0831b54dfca1b9e728295fff500215da14589fc0
lgtm