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.

Sign up for GitHub

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

Jump to bottom

test: fix flaky test-watch-file #34420

Closed
wants to merge 0 commits into from
Closed

test: fix flaky test-watch-file #34420

wants to merge 0 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 18, 2020 •
edited
Loading

Example of the failure this will avoid is https://ci.nodejs.org/job/node-test-commit-linux/36156/nodes=alpine-latest-x64/testReport/junit/(root)/test/pummel_test_watch_file/:

assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected "actual" not to be strictly deep-equal to:

2020-07-17T16:00:38.099Z
    at StatWatcher.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/test/pummel/test-watch-file.js:38:12)
    at StatWatcher.emit (events.js:314:20)
    at StatWatcher.onchange (internal/fs/watchers.js:63:8) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 2020-07-17T16:00:38.099Z,
  expected: 2020-07-17T16:00:38.099Z,
  operator: 'notDeepStrictEqual'
}
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 18, 2020
@Trott

This comment has been minimized.

Sign in to view
@Trott Trott force-pushed the fix-test-watch-file branch 4 times, most recently from 95b53f4 to 5feb148 Compare July 18, 2020 17:54
@Trott Trott changed the title test: improve flaky test-watch-file test: fix flaky test-watch-file Jul 18, 2020
@Trott Trott added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Jul 18, 2020
@Trott
Copy link
Member Author

Trott commented Jul 19, 2020 •
edited
Loading

Stress test that shows the failures on master: https://ci.nodejs.org/job/node-stress-single-test/122/

Stress test with this PR showing the test passing: https://ci.nodejs.org/job/node-stress-single-test/137 ✅

@Trott Trott marked this pull request as ready for review July 19, 2020 05:39
@Trott
Copy link
Member Author

Trott commented Jul 19, 2020

@nodejs/testing

@Trott

This comment has been minimized.

Sign in to view
addaleax
addaleax approved these changes Jul 20, 2020
View reviewed changes
test/pummel/test-watch-file.js Outdated
@@ -34,8 +34,11 @@ fs.closeSync(fs.openSync(f, 'w'));
let changes = 0;
function watchFile() {
fs.watchFile(f, (curr, prev) => {
// Make sure there is at least one watch event that shows a changed mtime.
if (curr.mtime <= prev.mtime) {
Copy link
Member

Choose a reason for hiding this comment

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

Just for my understanding, this would never really be a strictly-less-than relation, and only ever larger or equal, but if we check for equality we might as well also check for less-than?

Copy link
Member Author

@Trott Trott Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For the most part, yes. It's a coherence check. Super unlikely edge case: It is possible for the time on a host to be adjusted backwards (to correct for it having been set incorrectly in the first place, for example) but that would be highly unusual of course.

test/pummel/test-watch-file.js Outdated Show resolved Hide resolved
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2020
@Trott
Copy link
Member Author

Trott commented Jul 20, 2020 •
edited
Loading

Another stress CI with another fixup commit: https://ci.nodejs.org/job/node-stress-single-test/139/ ✅

@Trott
Copy link
Member Author

Trott commented Jul 21, 2020

@nodejs/fs

MylesBorins
MylesBorins reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

RSLGTM

Trott added a commit that referenced this pull request Jul 22, 2020
@Trott
test: fix flaky test-watch-file
b0b52b2 
PR-URL: #34420
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@Trott Trott closed this Jul 22, 2020
@Trott
Copy link
Member Author

Trott commented Jul 22, 2020

Landed in b0b52b2

@Trott Trott deleted the fix-test-watch-file branch July 22, 2020 00:31
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@Trott @cjihrig
test: fix flaky test-watch-file
50229d4 
PR-URL: #34420
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 27, 2020
@Trott @MylesBorins
test: fix flaky test-watch-file
eecb92c 
PR-URL: #34420
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Jul 28, 2020
Merged
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott @addaleax
test: fix flaky test-watch-file
0fb7d59 
PR-URL: #34420
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott @addaleax
test: fix flaky test-watch-file
8cb306e 
PR-URL: #34420
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
@mhdawson mhdawson mentioned this pull request Jan 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@MylesBorins MylesBorins MylesBorins left review comments

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Trott @MylesBorins @addaleax @nodejs-github-bot