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: make test-fs-watchfile reliable #13385

Closed
wants to merge 1 commit into from
Closed

test: make test-fs-watchfile reliable #13385

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 2, 2017 •
edited
Loading

Fixes: #13377

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. wip Issues and PRs that are still a work in progress. test Issues and PRs related to the tests. labels Jun 2, 2017
@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

Relevant stress tests:

AIX: https://ci.nodejs.org/job/node-stress-single-test/1263/nodes=aix61-ppc64/console
macOS: https://ci.nodejs.org/job/node-stress-single-test/nodes=osx1010/1265/console

@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/8428/

mhdawson
mhdawson approved these changes Jun 2, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

Stress tests indicate this doesn't fix the flakiness on AIX. I'm starting to wonder if sometimes AIX hands us watchers that will simply never fire. The fs.watch() stuff is highly specific to the OS.

@gireeshpunathil
Copy link
Member

Background

Watch facility on folders are not fool-proof in AIX, and we have skipped those tests which do that. I guess the new changes in
test-fs-watchfile.js is not catering to that.

#13111 made that change, it's original intent was to make sure the filename argument appears in the callback, when it fires - so I requested to include AIX as well, as the filename argument is availabe in that platform. But then the change introduced folder watch and hence the flaky result.

Proposals - one of:

  1. Amend the new change in test-fs-watchfile.js to skip AIX.
  2. Amend the new change in test-fs-watchfile.js to file watch as opposed to folder watch.

@gireeshpunathil gireeshpunathil mentioned this pull request Jun 2, 2017
Closed
@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

Thanks, @gireeshpunathil! I've moved the test back to parallel and updated it to skip the directory watch test on AIX.

CI: https://ci.nodejs.org/job/node-test-pull-request/8440/

@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

(Whoops, will need to update the commit message when landing.)

@Trott
test: make test-fs-watchfile reliable
85097f5 
Omitting AIX from `fs.watch()` portion of this test. It works
on AIX, but not reliably.

Fixes: nodejs#13377
@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

@refack I've change the error throwing stuff back to the way it was. I don't feel strongly about it and I have mixed feelings about doing small-but-unrelated refactors with more meaningful changes anyway. So happy to put it back the way you like. It also gave me an opportunity to fix the commit message!

CI again: https://ci.nodejs.org/job/node-test-pull-request/8442/

@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

Test failure on OS X is unrelated (and is something that will be fixed coincidentally by a NodeTodo task I just gave to someone yesterday).

Thoughts on merging this now rather than waiting the 72 hours on the grounds that it's trivial and fixes a CI issue?

@refack refack mentioned this pull request Jun 2, 2017
Merged
3 tasks
@refack
Copy link
Contributor

refack commented Jun 2, 2017

Thoughts on merging this now rather than waiting the 72 hours on the grounds that it's trivial and fixes a CI issue?

👍

@Trott
Copy link
Member Author

Trott commented Jun 2, 2017

Landed in 98aa25c

@Trott Trott closed this Jun 2, 2017
Trott added a commit that referenced this pull request Jun 2, 2017
@Trott
test: make test-fs-watchfile reliable
98aa25c 
Omitting AIX from `fs.watch()` portion of this test. It works
on AIX, but not reliably.

PR-URL: #13385
Fixes: #13377
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
jasnell pushed a commit that referenced this pull request Jun 5, 2017
@Trott @jasnell
test: make test-fs-watchfile reliable
406c2cd 
Omitting AIX from `fs.watch()` portion of this test. It works
on AIX, but not reliably.

PR-URL: #13385
Fixes: #13377
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell mentioned this pull request Jun 5, 2017
Closed
refack added a commit to refack/node that referenced this pull request Jun 7, 2017
@refack
test,fs: test fs.watch for filename
c05561e 
PR-URL: nodejs#13411
Refs: nodejs#13385
Refs: nodejs#13248
Refs: nodejs#13377
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Jun 7, 2017
@refack @jasnell
test,fs: test fs.watch for filename
658560e 
PR-URL: #13411
Refs: #13385
Refs: #13248
Refs: #13377
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins MylesBorins added dont-land-on-v6.x and removed wip Issues and PRs that are still a work in progress. labels Jul 17, 2017
@Trott Trott deleted the yaix branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Intermittent failure in parallel/test-fs-watchfile on AIX
8 participants
@Trott @gireeshpunathil @refack @jasnell @cjihrig @hiroppy @mhdawson @MylesBorins