node
node copied to clipboard
nodejs
test_runner: fix delete test file cause dependency file change not rerun the tests
There is an edge case left from https://github.com/nodejs/node/pull/53114
Reproduce step:
// create dependency index.js
export const a = 1;
export const b = 2;
export const c = 3;
// create test-a.mjs
import { a } from "./index.mjs";
import { test } from "node:test";
import assert from "node:assert";
test("test One", () => {
assert.equal(a, 1);
});
// create test-b.mjs
import { a, b } from "./index.mjs";
import { test } from "node:test";
import assert from "node:assert";
test("test Two", () => {
assert.equal(a, 1);
assert.equal(b, 2);
});
// create test-c.mjs
import { a, b, c } from "./index.mjs";
import { test } from "node:test";
import assert from "node:assert";
test("test Three", () => {
assert.equal(a, 1);
assert.equal(b, 2);
assert.equal(c, 3);
});
node --watch --test
# now delete test-a.mjs
# then make any change to index.mjs e.g. change const a = 1 to const a = 2
You would observe that test-b.mjs and test-c.mjs are not being rerun. The expected behaviour should be test-b.mjs, test-c.mjs being rerun and failed (since assert.strictEqual(a, 1) is not correct anymore).
Change explain:
When a watched test file is being deleted, the test runner will rerun the test(s) and because the test file is no longer there but the dependencyOwners wouldn't be able to know it, so it failed in silence. Since a test file is being deleted, we don't need to rerun anything so we can just safely return.
notes:
The test is quite identical to test/parallel/test-runner-watch-mode.mjs but with a more complicated setup, I am thinking to spend some time to see how I can refactor the test so we can write more complicated test cases in the future, I am committed and would like to do a separate PR for it.
Ref: https://github.com/nodejs/node/pull/53114
Haven't got back from @benjamingr yet on https://github.com/nodejs/node/pull/53533#discussion_r1667749702, shall we land this bug fix first and I can try to create a follow up PR to improve it.
hall we land this bug fix first and I can try to create a follow up PR to improve it.
SGTM
I figured out the watch capability is limited in AIX which caused the test in CI on that platform to fail.
Codecov Report
Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
Project coverage is 87.10%. Comparing base (
e0634f5) to head (b1e06fc). Report is 459 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| lib/internal/test_runner/runner.js | 0.00% | 6 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #53533 +/- ##
==========================================
- Coverage 87.10% 87.10% -0.01%
==========================================
Files 647 647
Lines 181739 181759 +20
Branches 34887 34889 +2
==========================================
+ Hits 158310 158323 +13
- Misses 16738 16740 +2
- Partials 6691 6696 +5
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/test_runner/runner.js | 84.86% <0.00%> (-0.69%) |
:arrow_down: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Green CI now 🥳 would appreciate someone can take a look again and give a green tick so this PR is able to land (my latest change was skipping the test on AIX platform as the watch capability is restricted)
@mcollina do you mind taking a quick look 🙏 as you recently did some work in this area as well