Simplify test_runner.py a bit #23995

A few simplifications to test_runner.py to make it easier to reason about (and perhaps fix #23799#pullrequestreview-845657169):

• Remove the num_running variable as it should be implied by the length of the jobs variable.
• Remove the i variable as it should be implied by the length of the test_results variable.
• Instead of counting results to determine if we're done, make the queue object itself responsible (by looking at running jobs and jobs left).

I think the problem with #23799 was that it introduced a second outer loop while i < test_count: which broke the --failfast option. If that option was chosen and a test failed, the break would leave the inner loop, but since the condition of the outer loop was not fulfilled, the inner loop would just be started again.

As far as I can see, this problem is still there (even though I can't reproduce the "pop from empty list" with this branch).

I see the same behavior as @mzumsande: --failfast option doesn't work, and can't reproduce "pop from empty list".

Here is a suggested fix. (I don't know how to suggest changes on lines unchanged.) Any feedback is appreciated as it is a learning opportunity.

• add failed attribute to TestHandler class

class TestHandler:
    """
    Trigger the test scripts passed in via the list.
    """

    def __init__(self, *, num_tests_parallel, tests_dir, tmpdir, test_list, flags, use_term_control, failed):
        assert num_tests_parallel >= 1
        self.num_jobs = num_tests_parallel
        self.tests_dir = tests_dir
        self.tmpdir = tmpdir
        self.test_list = test_list
        self.flags = flags
        self.jobs = []
        self.use_term_control = use_term_control
        self.failed = failed

job_queue = TestHandler(
    num_tests_parallel=jobs,
    tests_dir=tests_dir,
    tmpdir=tmpdir,
    test_list=test_list,
    flags=flags,
    use_term_control=use_term_control,
    failed=False
)

• modify 'failed' in case of fast fail

if failfast:
    job_queue.failed = True
    logging.debug("Early exiting after test failure")
    break

• modify done method

def done(self):
    return self.failed or (not (self.jobs or self.test_list))

I think the problem with #23799 was that it introduced a second outer loop while i < test_count: which broke the --failfast option. If that option was chosen and a test failed, the break would leave the inner loop, but since the condition of the outer loop was not fulfilled, the inner loop would just be started again.

failfast is very useful when having smol computing power

if this languge supports multiple block breaking (as many others do) I suppose why wouldnt you have it break 2 as an urgent amend to 23799 while something more calmly thought is coming

TODO: add some tests to test the tests framework

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #24195 (test: Fix failfast option for functional test runner by mzumsande)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

lgtm, but this still needs rebase, either here or in a separate pull.

Concept ACK

Closing. #23799 is fixed through #24195, and I don't feel like rebasing the other changes here.

Marked up for grabs.