FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: fix skip detection of test runner output by richardlau · Pull Request #53545 · nodejs/node · GitHub

/ node Public

tools: fix skip detection of test runner output - #53545

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richardlau:testharnessskip
Jun 24, 2024
ConversationCommits (1)ChecksFiles changed
Merged

tools: fix skip detection of test runner output#53545
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richardlau:testharnessskip

Conversation

richardlau commented Jun 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 22, 2024
richardlau mentioned this pull request Jun 22, 2024
Closed
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
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.

This comment was marked as outdated.

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

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Jun 22, 2024

Copy link
Copy Markdown
Collaborator

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

aduh95 reviewed Jun 22, 2024
View reviewed changes
Comment thread tools/test.py

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)

aduh95 Jun 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

richardlau Jun 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2024
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 commented Jun 24, 2024

Copy link
Copy Markdown
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 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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2024
nodejs-github-bot merged commit 2068c40 into nodejs:main Jun 24, 2024

nodejs-github-bot commented Jun 24, 2024

Copy link
Copy Markdown
Collaborator

Landed in 2068c40

richardlau deleted the testharnessskip branch June 24, 2024 13:08
targos pushed a commit that referenced this pull request Jun 25, 2024
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 mentioned this pull request Jun 25, 2024
Merged
Karliz24 mentioned this pull request Jul 9, 2024
Open
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
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 mentioned this pull request Jul 19, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 left review comments
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
MoLow MoLow approved these changes
atlowChemi atlowChemi 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

Back | FazBrowse Home | New Git URL