test: add error validation to assert.throws calls #11253

ejc-main · 2017-02-09T04:55:15Z

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

Trott · 2017-02-09T05:13:00Z

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.)

Trott

LGTM if CI is green. Definitely +1 on the regular expressions. Thanks for the contribution!

Trott · 2017-02-09T07:05:10Z

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

cjihrig

Did this pass make test for you locally?

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

cjihrig · 2017-02-09T15:02:14Z

test/pummel/test-crypto-dh.js

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


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$/

cjihrig · 2017-02-09T18:25:35Z

test/pummel/test-crypto-dh.js

  '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/ +


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 · 2017-02-11T18:03:12Z

@cjihrig ... PTAL

cjihrig · 2017-02-13T23:13:15Z

test/pummel/test-crypto-dh.js

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


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

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

jasnell · 2017-02-17T00:17:13Z

@cjihrig ... PTAL

jasnell · 2017-02-17T01:11:49Z

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

ejc-main · 2017-02-19T14:35:36Z

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?

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 · 2017-02-19T21:00:56Z

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

jasnell · 2017-02-19T21:01:07Z

Landed in fcf3cbe

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>

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

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>

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>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 9, 2017

ejc-main closed this Feb 9, 2017

ejc-main reopened this Feb 9, 2017

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Feb 9, 2017

Trott approved these changes Feb 9, 2017

View reviewed changes

targos approved these changes Feb 9, 2017

View reviewed changes

cjihrig requested changes Feb 9, 2017

View reviewed changes

ejc-main force-pushed the master branch from 7196680 to d6fa6d7 Compare February 11, 2017 16:16

cjihrig requested changes Feb 13, 2017

View reviewed changes

ejc-main added 5 commits February 14, 2017 09:16

test: add error validation to assert.throws calls

5056741

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

test: fixed code styling of assert.throws calls

86ba1c9

test: add error type to regular expressions

5049fe6

test: fixed assert.throws regular expressions

a0f6e1e

test: change some regular expressions to strings

a20d68e

ejc-main force-pushed the master branch from d6fa6d7 to a20d68e Compare February 14, 2017 15:11

cjihrig approved these changes Feb 17, 2017

View reviewed changes

jasnell approved these changes Feb 19, 2017

View reviewed changes

jasnell closed this Feb 19, 2017

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

tniessen mentioned this pull request Aug 29, 2023

test: simplify test-crypto-dh-group-setters #49404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add error validation to assert.throws calls #11253

test: add error validation to assert.throws calls #11253

ejc-main commented Feb 9, 2017

Trott commented Feb 9, 2017 •

edited

Loading

Trott left a comment •

edited

Loading

Trott commented Feb 9, 2017

cjihrig left a comment •

edited

Loading

cjihrig Feb 9, 2017

cjihrig Feb 9, 2017

jasnell commented Feb 11, 2017

cjihrig Feb 13, 2017

jasnell commented Feb 17, 2017

jasnell commented Feb 17, 2017

ejc-main commented Feb 19, 2017

jasnell commented Feb 19, 2017

jasnell commented Feb 19, 2017

test: add error validation to assert.throws calls #11253

test: add error validation to assert.throws calls #11253

Conversation

ejc-main commented Feb 9, 2017

Checklist

Affected core subsystem(s)

Trott commented Feb 9, 2017 • edited Loading

Trott left a comment • edited Loading

Choose a reason for hiding this comment

Trott commented Feb 9, 2017

cjihrig left a comment • edited Loading

Choose a reason for hiding this comment

cjihrig Feb 9, 2017

Choose a reason for hiding this comment

cjihrig Feb 9, 2017

Choose a reason for hiding this comment

jasnell commented Feb 11, 2017

cjihrig Feb 13, 2017

Choose a reason for hiding this comment

jasnell commented Feb 17, 2017

jasnell commented Feb 17, 2017

ejc-main commented Feb 19, 2017

jasnell commented Feb 19, 2017

jasnell commented Feb 19, 2017

Trott commented Feb 9, 2017 •

edited

Loading

Trott left a comment •

edited

Loading

cjihrig left a comment •

edited

Loading