Skip to content

test: improve test coverage in child_process#26282

Closed
juanarbol wants to merge 1 commit intonodejs:masterfrom
juanarbol:child-process-cov
Closed

test: improve test coverage in child_process#26282
juanarbol wants to merge 1 commit intonodejs:masterfrom
juanarbol:child-process-cov

Conversation

@juanarbol
Copy link
Member

@juanarbol juanarbol commented Feb 24, 2019

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 24, 2019
ZYSzys
ZYSzys reviewed Feb 24, 2019
View reviewed changes
juanarbol
juanarbol commented Feb 24, 2019
View reviewed changes
cjihrig
cjihrig reviewed Feb 24, 2019
View reviewed changes
juanarbol
juanarbol commented Feb 24, 2019
View reviewed changes
BridgeAR
BridgeAR reviewed Mar 5, 2019
View reviewed changes
BridgeAR
BridgeAR reviewed Mar 5, 2019
View reviewed changes
juanarbol
juanarbol commented Mar 6, 2019
View reviewed changes
@juanarbol
Copy link
Member Author

juanarbol commented Mar 6, 2019

I ran make -j4 test :(

@BridgeAR
Copy link
Member

BridgeAR commented Mar 10, 2019

@juanarbol seems like this requires a rebase. Would you be so kind and just rebase + force-push? Otherwise we can't run the CI.

@juanarbol juanarbol force-pushed the child-process-cov branch 2 times, most recently from d7b7781 to 4169280 Compare March 12, 2019 20:12
@juanarbol
Copy link
Member Author

juanarbol commented Mar 12, 2019

Sorry, I didn't see the rule of uppercased comments so I had to re-force-push to make CI pass.

@BridgeAR thank you so much! I learned a lot!!

mhdawson
mhdawson approved these changes Mar 12, 2019
View reviewed changes
Copy link
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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 12, 2019

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

@juanarbol don't worry, it is nice that you fixed that as it's less work for the person to merge the PR. Otherwise we'd fix the commit message while landing.

@juanarbol
Copy link
Member Author

juanarbol commented Mar 12, 2019

Happy to help!

@juanarbol
Copy link
Member Author

juanarbol commented Mar 24, 2019

Ping @BridgeAR

ZYSzys
ZYSzys approved these changes Mar 26, 2019
View reviewed changes
test/parallel/test-child-process-validate-stdio.js Outdated
Copy link
Member

@ZYSzys ZYSzys Mar 26, 2019

Choose a reason for hiding this comment

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

I think it would be better if we can collect all invalid options which both throw expectedError here(for reduce duplicated code).

Things like:

['foo', 600, [{ foo: 'bar' }]].forEach((stdio) => {
  common.expectsError(() => _validateStdio(stdio), expectedError);
})

But it shouldn't block this :)

@BridgeAR
Copy link
Member

BridgeAR commented Mar 27, 2019

@juanarbol sorry, this requires another rebase.

@juanarbol
Copy link
Member Author

juanarbol commented Mar 28, 2019

I've make jstest and make test both passes on my computer

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 3, 2019

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 9, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 15, 2019

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 16, 2019

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 16, 2019

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

Trott
Trott reviewed Dec 17, 2019
View reviewed changes
test/parallel/test-child-process-validate-stdio.js

// Should throw if string and not ignore, pipe, or inherit
assert.throws(() => getValidStdio('foo'), expectedError);
common.expectsError(() => getValidStdio('foo'), expectedError);
Copy link
Member

@Trott Trott Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please leave it as assert.throws() here and elsewhere? Better to use the public assert than our core-only common if there's no difference.

Copy link
Member

@Trott Trott Dec 17, 2019

Choose a reason for hiding this comment

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

Oh, I see, this just moves the expectsError, rather than introducing it. Never mind my comment. Should eventually be removed anyway but not necessarily in this PF and it probably needs to wait for Node.js 8 to EOL.

@Trott
Copy link
Member

Trott commented Dec 17, 2019

Landed in 14af00e

@Trott Trott closed this Dec 17, 2019
Trott pushed a commit that referenced this pull request Dec 17, 2019
@juanarbol @Trott
test: improve test coverage in child_process
14af00e 
PR-URL: #26282
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@juanarbol @MylesBorins
test: improve test coverage in child_process
72b4aee 
PR-URL: #26282
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@juanarbol @targos
test: improve test coverage in child_process
e6907f0 
PR-URL: #26282
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@juanarbol @BethGriggs
test: improve test coverage in child_process
d633ba0 
PR-URL: #26282
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
@juanarbol juanarbol deleted the child-process-cov branch January 19, 2021 16:23
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 left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@mhdawson mhdawson mhdawson approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@juanarbol @BridgeAR @nodejs-github-bot @Trott @cjihrig @mhdawson @ZYSzys