bpo-44708: Only re-run test methods that match names of previously failing test methods #27287

ambv · 2021-07-22T14:36:53Z

Details on the issue. It's probably easiest to review this commit by commit as every commit is an atomic change.

I'd also like to backport to 3.10 and 3.9 to speed up open PRs on those branches as well.

https://bugs.python.org/issue44708

…methods

…kipped)

ambv · 2021-07-22T14:41:02Z

Lib/test/libregrtest/main.py

            self.good.append(test_name)
-        elif ok in (FAILED, CHILD_ERROR):


The different order here is required due to how inheritance works. isinstance(Failed) would catch all the lower cases.

ambv · 2021-07-22T14:44:37Z

Lib/test/libregrtest/main.py


-        if rerun and ok not in {FAILED, CHILD_ERROR, INTERRUPTED}:
+        if rerun and not isinstance(result, (Failed, Interrupted)):


ChildError is a Failed subclass so we don't have to enumerate it. Additionally, I feel like this list was missing other failures like uncaught exceptions, env changeds, and refleaks. Now it doesn't.

For refleaks/envchanged I don't know if it makes sense to rerun because it those fail, then there is no way that the test suite is going to succeed after that, no? OTOH I don't see how it would hurt (just let's make sure that it works as we expect and it doesn't interfere with the refleak machinery).

This function is a little cryptic. The rerun boolean here means "we are already re-running" and this if statement figures out whether we should remove the current test that just re-ran from "bad" tests (which were populated on the first run). The bug that was there previously is that a refleak or env change in the second run would count as an "ok" outcome and the test would be removed from "bad", leading to a green CI badge that's wrong.

ambv · 2021-07-22T14:45:30Z

Lib/test/libregrtest/main.py

+                self.ns.match_tests.extend(error_names)
+                self.ns.match_tests.extend(failure_names)


This is the money shot.

ambv · 2021-07-22T14:46:57Z

Lib/test/libregrtest/runtest.py

-PASSED = 1
-FAILED = 0
-ENV_CHANGED = -1
-SKIPPED = -2
-RESOURCE_DENIED = -3
-INTERRUPTED = -4
-CHILD_ERROR = -5   # error in a child process
-TEST_DID_NOT_RUN = -6
-TIMEOUT = -7


Replacing those with objects was necessary to store related context when returning, which allows matching test method names in main.py. This helps with consistency when running across processes as well.

This is an excellent change, dealing with these have been quite painful before

ambv · 2021-07-22T14:48:22Z

Lib/test/libregrtest/runtest.py

-        ENV_CHANGED      test failed because it changed the execution environment
-        FAILED           test failed
-        PASSED           test passed
-        EMPTY_TEST_SUITE test ran no subtests.


This docstring was outdated anyway.

ambv · 2021-07-22T14:48:48Z

Lib/test/libregrtest/runtest_mp.py

    ns_dict, test_name = json.loads(worker_args)
-    ns = types.SimpleNamespace(**ns_dict)
+    ns = Namespace(**ns_dict)


Custom Namespace subclass pays off.

ambv · 2021-07-22T14:51:44Z

Lib/test/libregrtest/runtest.py

+        if display_failure:
+            msg = f"{msg} -- {exc}"
+        print(msg, file=sys.stderr, flush=True)
+        return Failed(test_name, errors=exc.errors, failures=exc.failures)


This is how we store the failed and erroring method names to be matched later for re-running.

Fidget-Spinner · 2021-07-22T15:59:44Z

Lib/test/libregrtest/runtest.py

+        self,
+        name: str,
+        duration_sec: float = 0.0,
+        xml_data: list[str] | None = None,


Drive-by comment: T1 | T2 syntax is 3.10 only and won't be able to land in 3.9 unless you use from __future__ import annotations or stringify them manually. Ditto for other occurrences below.

I don't have enough experience in libregrtest to review the rest, but this is a change I look forward to :).

pablogsal · 2021-07-22T17:21:05Z

Lib/test/libregrtest/runtest.py

+        if display_failure:
+            msg = f"{msg} -- {exc}"
+        print(msg, file=sys.stderr, flush=True)
+        return Failed(test_name, errors=exc.errors, failures=exc.failures)
    except support.TestFailed as exc:


In what scenarios are we are raising Failed but not TestFailedWithDetails now?

There are quite a few bare TestFailed in custom test_main functions in various test_*.py files. Example:

cpython/Lib/test/test_ssl.py

Line 5044 in c05a790

raise support.TestFailed("Can't read certificate file %r" % filename)

Lib/test/libregrtest/main.py

pablogsal · 2021-07-22T17:38:59Z

Lib/test/libregrtest/main.py

+            if errors or failures:
+                if self.ns.match_tests is None:
+                    self.ns.match_tests = []
+                self.ns.match_tests.extend(error_names)


Question: Do we want to rerun errors? In theory those are things like failures to set up the test and similar. Do you think that this is something we should run again?

The only case where we gather errors is with TestFailedWithDetails which means those are unittest failures and/or errors. We've been re-running the entire file in this scenario before. I think we should still as some of those are exceptions raised due to race conditions. We might think of tightening this up in a future update.

pablogsal

Fantastic job! 🚀 In general LGTM!

I left some comments with some questions and suggestions.

Co-authored-by: Pablo Galindo Salgado <[email protected]>

miss-islington · 2021-07-22T18:41:48Z

Thanks @ambv for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington · 2021-07-22T18:41:48Z

Thanks @ambv for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2021-07-22T18:41:52Z

Sorry, @ambv, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f1afef5e0d93d66fbf3c9aaeab8b3b8da9617583 3.9

miss-islington · 2021-07-22T18:41:54Z

