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.

Sign up for GitHub

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

Jump to bottom

test: assert that invalidcmd throws error code #23942

Closed
wants to merge 1 commit into from
Closed

test: assert that invalidcmd throws error code #23942

wants to merge 1 commit into from

Conversation

jeromecovington
Copy link
Contributor

Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.

I've been interested in custom error codes in core for various reasons,
@Trott suggested this refactor as a place to start looking into usage in
tests.

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 Oct 28, 2018
lpinca
lpinca reviewed Oct 28, 2018
View reviewed changes
test/parallel/test-child-process-spawn-typeerror.js Outdated
@@ -35,10 +35,14 @@ const invalidArgValueError =
const invalidArgTypeError =
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 12);
Copy link
Member

Choose a reason for hiding this comment

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

Can we simply change the second argument to 13 here and reuse invalidArgTypeError instead of defining invalidArgTypeErrorCallsOnce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 yes of course.

@jeromecovington jeromecovington force-pushed the refactor/test-child-process-spawn-typeerror branch from 2349ac6 to d69a3d5 Compare October 29, 2018 00:36
@Trott
Copy link
Member

Trott commented Oct 29, 2018

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 29, 2018
@jeromecovington
Copy link
Contributor Author

I was able to trace the test-commit failure to this error which seems harmless enough, although this is the first time I've tracked something like this down.

@Trott
Copy link
Member

Trott commented Oct 31, 2018

CI resume build: https://ci.nodejs.org/job/node-test-pull-request/18268/

@jeromecovington
Copy link
Contributor Author

The travis ci error I'm seeing is:

✖  0:0      Missing subsystem.                        subsystem

...Presumably for the commit message guidelines, but I think I covered that? The subsystem is tests, right?

@jeromecovington
Copy link
Contributor Author

...Oh...the subsystem looks to be test. Right, will fix.

@jeromecovington
test: assert that invalidcmd throws error code
b55906e 
Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.
@jeromecovington jeromecovington force-pushed the refactor/test-child-process-spawn-typeerror branch from d69a3d5 to b55906e Compare November 1, 2018 18:05
@jeromecovington jeromecovington changed the title tests: assert that invalidcmd throws error code test: assert that invalidcmd throws error code Nov 1, 2018
@jeromecovington
Copy link
Contributor Author

Hm. Updated the subsystem from tests to test in the commit message but that missing subsystem error still seems to show up in travis.

@richardlau
Copy link
Member

Hm. Updated the subsystem from tests to test in the commit message but that missing subsystem error still seems to show up in travis.

Travis is linting the wrong commit (9d7895c, which happens to be the very first commit in this repository).

cc fyi @Trott

@richardlau
Copy link
Member

@jeromecovington You could try rebasing onto the current master branch.

@Trott
Copy link
Member

Trott commented Nov 2, 2018

Don't worry about the commit linting. It lints the first commit in the PR. If there's a problem with the commit message, our tooling for landing will alert the person landing the PR, so it's not a big deal.

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

@Trott
Copy link
Member

Trott commented Nov 2, 2018

Landed in 878f587

@Trott Trott closed this Nov 2, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 2, 2018
@jeromecovington @Trott
test: assert that invalidcmd throws error code
878f587 
Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.

PR-URL: nodejs#23942
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 2, 2018

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

targos pushed a commit that referenced this pull request Nov 2, 2018
@jeromecovington @targos
test: assert that invalidcmd throws error code
1b97dbd 
Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.

PR-URL: #23942
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 14, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
@jeromecovington @MylesBorins
test: assert that invalidcmd throws error code
203c420 
Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.

PR-URL: #23942
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@jeromecovington @rvagg
test: assert that invalidcmd throws error code
9f4d9a2 
Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.

PR-URL: #23942
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@jeromecovington @MylesBorins
test: assert that invalidcmd throws error code
3c314d1 
Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.

PR-URL: #23942
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
@jeromecovington @MylesBorins
test: assert that invalidcmd throws error code
b3ab546 
Update invalidcmd test case in test-child-process-spawn-typeerror to
assert on specific expected error code.

PR-URL: #23942
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack 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.
9 participants
@jeromecovington @Trott @richardlau @refack @jasnell @lpinca @cjihrig @MylesBorins @nodejs-github-bot