test: fix flaky test-watch-file by Trott · Pull Request #34420 · nodejs/node · GitHub
Skip to content

test: fix flaky test-watch-file#34420

Closed
Trott wants to merge 0 commit into
nodejs:masterfrom
Trott:fix-test-watch-file
Closed

test: fix flaky test-watch-file#34420
Trott wants to merge 0 commit into
nodejs:masterfrom
Trott:fix-test-watch-file

Conversation

@Trott

@Trott Trott commented Jul 18, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 Trott force-pushed the fix-test-watch-file branch from 5feb148 to d748c81 Compare July 18, 2020 17:59
@Trott

Trott commented Jul 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

Trott commented Jul 19, 2020

Copy link
Copy Markdown
Member Author

@nodejs/testing

@Trott

This comment has been minimized.

Sign in to view
addaleax
addaleax approved these changes Jul 20, 2020
View reviewed changes
Comment thread test/pummel/test-watch-file.js Outdated

@addaleax addaleax Jul 20, 2020

Copy link
Copy Markdown
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?

@Trott Trott Jul 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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.

Comment thread test/pummel/test-watch-file.js Outdated
@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

Trott commented Jul 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@Trott

Trott commented Jul 21, 2020

Copy link
Copy Markdown
Member Author

@nodejs/fs

MylesBorins
MylesBorins reviewed Jul 21, 2020
View reviewed changes

@MylesBorins MylesBorins left a comment

Copy link
Copy Markdown
Contributor

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 Trott force-pushed the fix-test-watch-file branch from 35b6aa1 to b0b52b2 Compare July 22, 2020 00:30
@Trott

Trott commented Jul 22, 2020

Copy link
Copy Markdown
Member Author

@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
+1 more reviewer
@MylesBorins MylesBorins MylesBorins left review comments
Reviewers whose approvals may not affect merge requirements

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