test: fix test failure on aix #20940

BridgeAR · 2018-05-24T14:33:15Z

This makes sure there is enough stack space on different systems. 75 should definitely be enough on all systems.

Alternative to #20927

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

This makes sure there is enough stack space on different systems.

gireeshpunathil · 2018-05-24T14:43:51Z

75 works in my AIX box. Further thinking on it, --stack_size is not a function of the OS thread stack size, instead the amount we are offering to v8 - which means this should be platform agnostic from the native stack perspective, but a function of how much frame size each JS code has consumed already, which can be platform specific. (An explanation for the platform different behaviors, if anyone is interested).

richardlau · 2018-05-24T14:48:45Z

I'm not going to block this, but I am wary of magic values in test files. Can you guarantee 75 will be a safe value in the future? It feels like setting the stack_size introduces an inherent flakiness -- we picked 50 previously and that turned out to be insufficient.

gireeshpunathil · 2018-05-24T14:54:25Z

I guess the number which we are taking about is the amount of kilobytes that is required for the frames to execute the methods in this call chain:

RangeError: Maximum call stack size exceeded
    at runInThisContext (bootstrap_node.js:496:20)
    at NativeModule.compile (bootstrap_node.js:591:18)
    at NativeModule.require (bootstrap_node.js:541:18)
    at module.js:30:12
    at NativeModule.compile (bootstrap_node.js:596:7)
    at Function.NativeModule.require (bootstrap_node.js:541:18)
    at setupGlobalConsole (bootstrap_node.js:308:33)
    at startup (bootstrap_node.js:70:7)
    at bootstrap_node.js:608:3

if those sequences change and start consuming more stack space, 75 will become insufficient too.

BridgeAR · 2018-05-24T14:55:03Z

@richardlau I was surprised that it turned out as insufficient because I expected it to be a fixed value on all platforms. However, it would be very surprising if the new value would not be sufficient anymore (also in future cases) because that could only happen in case V8 changes something significantly to the worse or we add multiple extra layers and that is also very unlikely.

We should always realize if the value is insufficient on a CI run and I guess the aix failure originally just slipped through.

Trott · 2018-05-24T18:36:57Z

FWIW, this was also observed by @danbev on a macOS laptop. Not sure if that warrants changing the description from saying AIX or not.

Also in the "might not really require amending the commit message" department: The stylization of AIX is all caps.

addaleax · 2018-05-24T19:41:03Z

test/parallel/test-async-wrap-pop-id-during-load.js

@@ -15,7 +15,7 @@ const { spawnSync } = require('child_process');

 const ret = spawnSync(
  process.execPath,
-  ['--stack_size=50', __filename, 'async']
+  ['--stack_size=75', __filename, 'async']
 );
 assert.strictEqual(ret.status, 0);


Fwiw, I think this would have been more easily debuggable if this assertion had come last and the assertion on L22 would print out stderr in its failure message – maybe we can fix up the test for that as well?

Oh, actually, I had a branch to fix this and then didn't open a PR, but maybe it can be tacked onto this. IIRC, the change was this:

assert.strictEqual(ret.status, 0, `EXIT CODE: ${ret.status}, STDERR: ${ret.stderr}`);

I opened #20948 so that this one doesn't get delayed with any bike-shedding or CI re-running.

BridgeAR · 2018-05-25T10:46:32Z

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

BridgeAR · 2018-05-25T16:51:28Z

Please +1 to fast track this.

joyeecheung · 2018-05-27T09:32:06Z

There were some infra issues with the last CI. New CI: https://ci.nodejs.org/job/node-test-pull-request/15113/

Trott · 2018-05-28T03:50:01Z

node-test-commit-osx re-run: https://ci.nodejs.org/job/node-test-commit-osx/18888/

Trott · 2018-05-28T03:51:18Z

