Skip to content

lib: fix testNamePatterns and testSkipPatterns being ignored with isolation "none"#58496

Open
danielmbrasil wants to merge 2 commits intonodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Open

lib: fix testNamePatterns and testSkipPatterns being ignored with isolation "none"#58496
danielmbrasil wants to merge 2 commits intonodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none

Conversation

@danielmbrasil
Copy link
Contributor

Fixes #57399.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels May 28, 2025
globalSetupPath,
};

if (isolation === 'none') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it's a good approach. See my comment on the issue: #57399 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

I think it could be a good starting point, but I agree that at first glance, this solution feels "out of place".

I'll take a look as soon as I can and come back with better feedback 🚀

});
});

describe("with isolation set to 'none'",() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each of these tests passes individually, but when the whole describe block is run, the second test hangs indefinitely. I think it has something to do with how tests without isolation work, but I haven't found a fix yet.

Copy link
Member

Choose a reason for hiding this comment

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

I think it has something to do with how tests without isolation work

As you said, the problem is that the runner is running in the same process as the test file.
This is why you're encountering this issue.

You could follow the same logic used here: https://github.com/nodejs/node/blob/main/test/parallel/test-runner-run-watch.mjs

(Basically, by spawning a process that then calls run with isolation: none)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @pmarchini, sorry for the delay in getting back to this. I’ve followed your suggestion and got the tests working, thanks!

@danielmbrasil danielmbrasil force-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6c Compare May 28, 2025 14:37
@geeksilva97 geeksilva97 requested review from cjihrig and pmarchini May 28, 2025 14:40
@danielmbrasil danielmbrasil force-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068 Compare July 4, 2025 16:08
@danielmbrasil danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov
Copy link

codecov bot commented Jul 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58496      +/-   ##
==========================================
- Coverage   90.07%   90.07%   -0.01%     
==========================================
  Files         640      640              
  Lines      188442   188453      +11     
  Branches    36971    36975       +4     
==========================================
+ Hits       169735   169743       +8     
- Misses      11424    11426       +2     
- Partials     7283     7284       +1     
Files with missing lines Coverage Δ
lib/internal/test_runner/runner.js 93.25% <100.00%> (+0.54%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

const testSkipPatterns = options.testSkipPatterns ? JSON.stringify(options.testSkipPatterns) : undefined;
const isolation = options.isolation ? JSON.stringify(options.isolation) : undefined;

const code = `
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving this to a real fixture!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test runner run() ignores testNamePatterns and testSkipPatterns when isolation is "none"

3 participants