Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: fix delete test file cause dependency file change not rerun the tests #53533

Conversation

jakecastelli
Copy link
Member

@jakecastelli jakecastelli commented Jun 21, 2024

There is an edge case left from #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: #53114

@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 Jun 21, 2024
@jakecastelli jakecastelli changed the title test_runner: fix delete test file cause dependency file not watched test_runner: fix delete test file cause dependency file change not rerun the tests Jun 23, 2024
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
test/parallel/test-runner-watch-mode-complex.mjs Outdated Show resolved Hide resolved
@@ -0,0 +1,100 @@
// Flags: --expose-internals
Copy link
Member

Choose a reason for hiding this comment

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

Is this used only for util.createDeferredPromise?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it only used for util.createDeferredPromise

test/parallel/test-runner-watch-mode-complex.mjs Outdated Show resolved Hide resolved
test/parallel/test-runner-watch-mode-complex.mjs Outdated Show resolved Hide resolved
test/parallel/test-runner-watch-mode-complex.mjs Outdated Show resolved Hide resolved
@jakecastelli jakecastelli force-pushed the fix-test-runner-watch-dependency-update branch from d79b8d7 to e270e58 Compare June 28, 2024 12:42
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2024
@atlowChemi atlowChemi removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2024
@jakecastelli
Copy link
Member Author

maybe we can get CI started again? 👀

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024
@nodejs-github-bot
Copy link
Collaborator

let currentRun = '';
const runs = [];

child.stdout.on('data', (data) => {
Copy link
Member

Choose a reason for hiding this comment

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

This could just be .toArray()on child.stdout which would also save the util.createDeferredPromise and would make it easier to listen to stdout

Copy link
Member

Choose a reason for hiding this comment

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

(just make sure to not await it before you trigger the watch)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can I get a little bit of help here? I tried to use child.stdout.toArray() - but I cannot seem to be able to control the test runs, I cannot manage to get 2 runs (get result for initial run 3/3 pass, delete a test file modify another test to get the result of second run 2/2 pass) successfully, I can only get result for 1 run

I must be doing something incorrectly.

Would you mind explaining not await to what?

(just make sure to not await it before you trigger the watch)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I may have understood what you mean - use child.stdout.toArray() to get a promise then delete a test file and update a file in order to trigger the rerun (watch), after that resolve the promise to get all the std out results, am I understanding correctly? 👀

@jakecastelli
Copy link
Member Author

Haven't got back from @benjamingr yet on #53533 (comment), shall we land this bug fix first and I can try to create a follow up PR to improve it.

@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024
@nodejs-github-bot

This comment was marked as outdated.

@benjamingr
Copy link
Member

hall we land this bug fix first and I can try to create a follow up PR to improve it.

SGTM

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

jakecastelli and others added 4 commits August 7, 2024 21:38
When a watched test file is being deleted then the referenced dependency
file(s) will be updated incorrect when `unfilterFilesOwnedBy` method is
called, which will cause tests not being rerun when its referenced
dependency changed. To prevent this case, we can simply `return` when we
detect a watched test file being deleted.
Co-authored-by: Chemi Atlow <chemi@atlow.co.il>
@nodejs-github-bot
Copy link
Collaborator

@jakecastelli
Copy link
Member Author

I figured out the watch capability is limited in AIX which caused the test in CI on that platform to fail.

@jakecastelli jakecastelli force-pushed the fix-test-runner-watch-dependency-update branch from a82efaa to b1e06fc Compare August 8, 2024 04:11
Copy link

codecov bot commented Aug 8, 2024

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 ⚠️
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%) ⬇️

... and 27 files with indirect coverage changes

@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jakecastelli
Copy link
Member Author

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)

@jakecastelli jakecastelli added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 8, 2024
@jakecastelli
Copy link
Member Author

@mcollina do you mind taking a quick look 🙏 as you recently did some work in this area as well

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 14, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 14, 2024
@nodejs-github-bot nodejs-github-bot merged commit 1212eca into nodejs:main Aug 14, 2024
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1212eca

RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
When a watched test file is being deleted then the referenced dependency
file(s) will be updated incorrect when `unfilterFilesOwnedBy` method is
called, which will cause tests not being rerun when its referenced
dependency changed. To prevent this case, we can simply `return` when we
detect a watched test file being deleted.

PR-URL: #53533
Refs: #53114
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. 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.

7 participants