test_runner: run afterEach for runtime t.skip()

[igor-shevelenkov]

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]

Review requested:

• @nodejs/test_runner

[jsumners-nr]

Looks good to me.

[ljharb]

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]

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

[cjihrig]

You don't need the beforeEachTotal or afterEachTotal variables. The common.mustCall()s will enforce those for you.

[ljharb]

@igor-shevelenkov deviating from TAP semantics seems like a pretty big design issue, but fair that it'd be a breaking change. I'd still suggest making it (in a different PR ofc)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.77%. Comparing base (83893bb) to head (3a29ef0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61525      +/-   ##
==========================================
- Coverage   89.78%   89.77%   -0.01%     
==========================================
  Files         672      672              
  Lines      203809   203811       +2     
  Branches    39183    39189       +6     
==========================================
- Hits       182980   182963      -17     
- Misses      13166    13177      +11     
- Partials     7663     7671       +8     

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	97.31% <100.00%> (+<0.01%)	⬆️

... and 31 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.

[igor-shevelenkov]

@ljharb i'll research this topic, pros and cons and come back with possible solutions

[pmarchini]

@igor-shevelenkov deviating from TAP semantics seems like a pretty big design issue, but fair that it'd be a breaking change. I'd still suggest making it (in a different PR ofc)

I think that this might be a good discussion point for the next Node.js Test Runner Bi-Weekly Meeting 👀

cc @nodejs/test_runner

[cjihrig]

FWIW, I would strongly advise against letting TAP influence any decisions outside of the TAP reporter itself.

[ljharb]

@cjihrig true, it's not TAP specifically, it's the prior art of pre-existing TAP-generating test frameworks, most of which (possibly all) don't have a dynamic mechanism to skip a test from inside the test that's already begun.

[cjihrig]

I can't speak to whether or not anything else supports them, but they have been there for three and a half years (#43730) at this point. It doesn't seem like a good idea to break users who are depending on it at this point when this seems to be a straightforward fix.