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 timeout in sequential/test-fs-watch-system-limit #23560

Closed
wants to merge 180 commits into from
Closed

test: fix timeout in sequential/test-fs-watch-system-limit #23560

wants to merge 180 commits into from

Conversation

mmmscott
Copy link
Contributor

During run of make test on Linux, it is possible for this test
to spawn 200 processes and hang since there is a check to max
out at 200 child processes.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@mmmscott
test: fix timeout when running sequential/test-fs-watch-system-limit.js
9aa9e80 
During run of make test on Linux, it is possible for this test
to spawn 200 processes and hang since there is a check to max
out at 200 child processes.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 12, 2018
@targos targos added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@BridgeAR BridgeAR changed the title test: fix timeout when running sequential/test-fs-watch-system-limit.js test: fix timeout in sequential/test-fs-watch-system-limit Oct 12, 2018
addaleax and others added 24 commits October 12, 2018 21:07
@addaleax
deps: cherry-pick b0af309 from upstream V8
3ab4146 
Original commit message:

    [api] Remove deprecated wasm methods

    These methods were deprecated in 7.0, now we can remove them.

    R=adamk@chromium.org

    Bug: v8:7868
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I60badb378a055152bdd27aed67d11ddf74fce174
    Reviewed-on: https://chromium-review.googlesource.com/1209283
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#55695}

Refs: v8/v8@b0af309

PR-URL: nodejs#23415
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@rxiayth @addaleax
lib: migrate to getOptions in loaders.js
849aa1e 
PR-URL: nodejs#23455
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@leomoreno @Trott
test: improve test coverage for fs module
c24e963 
Covering the case when fs-read get invalid argument for file handle

PR-URL: nodejs#23601
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@ofrobots
src: reduce platform worker barrier lifetime
9f7e3a4 
Minor cleanup in the lifetime for the platform worker initialization
synchronization barrier.

PR-URL: nodejs#23419
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jungkumseok @Trott
test: fix assert.strictEqual arguments in test/parallel/test-c-ares.js
64d65ed 
When using `assert.strictEqual`, the first argument must be the actual
value and the second argument must be the expected value.

PR-URL: nodejs#23448
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@khalidadil @Trott
repl: remove unused variable e from try catch
f0c5c96 
PR-URL: nodejs#23449
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jehaines @Trott
test: fix order of values in test assertions
478fe0a 
Have actual first, expected second.

PR-URL: nodejs#23450
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@widatama @Trott
test: fix http local address test assertion
dee9d27 
Reverse the argument for assertion. The first argument should be the
actual value and the second value should be the expected value. When
there is an AssertionError, the expected and actual value will be
labeled correctly.

PR-URL: nodejs#23451
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@mmisiarek @Trott
repl: remove unused variable from try catch
c301518 
Catch statement defines err variable that is never used, so it
is safe to remove that.

PR-URL: nodejs#23452
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@aeisenberg @Trott
test: replace assert.throws w/ common.expectsError
0005846 
Converts RangeError assertions to use common.expectsError and includes
an assertion for the error code.

PR-URL: nodejs#23454
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@ivansieder @Trott
test: strictEqual correct order for http-information-processing test
84b21eb 
PR-URL: nodejs#23456
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@andy-ganchrow @Trott
test: fix assert.strictEqual argument order
a239270 
PR-URL: nodejs#23457
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@pdxmholmes @Trott
lib: remove unused 'e' from catch
68b4637 
PR-URL: nodejs#23458
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@epeden @Trott
test: swap assert.strictEqual args to actual, expected
767d4da 
PR-URL: nodejs#23459
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@mdaum @Trott
test: fix incorrect ordering of args in assert.strictEqual()
657ee7b 
PR-URL: nodejs#23461
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@rmarkins-godaddy @Trott
test: correct assert test
1c73990 
PR-URL: nodejs#23463
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@DennyScott @Trott
test: remove unused e variable in catch statement
8691d8f 
PR-URL: nodejs#23465
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@Trott
test: fix incorrect expectation order
c23d250 
test-http-expect-handling.js has an instance of the test value and the
expected value being reversed.

Refs: https://nodejs.org/api/assert.html

PR-URL: nodejs#23466
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@nolanmar511 @Trott
test: correct order for assert.strictEqual for inspector-helper test
ffa2710 
PR-URL: nodejs#23468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@Trott
test: update test-cluster-worker-events to use arrow functions
3989785 
PR-URL: nodejs#23469
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack @addaleax
test: mark test-http2-session-timeout as flake on ARM
882e022 
PR-URL: nodejs#23639
Refs: nodejs#20628
Reviewed-By: Rich Trott <rtrott@gmail.com>
@mmmscott
test: don't write to stream if test passed successfully
97d60f9 
On success case, timeout still tries to fire and write to
stream. Only write to stream if we haven't finished
@mmmscott
test: fix timeout when running sequential/test-fs-watch-system-limit.js
dbfb5d4 
During run of make test on Linux, it is possible for this test
to spawn 200 processes and hang since there is a check to max
out at 200 child processes.
@mmmscott
test: don't write to stream if test passed successfully
1ffcd0c 
On success case, timeout still tries to fire and write to
stream. Only write to stream if we haven't finished
binaryme and others added 15 commits October 15, 2018 15:34
@binaryme @BridgeAR
test: fix parameters in test-repl.js
a039c86 
fixed order of parameters in assert.strictEqual() assertion functions,
first argument provided was the expected value and the second value
was the actual value.

this is backwards from the documentation for assertions like
assert.strictEqual() where the first value being tested and the second
value is the expected value

PR-URL: nodejs#23609
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@milsosa @BridgeAR
test: fix assertions args order
31cb297 
PR-URL: nodejs#23608
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@ptichonczuk-tc @BridgeAR
test: re-order strictEqual paramater calls
6151040 
PR-URL: nodejs#23607
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@sheminusminus @BridgeAR
test: fix order of assert args in client response domain test
febb765 
PR-URL: nodejs#23604
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BridgeAR
src: change macro to fn
17c35a6 
Change base64_encoded_size and unbase64 to inline functions. The
base64_encoded_size is a constexpr to be used in function declarations.

PR-URL: nodejs#23603
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BridgeAR
test: fix assert equal order of arguments
f781b2c 
PR-URL: nodejs#23602
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@chucktheobald @BridgeAR
test: change order of assert.strictEquals arguments
f8b1e0e 
Fix assert.strictEquals argument order.

Arguments were actual first, expected second, contrary to
the documentation. Now, actual value is first and expected
value is second.

PR-URL: nodejs#23600
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@iansu @BridgeAR
test: fix assert parameter order in test-https-localaddress.js
697c3da 
PR-URL: nodejs#23599
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BridgeAR
test: fix assert.strictEqual() argument order
1237c14 
PR-URL: nodejs#23598
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@Agheb @BridgeAR
test: assertions arguments match docs
a979443 
PR-URL: nodejs#23594
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jehaines @BridgeAR
test: increased code coverage for slowCases
b2e4cb7 
Added test coverage for 4 un-covered if statements in slowCases

PR-URL: nodejs#23592
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@sam-github
test: use smaller keys for a faster keygen test
561e30d 
On my machine, this brings test execution time down from about 2
seconds to 0.2 seconds.

PR-URL: nodejs#23430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@AndreasMadsen
async_hooks: remove promise object from resource
1a2cf66 
While it doesn't make any difference now. In the future PromiseHooks
could be refactored to provide an asyncId instead of the promise object.
That would make escape analysis on promises possible.

Escape analysis on promises could lead to a more efficient destroy hook,
if provide by PromiseHooks as well. But at the very least would allow
the destroy hook to be emitted earlier. The destroy hook not being
emitted on promises frequent enough is a known and reported issue.
See nodejs#14446 and
Jeff-Lewis/cls-hooked#11.

While all this is speculation for now, it all depends on the promise
object not being a part of the PromiseWrap resource object.

Ref: nodejs#14446
Ref: nodejs/diagnostics#188

PR-URL: nodejs#23443
Refs: nodejs#14446
Refs: nodejs/diagnostics#188
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
@christian-bromann @refack
test: add test coverage for fs.truncate
74f854e 
Add test to check:
- for `null` as len parameter
- if error is propagated into callback if file doesn't exist
- if an error is actually thrown if len is not a number

PR-URL: nodejs#23620
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@addaleax @danbev
zlib: refactor zlib internals
c34eae5 
Split out things that are specific to zlib as a specific
compression library, vs. the interface that is common to
most C compression libraries.

This should pave the way for including support for e.g.
brotli.

PR-URL: nodejs#23360
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
addaleax
addaleax reviewed Oct 16, 2018
View reviewed changes
test/sequential/test-fs-watch-system-limit.js Outdated
const timeoutStream = new stream.PassThrough();
timeoutStream.write('Error: timeout');
timeoutStream.end();
timeoutStream.pipe(gatherStderr);
Copy link
Member

Choose a reason for hiding this comment

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

Why not call gatherStderr.write() directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call!

test/sequential/test-fs-watch-system-limit.js Outdated
timeoutStream.pipe(gatherStderr);

finished = true;
processes.forEach((proc) => proc.kill());
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? Shouldn’t it already be happening below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was a bit aggressive with the process killing since the test had left my machine with a lot of child processes, but you're right

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be good to add to process.once('exit') in case the test fails in the middle.

test/sequential/test-fs-watch-system-limit.js
let accumulated = '';
gatherStderr.on('data', common.mustCallAtLeast((chunk) => {
accumulated += chunk;
if (accumulated.includes('Error:') && !finished) {
assert(
accumulated.includes('ENOSPC: System limit for number ' +
'of file watchers reached') ||
accumulated.includes('EMFILE: '),
accumulated.includes('EMFILE: ') ||
accumulated.includes('Timeout'),
Copy link
Member

Choose a reason for hiding this comment

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

This spells Timeout with a capital T, the text above has a lowercase t, so they don’t match … is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unintentional, thank you

dtoki and others added 10 commits October 16, 2018 13:46
@dtoki @addaleax
src: add deprecation warning to errname()
91fe7e5 
PR-URL: nodejs#23597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jonathansamines @addaleax
test: fix strictEqual arguments order
b4a5ff9 
Fix strictEqual assertion arguments order to conform
to the function signature in buffer tests

PR-URL: nodejs#23486
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@mmmscott
test: simplify test case by just writing directly to stream object
a17077f
@mmmscott
test: fix timeout when running sequential/test-fs-watch-system-limit.js
9a32087 
During run of make test on Linux, it is possible for this test
to spawn 200 processes and hang since there is a check to max
out at 200 child processes.
@mmmscott
test: don't write to stream if test passed successfully
860e82f 
On success case, timeout still tries to fire and write to
stream. Only write to stream if we haven't finished
@mmmscott
test: fix timeout when running sequential/test-fs-watch-system-limit.js
77f3ae8 
During run of make test on Linux, it is possible for this test
to spawn 200 processes and hang since there is a check to max
out at 200 child processes.
@mmmscott
test: don't write to stream if test passed successfully
8f9b07d 
On success case, timeout still tries to fire and write to
stream. Only write to stream if we haven't finished
@mmmscott
test: don't write to stream if test passed successfully
8024ad9 
On success case, timeout still tries to fire and write to
stream. Only write to stream if we haven't finished
@mmmscott
test: simplify test case by just writing directly to stream object
b4468d9
@mmmscott
Merge branch 'fix-fs-watch-limit-timeout' of https://github.com/marcu…
7e51c77 
…sscott/node into fix-fs-watch-limit-timeout
@mmmscott
Copy link
Contributor Author

sorry, i messed up this rebase completely. i'm going to close this PR and redo it.

@mmmscott mmmscott closed this Oct 16, 2018
@mmmscott mmmscott deleted the fix-fs-watch-limit-timeout branch October 16, 2018 14:10
@refack
Copy link
Contributor

refack commented Oct 16, 2018

@marcusscott in case you still have trouble rebasing, you could reopen this PR, and I could help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@refack refack refack left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.