[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tools: fix skip detection of test runner output #53545

Merged
merged 1 commit into from
Jun 24, 2024
Merged

tools: fix skip detection of test runner output #53545

merged 1 commit into from
Jun 24, 2024

Conversation

richardlau
Copy link
Member
@richardlau richardlau commented Jun 22, 2024 •
edited
Loading

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

skip_regex = re.compile(r'# SKIP\S*\s+(.*)', re.IGNORECASE)

node/tools/test.py

Lines 388 to 391 in d335487

skip = skip_regex.search(output.output.stdout)
if skip:
logger.info(
'ok %i %s # skip %s' % (self._done, command, skip.group(1)))

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

$ ./node --test --test-reporter=tap test/parallel/test-dotenv-node-options.js
TAP version 13
# Subtest: .env supports NODE_OPTIONS
    # Subtest: should have access to permission scope
    ok 1 - should have access to permission scope
      ---
      duration_ms: 26.0744
      ...
    # Subtest: validate only read permissions are enabled
    ok 2 - validate only read permissions are enabled
      ---
      duration_ms: 22.168635
      ...
    # Subtest: TZ environment variable
    ok 3 - TZ environment variable
      ---
      duration_ms: 19.970299
      ...
    1..3
ok 1 - .env supports NODE_OPTIONS
  ---
  duration_ms: 69.387679
  type: 'suite'
  ...
1..1
# tests 3
# suites 1
# pass 3
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 144.850498
$

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:

$ ./tools/test.py --progress=tap parallel/test-dotenv-node-options
TAP version 13
1..1
ok 1 parallel/test-dotenv-node-options # skip 0
  ---
  duration_ms: 213.30700
  ...

All tests passed.
$

With the updated regex:

$ ./tools/test.py --progress=tap parallel/test-dotenv-node-options
TAP version 13
1..1
ok 1 parallel/test-dotenv-node-options
  ---
  duration_ms: 212.83200
  ...

All tests passed.
$

cc @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 22, 2024
@richardlau richardlau mentioned this pull request Jun 22, 2024
Closed
@richardlau richardlau added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 22, 2024
@richardlau
tools: fix skip detection of test runner output
d8be819 
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.
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59924/

aduh95
aduh95 reviewed Jun 22, 2024
View reviewed changes
tools/test.py
@@ -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)
Copy link
Contributor

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?

Suggested change
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)

Copy link
Member Author
@richardlau richardlau Jun 22, 2024 •
edited
Loading

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.

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 24, 2024
@nodejs-github-bot
Copy link
Collaborator
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/.ncu
https://github.com/nodejs/node/actions/runs/9645012534

@richardlau richardlau added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2068c40 into nodejs:main Jun 24, 2024
60 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2068c40

@richardlau richardlau deleted the testharnessskip branch June 24, 2024 13:08
targos pushed a commit that referenced this pull request Jun 25, 2024
@richardlau @targos
tools: fix skip detection of test runner output
657c439 
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>
targos pushed a commit that referenced this pull request Jun 25, 2024
@richardlau @targos
tools: fix skip detection of test runner output
71321bb 
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>
@targos targos mentioned this pull request Jun 25, 2024
Merged
@Karliz24 Karliz24 mentioned this pull request Jul 9, 2024
Open
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@richardlau @marco-ippolito
tools: fix skip detection of test runner output
39e8748 
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>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@richardlau @marco-ippolito
tools: fix skip detection of test runner output
61a1c43 
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>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@atlowChemi atlowChemi atlowChemi approved these changes

@MoLow MoLow MoLow approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

parallel/test-dotenv-node-options skipped
7 participants
@richardlau @nodejs-github-bot @benjamingr @lpinca @MoLow @aduh95 @atlowChemi