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.

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

test: try to fix test-cli-node-options #14195

Merged
merged 1 commit into from
Sep 4, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 12, 2017

  • partitioning the sub-process groups
  • use -e instead of module
  • reduce maxBuffer

Fixes: #14191

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

test,process,cli

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 12, 2017
@refack refack added flaky-test Issues and PRs related to the tests with unstable failures on the CI. cli Issues and PRs related to the Node.js command line interface. process Issues and PRs related to the process subsystem. arm Issues and PRs related to the ARM platform. labels Jul 12, 2017
@refack
Copy link
Contributor Author

refack commented Jul 12, 2017

@refack
Copy link
Contributor Author

refack commented Jul 12, 2017

/cc @nodejs/platform-arm @nodejs/testing @sam-github

@refack refack requested a review from sam-github July 12, 2017 16:30
@refack
Copy link
Contributor Author

refack commented Jul 12, 2017

Anecdotal number from my local Windows machine:

D:\code\node>time < nul&D:\bin\dev\node\node9pre.exe D:\code\node\test\parallel\test-cli-node-options.js&time<nul
The current time is: 12:31:15.20
Enter the new time: The current time is: 12:31:19.92
Enter the new time:
D:\code\node>git checkout master
Switched to branch 'master'
Your branch is ahead of 'refack/master' by 4 commits.
  (use "git push" to publish your local commits)

D:\code\node>time < nul&D:\bin\dev\node\node9pre.exe D:\code\node\test\parallel\test-cli-node-options.js&time<nul
The current time is: 12:31:28.26
Enter the new time: The current time is: 12:31:34.14
Enter the new time:
D:\code\node>

After PR ~4.72s
Pre PR ~5.88s
Down to ~80% time (a.k.a 25% improvement 🤓)

maxBuffer: 1000000000,
};
exec(process.execPath, argv, opts, common.mustCall(function(err, stdout) {
const argv = ['-e', 'console.log("B")'];
Copy link
Member

Choose a reason for hiding this comment

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

If all we're doing with -e is console.log() perhaps consider using -p instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@refack
Copy link
Contributor Author

refack commented Jul 12, 2017

-p 5.17s, 5.16s, 5.11s
-e 4.92s, 4.78s, 4.76

(faster by 5% with p = 2.3E-05)
Going back 🤷‍♂️

@refack refack force-pushed the tweak-not-test-cli-node-options branch from 31efaf7 to e87ba7e Compare July 12, 2017 17:15
@refack
Copy link
Contributor Author

refack commented Jul 12, 2017

https://ci.nodejs.org/job/node-test-binary-arm/9184/RUN_SUBSET=2,label=pi1-raspbian-wheezy/console

ok 30 parallel/test-cli-node-options-disallowed
  ---
  duration_ms: 9.884
  ...

https://ci.nodejs.org/job/node-test-binary-arm/9184/RUN_SUBSET=3,label=pi1-raspbian-wheezy/console

ok 29 parallel/test-cli-node-options
  ---
  duration_ms: 149.423
  ...

@refack
Copy link
Contributor Author

refack commented Jul 12, 2017

irrelevant

@nodejs/testing
Ready for expedited landing

@BridgeAR
Copy link
Member

Needs a rebase

@refack refack force-pushed the tweak-not-test-cli-node-options branch from e87ba7e to b6bb016 Compare September 3, 2017 21:44
@refack
Copy link
Contributor Author

refack commented Sep 3, 2017

This is mainly to avoid timing out, currently on pi1-raspbian-wheezy the joined file can take > 40s

ok 31 parallel/test-cli-node-options
  ---
  duration_ms: 40.833
  ...

(https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=1,label=pi1-raspbian-wheezy/9971)

split files take:

ok 30 parallel/test-cli-node-options
  ---
  duration_ms: 32.395
  ...
ok 31 parallel/test-cli-node-options-disallowed
  ---
  duration_ms: 5.89
  ...

/cc @nodejs/testing

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

@refack
Copy link
Contributor Author

refack commented Sep 4, 2017

* partitioning the subprocess groups
* use `-e` instead of module
* reduce maxBuffer

PR-URL: nodejs#14195
Fixes: nodejs#14191
Reviewed-By: Rich Trott <[email protected]>
@refack refack force-pushed the tweak-not-test-cli-node-options branch from b6bb016 to 86e7c61 Compare September 4, 2017 21:52
@refack refack merged commit 86e7c61 into nodejs:master Sep 4, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
* partitioning the subprocess groups
* use `-e` instead of module
* reduce maxBuffer

PR-URL: nodejs/node#14195
Fixes: nodejs/node#14191
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
* partitioning the subprocess groups
* use `-e` instead of module
* reduce maxBuffer

PR-URL: #14195
Fixes: #14191
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
* partitioning the subprocess groups
* use `-e` instead of module
* reduce maxBuffer

PR-URL: #14195
Fixes: #14191
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
* partitioning the subprocess groups
* use `-e` instead of module
* reduce maxBuffer

PR-URL: #14195
Fixes: #14191
Reviewed-By: Rich Trott <[email protected]>
@refack refack deleted the tweak-not-test-cli-node-options branch September 18, 2017 01:02
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Issues and PRs related to the ARM platform. cli Issues and PRs related to the Node.js command line interface. flaky-test Issues and PRs related to the tests with unstable failures on the CI. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: post-snapshot test-cli-node-options timeouts on RasberryPi
6 participants