test: improve test coverage in child_process #26282
Conversation
|
I ran |
|
@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
force-pushed
the
child-process-cov
branch
2 times, most recently
from
d7b7781
to
4169280
Compare
|
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!! |
|
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. |
|
Happy to help! |
|
Ping @BridgeAR |
|
|
||
| // Should throw if stdio is not a valid option | ||
| { | ||
| const stdio = [{ foo: 'bar' }]; |
There was a problem hiding this comment.
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 :)
|
@juanarbol sorry, this requires another rebase. |
juanarbol
force-pushed
the
child-process-cov
branch
from
4169280
to
dc0998c
Compare
|
I've |
This comment has been minimized.
This comment has been minimized.
BridgeAR
added
the
author ready
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
targos
force-pushed
the
child-process-cov
branch
from
dc0998c
to
1711bbe
Compare
|
|
||
| // Should throw if string and not ignore, pipe, or inherit | ||
| assert.throws(() => getValidStdio('foo'), expectedError); | ||
| common.expectsError(() => getValidStdio('foo'), expectedError); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Landed in 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>
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>
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>
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>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes