test: remove error message checking in test-worker-init-failure by Trott · Pull Request #34727 · nodejs/node

Trott · 2020-08-11T14:20:33Z

Let the check for the error code suffice and don't check for a
particular string in the message.

Fixes: #33759

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2020-08-11T14:30:21Z

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

gireeshpunathil · 2020-08-11T14:36:48Z

test/parallel/test-worker-init-failure.js


    worker.on('error', (e) => {
-      assert.match(e.message, /EMFILE/);
      assert.ok(e.code === 'ERR_WORKER_INIT_FAILED' || e.code === 'EMFILE');


I am not sure if this will work: as in the error site in the related failure incident, e.code is going to be ENOENT?

and worker did not fail in it's INIT, instead at runtime, so the assertion at line 38 would fail anyways?

I guess putting Fixes: in the commit message was a bit hasty. I still think we should probably remove the message sniffing, even if it doesn't resolve the test flakiness.

Trott · 2020-08-11T14:58:43Z

In trying to test this, the test fails when run in parallel with itself. I can make it fail with ENFILE that way pretty easily. I'm wondering if the real problem is that this test is somewhat resource-intensive and needs to be either dialed down a little bit or else moved to sequential.

gireeshpunathil · 2020-08-11T15:04:08Z

assert.ok(e.code === 'ERR_WORKER_INIT_FAILED' || e.code === 'EMFILE' || e.code === 'ENOENT');

@Trott - are you able to test (and cause it to fail) with this change to the tests's assertion?

Trott · 2020-08-11T22:11:08Z

assert.ok(e.code === 'ERR_WORKER_INIT_FAILED' || e.code === 'EMFILE' || e.code === 'ENOENT');

@Trott - are you able to test (and cause it to fail) with this change to the tests's assertion?

Unfortunately, no. The errors are varied.

tools/test.py -j 64 --repeat 192 test/parallel/test-worker-init-failure.js
=== release test-worker-init-failure ===                   
Path: parallel/test-worker-init-failure
child stdout: 

child stderr: (libuv) kqueue(): Too many open files in system
net.js:328
      err = this._handle.open(fd);
                         ^

Error: ENFILE: file table overflow, uv_pipe_open
    at new Socket (net.js:328:26)
    at createWritableStdioStream (internal/bootstrap/switches/is_main_thread.js:72:18)
    at process.getStdout [as stdout] (internal/bootstrap/switches/is_main_thread.js:122:12)
    at new Worker (internal/worker.js:179:42)
    at Object.<anonymous> (/Users/trott/io.js/test/parallel/test-worker-init-failure.js:25:20)
    at Module._compile (internal/modules/cjs/loader.js:1265:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1286:10)
    at Module.load (internal/modules/cjs/loader.js:1114:32)
    at Function.Module._load (internal/modules/cjs/loader.js:976:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12) {
  errno: -23,
  code: 'ENFILE',
  syscall: 'uv_pipe_open'
}
assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

    at ChildProcess.<anonymous> (/Users/trott/io.js/test/parallel/test-worker-init-failure.js:63:12)
    at ChildProcess.<anonymous> (/Users/trott/io.js/test/common/index.js:365:15)
    at ChildProcess.emit (events.js:314:20)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 0,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/trott/io.js/test/parallel/test-worker-init-failure.js
=== release test-worker-init-failure ===                   
Path: parallel/test-worker-init-failure
child stdout: 

child stderr: /Users/trott/io.js/out/Release/node[55368]: ../src/tracing/agent.cc:55:node::tracing::Agent::Agent(): Assertion `(uv_loop_init(&tracing_loop_)) == (0)' failed.
 1: 0x1000ae755 node::Abort() [/Users/trott/io.js/out/Release/node]
 2: 0x1000ae5c1 node::Assert(node::AssertionInfo const&) [/Users/trott/io.js/out/Release/node]
 3: 0x10016e028 node::tracing::Agent::Agent() [/Users/trott/io.js/out/Release/node]
 4: 0x1000839a6 node::V8Platform::Initialize(int) [/Users/trott/io.js/out/Release/node]
 5: 0x1000834c3 node::InitializeOncePerProcess(int, char**) [/Users/trott/io.js/out/Release/node]
 6: 0x100083bce node::Start(int, char**) [/Users/trott/io.js/out/Release/node]
 7: 0x7fff6ddbdcc9 start [/usr/lib/system/libdyld.dylib]
 8: 0x3 
/bin/sh: line 1: 55368 Abort trap: 6           /Users/trott/io.js/out/Release/node /Users/trott/io.js/test/parallel/test-worker-init-failure.js child
assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

134 !== 0

    at ChildProcess.<anonymous> (/Users/trott/io.js/test/parallel/test-worker-init-failure.js:63:12)
    at ChildProcess.<anonymous> (/Users/trott/io.js/test/common/index.js:365:15)
    at ChildProcess.emit (events.js:314:20)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 134,
  expected: 0,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/trott/io.js/test/parallel/test-worker-init-failure.js

=== release test-worker-init-failure ===                   
Path: parallel/test-worker-init-failure
Command: out/Release/node /Users/trott/io.js/test/parallel/test-worker-init-failure.js
--- CRASHED (Signal: 11) ---
=== release test-worker-init-failure ===                   
Path: parallel/test-worker-init-failure
out/Release/node[54987]: ../src/tracing/agent.cc:55:node::tracing::Agent::Agent(): Assertion `(uv_loop_init(&tracing_loop_)) == (0)' failed.
 1: 0x1000ae755 node::Abort() [out/Release/node]
 2: 0x1000ae5c1 node::Assert(node::AssertionInfo const&) [out/Release/node]
 3: 0x10016e028 node::tracing::Agent::Agent() [out/Release/node]
 4: 0x1000839a6 node::V8Platform::Initialize(int) [out/Release/node]
 5: 0x1000834c3 node::InitializeOncePerProcess(int, char**) [out/Release/node]
 6: 0x100083bce node::Start(int, char**) [out/Release/node]
 7: 0x7fff6ddbdcc9 start [/usr/lib/system/libdyld.dylib]
Command: out/Release/node /Users/trott/io.js/test/parallel/test-worker-init-failure.js
--- CRASHED (Signal: 6) ---
=== release test-worker-init-failure ===                   
Path: parallel/test-worker-init-failure
child stdout: 

child stderr: /bin/sh: line 1: 55006 Abort trap: 6           /Users/trott/io.js/out/Release/node /Users/trott/io.js/test/parallel/test-worker-init-failure.js child
assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

134 !== 0

    at ChildProcess.<anonymous> (/Users/trott/io.js/test/parallel/test-worker-init-failure.js:63:12)
    at ChildProcess.<anonymous> (/Users/trott/io.js/test/common/index.js:365:15)
    at ChildProcess.emit (events.js:314:20)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 134,
  expected: 0,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/trott/io.js/test/parallel/test-worker-init-failure.js

Unfortunately, the test is sensitive to resource constraints and is unreliable on macOS in CI when in parallel. Fixes: nodejs#34727

gireeshpunathil · 2020-08-12T13:12:50Z

@Trott - thanks. While the test did not expect these failures, with the (resource) constrained execution, these failures are absolutely meaningful.

the first one (Error: ENFILE: file table overflow, uv_pipe_open) is another manifestation of libuv failure when fds runs out.

the secone one (../src/tracing/agent.cc:55:node::tracing::Agent::Agent(): Assertion (uv_loop_init(&tracing_loop_)) == (0)' failed.`) is a worker failure scenario that is not covered under #31621

IMO the first one can be accommodated in the test, while the second one should be fixed in the tracing agent.

nodejs-github-bot · 2020-08-14T01:54:34Z

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

Trott · 2020-08-14T02:29:29Z

I removed "Fixes: " from the commit message.

Landed in 9861962

Refs: nodejs#34727 (comment) PR-URL: nodejs#34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 11, 2020

Trott requested review from addaleax and gireeshpunathil August 11, 2020 14:20

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2020

addaleax approved these changes Aug 11, 2020

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2020

gireeshpunathil reviewed Aug 11, 2020

View reviewed changes

Trott added a commit to Trott/io.js that referenced this pull request Aug 11, 2020

test: move test-worker-init-failure to sequential

346d4fe

Unfortunately, the test is sensitive to resource constraints and is unreliable on macOS in CI when in parallel. Fixes: nodejs#34727

Trott mentioned this pull request Aug 11, 2020

test: move test-worker-init-failure to sequential #34740

Closed

2 tasks

jasnell approved these changes Aug 12, 2020

View reviewed changes

Trott added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 14, 2020

Trott closed this Aug 14, 2020

Trott force-pushed the test-fix branch from 5cf4c45 to 9861962 Compare August 14, 2020 02:29

Trott deleted the test-fix branch August 14, 2020 02:29

Trott merged commit 9861962 into nodejs:master Aug 14, 2020

Trott mentioned this pull request Aug 14, 2020

test: allow ENFILE in test-worker-init-failure #34769

Merged

2 tasks

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: remove error message checking in test-worker-init-failure#34727

test: remove error message checking in test-worker-init-failure#34727
Trott merged 0 commit intonodejs:masterfrom
Trott:test-fix

Trott commented Aug 11, 2020

Uh oh!

nodejs-github-bot commented Aug 11, 2020

Uh oh!

gireeshpunathil Aug 11, 2020

Uh oh!

gireeshpunathil Aug 11, 2020

Uh oh!

Trott Aug 11, 2020

Uh oh!

Trott commented Aug 11, 2020

Uh oh!

gireeshpunathil commented Aug 11, 2020

Uh oh!

Trott commented Aug 11, 2020

Uh oh!

gireeshpunathil commented Aug 12, 2020

Uh oh!

nodejs-github-bot commented Aug 14, 2020

Uh oh!

Trott commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

Trott commented Aug 11, 2020

Checklist

Uh oh!

nodejs-github-bot commented Aug 11, 2020

Uh oh!

gireeshpunathil Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

Trott Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 11, 2020

Uh oh!

gireeshpunathil commented Aug 11, 2020

Uh oh!

Trott commented Aug 11, 2020

Uh oh!

gireeshpunathil commented Aug 12, 2020

Uh oh!

nodejs-github-bot commented Aug 14, 2020

Uh oh!

Trott commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants