Skip to content

test: decrease duration of test-cli-syntax#14187

Closed
evanlucas wants to merge 1 commit intonodejs:masterfrom
evanlucas:testclisyntax
Closed

test: decrease duration of test-cli-syntax#14187
evanlucas wants to merge 1 commit intonodejs:masterfrom
evanlucas:testclisyntax

Conversation

@evanlucas
Copy link
Copy Markdown
Contributor

@evanlucas evanlucas commented Jul 12, 2017

Previously, test/parallel/test-cli-syntax.js was spawning a lot of child
processes, but using spawnSync, which made the test run each child
process serially. This switches most of the test cases to use exec so
that they are asynchronous. Locally, the test went from > 5 seconds to
under 2 seconds.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@evanlucas
test: decrease duration of test-cli-syntax
cb13539 
Previously, test/parallel/test-cli-syntax.js was spawning a lot of child
processes, but using spawnSync, which made the test run each child
process serially. This switches most of the test cases to use exec so
that they are asynchronous. Locally, the test went from > 5 seconds to
under 2 seconds.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 12, 2017
@evanlucas
Copy link
Copy Markdown
Contributor Author

evanlucas commented Jul 12, 2017

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

@mscdex mscdex added the cli Issues and PRs related to the Node.js command line interface. label Jul 12, 2017
mhdawson
mhdawson approved these changes Jul 12, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jul 12, 2017

Like the other changes to address the slowdown caused by disabling snapshots I'd suggest we expedite landing this one as well.

Trott
Trott approved these changes Jul 12, 2017
View reviewed changes
Copy link
Copy Markdown
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
refack approved these changes Jul 12, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

1 Nit

Comment thread test/parallel/test-cli-syntax.js
const common = require('../common');
const assert = require('assert');
const spawnSync = require('child_process').spawnSync;
const {exec, spawnSync} = require('child_process');
Copy link
Copy Markdown
Contributor

@refack refack Jul 12, 2017

Choose a reason for hiding this comment

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

space inside the curly braces #14162

Copy link
Copy Markdown
Contributor Author

@evanlucas evanlucas Jul 12, 2017

Choose a reason for hiding this comment

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

Is that a lint rule? If so, when did that change?

Copy link
Copy Markdown
Contributor

@refack refack Jul 12, 2017

Choose a reason for hiding this comment

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

Is that a lint rule? If so, when did that change?

about to land...

@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 12, 2017

Why not refactor the other two uses of spawnSync?

@evanlucas
Copy link
Copy Markdown
Contributor Author

evanlucas commented Jul 12, 2017

@refack they use spawnSync's input option which writes stdin to the process.

@evanlucas
Copy link
Copy Markdown
Contributor Author

evanlucas commented Jul 13, 2017

Trying CI one more time https://ci.nodejs.org/job/node-test-pull-request/9108/

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jul 13, 2017

CI was good landing.

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jul 13, 2017

Landed as a74ddff

@mhdawson mhdawson closed this Jul 13, 2017
mhdawson pushed a commit that referenced this pull request Jul 13, 2017
@evanlucas @mhdawson
test: decrease duration of test-cli-syntax
a74ddff 
Previously, test/parallel/test-cli-syntax.js was spawning a lot of child
processes, but using spawnSync, which made the test run each child
process serially. This switches most of the test cases to use exec so
that they are asynchronous. Locally, the test went from > 5 seconds to
under 2 seconds.

PR-URL: #14187
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@evanlucas
Copy link
Copy Markdown
Contributor Author

evanlucas commented Jul 13, 2017

Ah thanks @mhdawson. I meant to land this morning but forgot about it. :]

@evanlucas evanlucas deleted the testclisyntax branch July 13, 2017 22:54
@addaleax addaleax mentioned this pull request Jul 18, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@evanlucas @addaleax
test: decrease duration of test-cli-syntax
1671fe4 
Previously, test/parallel/test-cli-syntax.js was spawning a lot of child
processes, but using spawnSync, which made the test run each child
process serially. This switches most of the test cases to use exec so
that they are asynchronous. Locally, the test went from > 5 seconds to
under 2 seconds.

PR-URL: #14187
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@evanlucas @Fishrock123
test: decrease duration of test-cli-syntax
e726f52 
Previously, test/parallel/test-cli-syntax.js was spawning a lot of child
processes, but using spawnSync, which made the test run each child
process serially. This switches most of the test cases to use exec so
that they are asynchronous. Locally, the test went from > 5 seconds to
under 2 seconds.

PR-URL: #14187
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Aug 16, 2017

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

MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@evanlucas @MylesBorins
test: decrease duration of test-cli-syntax
a4e2ced 
Previously, test/parallel/test-cli-syntax.js was spawning a lot of child
processes, but using spawnSync, which made the test run each child
process serially. This switches most of the test cases to use exec so
that they are asynchronous. Locally, the test went from > 5 seconds to
under 2 seconds.

PR-URL: #14187
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 13, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@refack refack refack approved these changes

@mhdawson mhdawson mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cli Issues and PRs related to the Node.js command line interface. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@evanlucas @mhdawson @refack @MylesBorins @Trott @cjihrig @mscdex @nodejs-github-bot