test: simplify test_runner.py #29497

tdb3 · 2024-02-28T00:28:12Z

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).

DrahtBot · 2024-02-28T00:28:15Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

kevkevinpal · 2024-03-02T22:22:19Z

I tried this with --failfast and it seemed to properly fail fast

tdb3 · 2024-03-02T22:35:46Z

I tried this with --failfast and it seemed to properly fail fast

Thank you for the test

kevkevinpal · 2024-03-02T22:42:46Z

I tested with this diff e82b342

and ran ./test/functional/test_runner.py --failfast

davidgumberg · 2024-03-11T14:53:49Z

test/functional/test_runner.py

@@ -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)


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 /.

davidgumberg · 2024-03-11T14:56:54Z

test/functional/test_runner.py

        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])


Nit: We could use an f-string here and replace test_count with len(test_list)

Suggested change

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]}"

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 · 2024-03-11T21:56:24Z

Thanks for the review @davidgumberg!
Force pushed. Responses inline.

marcofleon

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.

davidgumberg · 2024-03-12T00:40:21Z

reACK 8f2d814

mzumsande

Code Review ACK 8f2d814

tdb3 · 2024-03-12T21:52:05Z

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.

Thank you for the review @marcofleon!

tdb3 · 2024-03-12T21:52:37Z

Code Review ACK 8f2d814

Thank you for the review @mzumsande!

@sipa

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>

tdb3 · 2024-03-12T22:01:17Z

rebased

mzumsande · 2024-03-12T22:28:11Z

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).

tdb3 · 2024-03-12T22:33:27Z

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).

davidgumberg · 2024-03-12T23:30:41Z

reACK 0831b54
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

marcofleon

re-ACK 0831b54

maflcko

lgtm

0831b54 test: simplify test_runner.py (tdb3) Pull request description: Implements the simplifications to test_runner.py proposed by sipa in PR bitcoin#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). ACKs for top commit: mzumsande: re-ACK 0831b54 davidgumberg: reACK bitcoin@0831b54 marcofleon: re-ACK 0831b54 Tree-SHA512: e5473e68d49cd779b29d97635329283ae7195412cb1e92461675715ca7eedb6519a1a93ba28d40ca6f015d270f7bcd3e77cef279d9cd655155ab7805b49638f1

b70e091 Merge bitcoin#29667: fuzz: actually test garbage >64b in p2p transport test (fanquake) 6d7aa3d Merge bitcoin#29497: test: simplify test_runner.py (fanquake) d0e15d5 Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper conversions (Ava Chow) 045fa5f Merge bitcoin#29514: tests: Provide more helpful assert_equal errors (Ava Chow) bd607f0 Merge bitcoin#29393: i2p: log connection was refused due to arbitrary port (Ava Chow) c961755 Merge bitcoin#29595: doc: Wrap flags with code in developer-notes.md (fanquake) 8d6e5e7 Merge bitcoin#29583: fuzz: Apply fuzz env (suppressions, etc.) when fetching harness list (fanquake) 4dce690 Merge bitcoin#29576: Update functional test runner to return error code when no tests are found to run (fanquake) 910a7d6 Merge bitcoin#29529: fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake) fdac2b3 Merge bitcoin#29493: subtree: update crc32c subtree (fanquake) a23b342 Merge bitcoin#29475: doc: Fix Broken Links (fanquake) 92bad90 Merge bitcoin#28178: fuzz: Generate with random libFuzzer settings (fanquake) 9b6a05d Merge bitcoin#29443: depends: fix BDB compilation on OpenBSD (fanquake) 9963e6b Merge bitcoin#29413: fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse` (fanquake) 3914745 Merge bitcoin#29425: test: fix intermittent failure in wallet_reorgrestore.py (fanquake) b719883 Merge bitcoin#29399: test: Fix utxo set hash serialisation signedness (fanquake) f096880 Merge bitcoin#29377: test: Add makefile target for running unit tests (Ava Chow) 03e0bd3 Merge bitcoin#27319: addrman, refactor: improve stochastic test in `AddSingle` (Ava Chow) Pull request description: ## Issue being fixed or feature implemented Batch of trivial backports ## What was done? See commits ## How Has This Been Tested? built locally; large combined merge passed tests locally ## Breaking Changes Should be none ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b70e091 knst: utACK b70e091 Tree-SHA512: 659a931f812c8a92cf3854abb873d92885219a392d6aa8e49ac4b27fe41e8e163ef9a135050e7c2e1bd33cecd2f7dae215e05a9c29f62e837e0057d3c16746d6

DrahtBot added the Tests label Feb 28, 2024

davidgumberg reviewed Mar 11, 2024

View reviewed changes

tdb3 force-pushed the 20240227_testrunnersimplification branch from 0dfd4c2 to 8f2d814 Compare March 11, 2024 21:55

marcofleon approved these changes Mar 12, 2024

View reviewed changes

mzumsande reviewed Mar 12, 2024

View reviewed changes

tdb3 force-pushed the 20240227_testrunnersimplification branch from 8f2d814 to 0831b54 Compare March 12, 2024 22:00

DrahtBot requested review from davidgumberg and marcofleon March 12, 2024 22:28

marcofleon reviewed Mar 14, 2024

View reviewed changes

maflcko approved these changes Mar 14, 2024

View reviewed changes

fanquake merged commit 6850d72 into bitcoin:master Mar 14, 2024

bitcoin locked and limited conversation to collaborators Mar 14, 2025

		@@ -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)

	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]}"

test: simplify test_runner.py #29497

test: simplify test_runner.py #29497

Uh oh!

Conversation

tdb3 commented Feb 28, 2024 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

kevkevinpal commented Mar 2, 2024

Uh oh!

tdb3 commented Mar 2, 2024

Uh oh!

kevkevinpal commented Mar 2, 2024

Uh oh!

davidgumberg Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

tdb3 Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidgumberg Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

tdb3 Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

tdb3 commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcofleon left a comment

Choose a reason for hiding this comment

Uh oh!

davidgumberg commented Mar 12, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 commented Mar 12, 2024

Uh oh!

tdb3 commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdb3 commented Mar 12, 2024

Uh oh!

mzumsande commented Mar 12, 2024

Uh oh!

tdb3 commented Mar 12, 2024

Uh oh!

davidgumberg commented Mar 12, 2024

Uh oh!

marcofleon left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdb3 commented Feb 28, 2024 •

edited by fanquake

Loading

DrahtBot commented Feb 28, 2024 •

edited

Loading

tdb3 Mar 11, 2024 •

edited

Loading

tdb3 commented Mar 11, 2024 •

edited

Loading

tdb3 commented Mar 12, 2024 •

edited

Loading