Sorry @ambv, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker f1afef5e0d93d66fbf3c9aaeab8b3b8da9617583 3.10

miss-islington · 2021-07-22T18:59:59Z

Thanks @ambv for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

bedevere-bot · 2021-07-22T19:00:07Z

GH-27290 is a backport of this pull request to the 3.10 branch.

…iling test methods (pythonGH-27287) * Move to a static argparse.Namespace subclass * Roughly annotate runtest.py * Refactor libregrtest to use lossless test result objects * Only re-run test methods that match names of previously failing test methods * Adopt tests to cover test method name matching Co-authored-by: Pablo Galindo Salgado <[email protected]> (cherry picked from commit f1afef5) Co-authored-by: Łukasz Langa <[email protected]>

…sly failing test methods (pythonGH-27287) * Move to a static argparse.Namespace subclass * Roughly annotate runtest.py * Refactor libregrtest to use lossless test result objects * Only re-run test methods that match names of previously failing test methods * Adopt tests to cover test method name matching Co-authored-by: Pablo Galindo Salgado <[email protected]>. (cherry picked from commit f1afef5) Co-authored-by: Łukasz Langa <[email protected]>

…iling test methods (GH-27287) (GH-27290) * Move to a static argparse.Namespace subclass * Roughly annotate runtest.py * Refactor libregrtest to use lossless test result objects * Only re-run test methods that match names of previously failing test methods * Adopt tests to cover test method name matching Co-authored-by: Pablo Galindo Salgado <[email protected]> (cherry picked from commit f1afef5) Co-authored-by: Łukasz Langa <[email protected]>

…sly failing test methods (GH-27287) (GH-27293) * Move to a static argparse.Namespace subclass * Roughly annotate runtest.py * Refactor libregrtest to use lossless test result objects * Only re-run test methods that match names of previously failing test methods * Adopt tests to cover test method name matching Co-authored-by: Pablo Galindo Salgado <[email protected]>. (cherry picked from commit f1afef5) Co-authored-by: Łukasz Langa <[email protected]>

ambv added 7 commits July 21, 2021 17:22

Move to a static namespace

db6e311

Roughly annotate runtest.py

59d83c1

Refactor libregrtest to use lossless test result objects

89b209e

Only re-run test methods that match names of previously failing test …

c049888

…methods

Support multiprocessing test runners, too

d55e65b

Don't accumulate matches when multiple test files re-run

aad108f

Adopt tests to cover test method name matching

67c3e3d

ambv requested a review from pablogsal July 22, 2021 14:36

bedevere-bot added the awaiting core review label Jul 22, 2021

the-knights-who-say-ni added the CLA signed label Jul 22, 2021

ambv added 2 commits July 22, 2021 16:38

Add Blurb

26f3ba3

Change check order for Skipped and ResourceDenied (which subclasses S…

fb4346a

…kipped)

ambv commented Jul 22, 2021

View reviewed changes

Run make patchcheck to normalize whitespace

d9d7e0e

Fidget-Spinner reviewed Jul 22, 2021

View reviewed changes

pablogsal reviewed Jul 22, 2021

View reviewed changes

Lib/test/libregrtest/main.py Outdated Show resolved Hide resolved

pablogsal reviewed Jul 22, 2021

View reviewed changes

pablogsal approved these changes Jul 22, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 22, 2021

Explicitly name list comprehension variable for clarity

3e4622b

Co-authored-by: Pablo Galindo Salgado <[email protected]>

ambv merged commit f1afef5 into python:main Jul 22, 2021

bedevere-bot removed the awaiting merge label Jul 22, 2021

ambv added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Jul 22, 2021

miss-islington assigned ambv Jul 22, 2021

ambv added needs backport to 3.10 only security fixes and removed needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Jul 22, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 22, 2021

ambv deleted the regrtest-single-rerun branch May 3, 2022 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-44708: Only re-run test methods that match names of previously failing test methods #27287

bpo-44708: Only re-run test methods that match names of previously failing test methods #27287

ambv commented Jul 22, 2021 •

edited by bedevere-bot

Loading

ambv Jul 22, 2021

ambv Jul 22, 2021

pablogsal Jul 22, 2021

ambv Jul 22, 2021

ambv Jul 22, 2021

ambv Jul 22, 2021

pablogsal Jul 22, 2021

ambv Jul 22, 2021

ambv Jul 22, 2021

pablogsal Jul 22, 2021

ambv Jul 22, 2021 •

edited

Loading

Fidget-Spinner Jul 22, 2021 •

edited

Loading

pablogsal Jul 22, 2021

ambv Jul 22, 2021

pablogsal Jul 22, 2021

ambv Jul 22, 2021

pablogsal left a comment

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

bedevere-bot commented Jul 22, 2021

		self.good.append(test_name)
		elif ok in (FAILED, CHILD_ERROR):


		if rerun and ok not in {FAILED, CHILD_ERROR, INTERRUPTED}:
		if rerun and not isinstance(result, (Failed, Interrupted)):

		self.ns.match_tests.extend(error_names)
		self.ns.match_tests.extend(failure_names)

bpo-44708: Only re-run test methods that match names of previously failing test methods #27287

bpo-44708: Only re-run test methods that match names of previously failing test methods #27287

Conversation

ambv commented Jul 22, 2021 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambv Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Fidget-Spinner Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal left a comment

Choose a reason for hiding this comment

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

miss-islington commented Jul 22, 2021

bedevere-bot commented Jul 22, 2021

ambv commented Jul 22, 2021 •

edited by bedevere-bot

Loading

ambv Jul 22, 2021 •

edited

Loading

Fidget-Spinner Jul 22, 2021 •

edited

Loading