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: add error validation to assert.throws calls #11253

Closed
wants to merge 5 commits into from

Conversation

ejc-main
Copy link
Contributor

@ejc-main ejc-main commented Feb 9, 2017

Used regular expressions to validate error messages.
Also added messages (third parameter) to the assert.throws calls.

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

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 9, 2017
@ejc-main ejc-main closed this Feb 9, 2017
@ejc-main ejc-main reopened this Feb 9, 2017
@Trott
Copy link
Member

Trott commented Feb 9, 2017

I'd be really surprised if make test/vcbuild test didn't fail on this in its current state. Could you please be sure to run make test (or, on Windows, vcbuild test) on code you submit?

(In this case, make jslint is enough because pummel tests don't get run by make test so we're really just linting, which make test does after running the tests. But make jslint will save you a few minutes.)

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Feb 9, 2017
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. Definitely +1 on the regular expressions. Thanks for the contribution!

@Trott
Copy link
Member

Trott commented Feb 9, 2017

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Did this pass make test for you locally?

EDIT: Nevermind, I see this is a pummel test.

function() {
crypto.getDiffieHellman('unknown-group');
},
/^Unknown group$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these regular expressions will work. I think you need to include the error type as well. In this case:

/^Error: Unknown group$/

'crypto.getDiffieHellman(\'unknown-group\') ' +
'failed to throw the expected error.'
);
assert.throws(
function() {
crypto.getDiffieHellman('modp1').setPrivateKey('');
},
/^crypto\.getDiffieHellman\(\.\.\.\)\.setPrivateKey is not a function$/,
/^TypeError: crypto\.getDiffieHellman\(\.\.\.\)\.setPrivateKey/ +
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this does what you're attempting to do. You'll probably have to use the RegExp() constructor here and build the regex out of strings.

@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

@cjihrig ... PTAL

function() {
crypto.getDiffieHellman('modp1').setPrivateKey('');
},
new RegExp(/^TypeError: crypto\.getDiffieHellman\(\.\.\.\)\./.source +
Copy link
Contributor

Choose a reason for hiding this comment

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

You can work with strings here. You don't need to pass regular expression literals to the RegExp constructor.

@jasnell
Copy link
Member

jasnell commented Feb 17, 2017

@cjihrig ... PTAL

@jasnell
Copy link
Member

jasnell commented Feb 17, 2017

@ejc-main
Copy link
Contributor Author

I'm not sure why it's saying

test/arm — tests failed!

When you check the details there are no indications of any tests failing. Could someone explain?

jasnell pushed a commit that referenced this pull request Feb 19, 2017
PR-URL: #11253
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 19, 2017

The arm build bot just reports it's status incorrectly from time to time. All is good

@jasnell
Copy link
Member

jasnell commented Feb 19, 2017

Landed in fcf3cbe

@jasnell jasnell closed this Feb 19, 2017
addaleax pushed a commit that referenced this pull request Feb 22, 2017
PR-URL: #11253
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11253
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11253
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11253
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11253
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
tniessen added a commit to tniessen/node that referenced this pull request Aug 29, 2023
I can't tell why the test was written that way in the first place, but
it seems sufficient to check that setPrivateKey and setPublicKey are
both undefined.

Refs: nodejs/node-v0.x-archive#2638
Refs: nodejs#11253
nodejs-github-bot pushed a commit that referenced this pull request Aug 31, 2023
I can't tell why the test was written that way in the first place, but
it seems sufficient to check that setPrivateKey and setPublicKey are
both undefined.

Refs: nodejs/node-v0.x-archive#2638
Refs: #11253
PR-URL: #49404
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
I can't tell why the test was written that way in the first place, but
it seems sufficient to check that setPrivateKey and setPublicKey are
both undefined.

Refs: nodejs/node-v0.x-archive#2638
Refs: #11253
PR-URL: #49404
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
I can't tell why the test was written that way in the first place, but
it seems sufficient to check that setPrivateKey and setPublicKey are
both undefined.

Refs: nodejs/node-v0.x-archive#2638
Refs: nodejs#11253
PR-URL: nodejs#49404
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants