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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

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

Closed
danielmbrasil wants to merge 2 commits into
nodejs:mainfrom
danielmbrasil:fix/test-runner-ignore-patterns-isolation-none
Closed

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

Conversation

@danielmbrasil

Copy link
Copy Markdown
Contributor

Fixes#57399.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
@@ -691,6 +691,17 @@ function run(options = kEmptyObject) {
cwd,
globalSetupPath,
};

if (isolation === 'none') {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 πŸš€

Comment threadtest/parallel/test-runner-run.mjs Outdated
@@ -648,6 +648,48 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

describe("with isolation set to 'none'",() => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9af076f to 9aeef6cCompareMay 28, 2025 14:37
@danielmbrasil
danielmbrasilforce-pushed the fix/test-runner-ignore-patterns-isolation-none branch from 9aeef6c to db62068CompareJuly 4, 2025 16:08
@danielmbrasil
danielmbrasil marked this pull request as ready for review July 4, 2025 16:13
@codecov

codecovBot commented Jul 4, 2025

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (813b4e8) to head (db62068).
⚠️ Report is 3014 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 linesCoverage Ξ”
lib/internal/test_runner/runner.js93.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
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.test_runnerIssues 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

@danielmbrasil@nodejs-github-bot@pmarchini