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: fix flaky test-watch-file #34420

Closed
wants to merge 0 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 18, 2020

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.

@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

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.

@@ -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

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

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

Copy link
Member

@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
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
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
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
addaleax pushed a commit that referenced this pull request Sep 22, 2020
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
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
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. 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants