Skip to content

test_runner: run afterEach for runtime t.skip() - #61525

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
igor-shevelenkov:main
Feb 26, 2026
Merged

test_runner: run afterEach for runtime t.skip()#61525
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
igor-shevelenkov:main

Conversation

@igor-shevelenkov

Copy link
Copy Markdown
Contributor

Ensure afterEach runs when a test is skipped at runtime (t.skip()), while keeping static { skip: true } behavior unchanged.

Runtime t.skip() previously set skipped and prevented afterEach, even if beforeEach ran.
This left resources uncleaned and differed from user expectations.

Fixes: #61462

@nodejs-github-bot

Copy link
Copy Markdown
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 Jan 26, 2026

@jsumners-nr jsumners-nr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me.

@ljharb

ljharb commented Jan 26, 2026

Copy link
Copy Markdown
Member

This looks fine, but t.skip() shouldn't be marking the overarching test as skipped at all - its purpose is to generate a string comment that will show up as a skipped assertion.

@igor-shevelenkov

Copy link
Copy Markdown
Contributor Author

Thanks @ljharb. I see from tape-testing/tape#545 that in tape, t.skip() is designed as an assertion-level skip. Not a test-level skip. That aligns with TAP semantics.
However, Node's test runner has established t.skip() as a test-level runtime skip. It marks the whole test as skipped, not just an assertion. If we choose to change that i think it'll be a breaking change.
I think something like a separate t.skipAssertion() method would be better

Comment on lines +32 to +36
process.on('exit', () => {
assert.strictEqual(beforeEachTotal, 2);
assert.strictEqual(afterEachRuntimeSkip, 1);
assert.strictEqual(afterEachTotal, 2);
});

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.