test: skip fs.watch on Travis

[refack]

Fixes: #21310
Refs: https://docs.travis-ci.com/user/environment-variables/#default-environment-variables

• Refs: https://travis-ci.com/nodejs/node/jobs/142234998

  • test/async-hooks/test-fseventwrap.js
  • test/parallel/test-fs-watch-close-when-destroyed.js

• Ref: https://travis-ci.com/nodejs/node/jobs/142716959

  • test/sequential/test-fs-watch.js

Refs: #24198 (comment)

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]

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/726/pipeline

[refack]

/CC @nodejs/testing

[BridgeAR]

Ping @refack

[mscdex]

I still wonder if we can't instead fix the root of the problem in some way, perhaps as I suggested here or some other solution?

[refack]

I still wonder if we can't instead fix the root of the problem in some way, perhaps as I suggested here or some other solution?

Since it tests in the faster boot container, we run with:

node/.travis.yml

Line 4 in 04195ad

sudo: false

IMHO switching to the full VM is not worth it, it will be slower (and it's not nice to Travis.com who are giving us this service for free). Also since we only use the Travis report as a quick way to get feedback, and not as a replacement to our testing cluster, I don't feel bad about trimming down the test suite that runs on Travis.

On the other hand, I'm not 100% about adding cruft to our tests. A better approach is probably to enable ignoring tests via https://github.com/nodejs/node/blob/master/test/parallel/parallel.status

[richardlau]

A better approach is probably to enable ignoring tests via https://github.com/nodejs/node/blob/master/test/parallel/parallel.status

I'd much prefer this than the current approach in this PR.

[refack]

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

@nodejs/testing @nodejs/fs PTAL

[thefourtheye]

The tests being skipped here need to be tested on all our supported platforms. We even have captured the caveats in our docs, here.

I understand these tests make our CI flaky, but I would prefer not to skip them if that is an option.

[richardlau]

I still wonder if we can't instead fix the root of the problem in some way, perhaps as I suggested here or some other solution?

Since it tests in the faster boot container, we run with:

node/.travis.yml

Line 4 in 04195ad
sudo: false

IMHO switching to the full VM is not worth it, it will be slower (and it's not nice to Travis.com who are giving us this service for free). Also since we only use the Travis report as a quick way to get feedback, and not as a replacement to our testing cluster, I don't feel bad about trimming down the test suite that runs on Travis.

FYI The container-based environment is being phased out: https://blog.travis-ci.com/2018-10-04-combining-linux-infrastructures

[refack]

I understand these tests make our CI flaky, but I would prefer not to skip them if that is an option.

Just to be clear, it's not skipped on our CI, only Travis (which we don't consider an official CI)

[refack]

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18424/
Resume again: https://ci.nodejs.org/job/node-test-pull-request/18425/

[thefourtheye]

@refack Thanks for clarifying. I overlooked that initially.

[refack]

But, Rich, someone seeing process.env.TRAVIS will know that it's checking for Travis but not why. common.hasLimitedFSWatchers() explains right in the name the reason that we are skipping the test. (And if you don't think so, suggest a better name, Rich!)

I inlined the test logic (if (process.env.TRAVIS)) and used the skip message to explain why ('Skip fs.watch-intensive test because of Travis limitation.')

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

[Trott]

Might be good to hold off on landing this until after #24511 to see if that fixes the problem. I'm going to put a blocked label here, but feel free to remove it if you feel that's not the right way to go.

[refack]

Let's close this as superseded by #24511 (since even if that doesn't directly solve the issue we should just increase the number of watchers allowed)