(Removed fast-track label because it's been open for four days and has three approvals, so all it needs is a clean CI.)

This makes sure there is enough stack space on different systems. PR-URL: nodejs#20940 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Trott · 2018-05-28T05:55:39Z

Landed in cfc3866

This makes sure there is enough stack space on different systems. PR-URL: #20940 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Currently, when building with --debug test-async-wrap-pop-id-during-load fails on macosx with the following error: $ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js assert.js:86 throw new AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR: internal/bootstrap/loaders.js:275 const script = new ContextifyScript( ^ RangeError: Maximum call stack size exceeded at NativeModule.compile (internal/bootstrap/loaders.js:275:22) at NativeModule.require (internal/bootstrap/loaders.js:168:18) at assert.js:31:43 at NativeModule.compile (internal/bootstrap/loaders.js:299:7) at NativeModule.require (internal/bootstrap/loaders.js:168:18) at internal/process/main_thread_only.js:23:16 at NativeModule.compile (internal/bootstrap/loaders.js:299:7) at Function.NativeModule.require (internal/bootstrap/loaders.js:168:18) at startup (internal/bootstrap/node.js:58:38) at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3) at Object.<anonymous> (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8) at Module._compile (internal/modules/cjs/loader.js:707:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10) at Module.load (internal/modules/cjs/loader.js:605:32) at tryModuleLoad (internal/modules/cjs/loader.js:544:12) at Function.Module._load (internal/modules/cjs/loader.js:536:3) at Function.Module.runMain (internal/modules/cjs/loader.js:760:12) at startup (internal/bootstrap/node.js:308:19) at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3) This commit suggests increasing the stack_size to 80. Refs: nodejs#20940

Currently, when building with --debug test-async-wrap-pop-id-during-load fails on macosx with the following error: $ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js assert.js:86 throw new AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR: internal/bootstrap/loaders.js:275 const script = new ContextifyScript( ^ RangeError: Maximum call stack size exceeded at NativeModule.compile (internal/bootstrap/loaders.js:275:22) at NativeModule.require (internal/bootstrap/loaders.js:168:18) at assert.js:31:43 at NativeModule.compile (internal/bootstrap/loaders.js:299:7) at NativeModule.require (internal/bootstrap/loaders.js:168:18) at internal/process/main_thread_only.js:23:16 at NativeModule.compile (internal/bootstrap/loaders.js:299:7) at Function.NativeModule.require (internal/bootstrap/loaders.js:168:18) at startup (internal/bootstrap/node.js:58:38) at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3) at Object.<anonymous> (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8) at Module._compile (internal/modules/cjs/loader.js:707:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10) at Module.load (internal/modules/cjs/loader.js:605:32) at tryModuleLoad (internal/modules/cjs/loader.js:544:12) at Function.Module._load (internal/modules/cjs/loader.js:536:3) at Function.Module.runMain (internal/modules/cjs/loader.js:760:12) at startup (internal/bootstrap/node.js:308:19) at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3) This commit suggests increasing the stack_size to 80. Refs: nodejs#20940 PR-URL: nodejs#23996 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Currently, when building with --debug test-async-wrap-pop-id-during-load fails on macosx with the following error: $ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js assert.js:86 throw new AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR: internal/bootstrap/loaders.js:275 const script = new ContextifyScript( ^ RangeError: Maximum call stack size exceeded at NativeModule.compile (internal/bootstrap/loaders.js:275:22) at NativeModule.require (internal/bootstrap/loaders.js:168:18) at assert.js:31:43 at NativeModule.compile (internal/bootstrap/loaders.js:299:7) at NativeModule.require (internal/bootstrap/loaders.js:168:18) at internal/process/main_thread_only.js:23:16 at NativeModule.compile (internal/bootstrap/loaders.js:299:7) at Function.NativeModule.require (internal/bootstrap/loaders.js:168:18) at startup (internal/bootstrap/node.js:58:38) at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3) at Object.<anonymous> (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8) at Module._compile (internal/modules/cjs/loader.js:707:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10) at Module.load (internal/modules/cjs/loader.js:605:32) at tryModuleLoad (internal/modules/cjs/loader.js:544:12) at Function.Module._load (internal/modules/cjs/loader.js:536:3) at Function.Module.runMain (internal/modules/cjs/loader.js:760:12) at startup (internal/bootstrap/node.js:308:19) at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3) This commit suggests increasing the stack_size to 80. Refs: #20940 PR-URL: #23996 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

test: fix test failure on aix

34198c7

This makes sure there is enough stack space on different systems.

nodejs-github-bot added the test Issues and PRs related to the tests. label May 24, 2018

gireeshpunathil approved these changes May 24, 2018

View reviewed changes

Trott approved these changes May 24, 2018

View reviewed changes

addaleax approved these changes May 24, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 25, 2018

BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label May 25, 2018

Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label May 28, 2018

danbev approved these changes May 28, 2018

View reviewed changes

Trott closed this May 28, 2018

Trott mentioned this pull request May 28, 2018

Revert "test: reduce runtime" #20927

Closed

1 task

MylesBorins mentioned this pull request May 29, 2018

v10.3.0 proposal #21011

Merged

danbev mentioned this pull request Oct 31, 2018

test: increase --stack_size test-async-wrap-pop #23996

Closed

2 tasks

BridgeAR deleted the fix-aix-failure branch January 20, 2020 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix test failure on aix #20940

test: fix test failure on aix #20940

BridgeAR commented May 24, 2018

gireeshpunathil commented May 24, 2018

richardlau commented May 24, 2018

gireeshpunathil commented May 24, 2018

BridgeAR commented May 24, 2018

Trott commented May 24, 2018

addaleax May 24, 2018

Trott May 24, 2018

Trott May 24, 2018

BridgeAR commented May 25, 2018

BridgeAR commented May 25, 2018

joyeecheung commented May 27, 2018

Trott commented May 28, 2018

Trott commented May 28, 2018

Trott commented May 28, 2018

test: fix test failure on aix #20940

test: fix test failure on aix #20940

Conversation

BridgeAR commented May 24, 2018

Checklist

gireeshpunathil commented May 24, 2018

richardlau commented May 24, 2018

gireeshpunathil commented May 24, 2018

BridgeAR commented May 24, 2018

Trott commented May 24, 2018

addaleax May 24, 2018

Choose a reason for hiding this comment

Trott May 24, 2018

Choose a reason for hiding this comment

Trott May 24, 2018

Choose a reason for hiding this comment

BridgeAR commented May 25, 2018

BridgeAR commented May 25, 2018

joyeecheung commented May 27, 2018

Trott commented May 28, 2018

Trott commented May 28, 2018

Trott commented May 28, 2018