-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: fix skip detection of test runner output #53545
Conversation
Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped.
This comment was marked as outdated.
This comment was marked as outdated.
c3777c0
to
d8be819
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@@ -83,7 +83,7 @@ def get_module(name, path): | |||
|
|||
|
|||
logger = logging.getLogger('testrunner') | |||
skip_regex = re.compile(r'# SKIP\S*\s+(.*)', re.IGNORECASE) | |||
skip_regex = re.compile(r'(?:\d+\.\.\d+|ok|not ok).*# SKIP\S*\s+(.*)', re.IGNORECASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we want to not match line returns?
skip_regex = re.compile(r'(?:\d+\.\.\d+|ok|not ok).*# SKIP\S*\s+(.*)', re.IGNORECASE) | |
skip_regex = re.compile(r'(?:\d+\.\.\d+|ok|not ok)[^\n]*# SKIP\S*\s+(.*)', re.IGNORECASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .
character already excludes newlines.
https://docs.python.org/3/library/re.html
(Dot.) In the default mode, this matches any character except a newline. If the DOTALL flag has been specified, this matches any character including a newline.
Commit Queue failed- Loading data for nodejs/node/pull/53545 ✔ Done loading data for nodejs/node/pull/53545 ----------------------------------- PR info ------------------------------------ Title tools: fix skip detection of test runner output (#53545) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch richardlau:testharnessskip -> nodejs:main Labels test, tools Commits 1 - tools: fix skip detection of test runner output Committers 1 - Richard Lau PR-URL: https://github.com/nodejs/node/pull/53545 Fixes: https://github.com/nodejs/node/issues/50346 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Luigi Pinca Reviewed-By: Chemi Atlow Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/53545 Fixes: https://github.com/nodejs/node/issues/50346 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Luigi Pinca Reviewed-By: Chemi Atlow Reviewed-By: Moshe Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 22 Jun 2024 03:56:35 GMT ✔ Approvals: 4 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133822428 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133854918 ✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133920028 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133923840 ✘ Last GitHub CI failed ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/9645012534 |
Landed in 2068c40 |
Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped. PR-URL: #53545 Fixes: #50346 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped. PR-URL: #53545 Fixes: #50346 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped. PR-URL: #53545 Fixes: #50346 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped. PR-URL: #53545 Fixes: #50346 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Fix the Python test harness so that it no longer treats the
# skipped
part of the summary at the end of the built-in test runner output as marking the test as skipped.Fixes: #50346
So it turns out that the Python test runner scans the output of the tests to look for SKIP directives:
node/tools/test.py
Line 86 in d335487
node/tools/test.py
Lines 388 to 391 in d335487
(this regex is based on http://testanything.org/tap-specification.html#skipping-tests).
For the tests that are using the built-in test runner, this is matching the
# skipped
line in the summary at the end of the output,e.g.
which causes the Python test harness to insert a SKIP directive with the number of skipped tests (e.g. 0 in this case) as the reason for skipping:
With the updated regex:
cc @nodejs/test_runner