tools: enforce two arguments in assert.throws #12270

targos · 2017-04-07T14:22:22Z

First commit adds a RegExp argument to the remaining places where it is missing in preparation for the second commit that enforces the presence of at least two arguments in assert.throws().

Checklist

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

Affected core subsystem(s)

test, tools

jasongin · 2017-04-07T17:10:41Z

test/addons-napi/test_constructor/test.js

-assert.throws(() => { test_object.readonlyValue = 3; });
+const expected = new RegExp(
+  '^TypeError: Cannot assign to read only property \'readonlyValue\' of ' +
+  'object \'#<MyObject>\'$'


Matching the complete error message seems fragile. What if V8 changes the wording of the error message?

Would matching the type of error object be sufficient, using the overload of assert.throws() that takes a constructor?

assert.throws(() => { /* some code that throws a TypeError */ }, TypeError);

jasongin · 2017-04-07T17:11:33Z

test/parallel/test-assert-checktag.js

@@ -45,7 +45,8 @@ function re(literals, ...values) {
  }
  assert.doesNotThrow(() => assert.deepEqual(fakeGlobal, global));
  // Message will be truncated anyway, don't validate
-  assert.throws(() => assert.deepStrictEqual(fakeGlobal, global));
+  assert.throws(() => assert.deepStrictEqual(fakeGlobal, global),
+                /^AssertionError: /);


As above, consider just passing the AssertionError constructor here instead of a regexp.

Will do. I actually tried to put AssertionError first but realized it's not globally available. Your comment made me look closer and I saw that it is exported by the assert module.

Trott

LGTM (with changes suggested by @jasongin). I'm slightly bummed about this, though, because I was going to hand all these changes out at Code + Learn later this month. :-D

targos · 2017-04-10T06:27:05Z

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

This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: nodejs#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Enables the requireTwo option of our custom rule. PR-URL: nodejs#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

gibfahn · 2017-06-18T12:37:07Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

gibfahn · 2017-06-18T12:40:31Z

@targos when you merge multiple commits, could you still comment with Landed in HASH1 and HASH2 as per the Collaborator Guide?

targos · 2017-06-19T08:44:50Z

Backport PR: #13785

This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Enables the requireTwo option of our custom rule. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Enables the requireTwo option of our custom rule. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Enables the requireTwo option of our custom rule. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: nodejs#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. Backport-PR-URL: #19447 PR-URL: #12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: nodejs/node#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Enables the requireTwo option of our custom rule. PR-URL: nodejs/node#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos requested a review from Trott April 7, 2017 14:22

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Apr 7, 2017

cjihrig approved these changes Apr 7, 2017

View reviewed changes

mscdex added the assert Issues and PRs related to the assert subsystem. label Apr 7, 2017

jasongin reviewed Apr 7, 2017

View reviewed changes

Trott approved these changes Apr 7, 2017

View reviewed changes

targos force-pushed the assert-throws-requiretwo branch 2 times, most recently from ae3e425 to efdf030 Compare April 10, 2017 06:26

targos added 2 commits April 13, 2017 11:31

tools: enforce two arguments in assert.throws

a4b9c58

Enables the requireTwo option of our custom rule. PR-URL: nodejs#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos force-pushed the assert-throws-requiretwo branch from efdf030 to a4b9c58 Compare April 13, 2017 09:32

targos merged commit a4b9c58 into nodejs:master Apr 13, 2017

targos deleted the assert-throws-requiretwo branch April 13, 2017 10:32

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 18, 2017

This was referenced Jun 19, 2017

test: improve buffer.includes error verification tests #11203

Closed

test: extended test to makeCallback cb type check #12140

Closed

(v6.x backport) tools: enforce two arguments in assert.throws #13785

Closed

gibfahn added backported-to-v6.x and removed backport-requested-v6.x labels Jun 20, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: enforce two arguments in assert.throws #12270

tools: enforce two arguments in assert.throws #12270

targos commented Apr 7, 2017

jasongin Apr 7, 2017

jasongin Apr 7, 2017

targos Apr 7, 2017

Trott left a comment •

edited

targos commented Apr 10, 2017

gibfahn commented Jun 18, 2017

gibfahn commented Jun 18, 2017

targos commented Jun 19, 2017

tools: enforce two arguments in assert.throws #12270

tools: enforce two arguments in assert.throws #12270

Conversation

targos commented Apr 7, 2017

Checklist

Affected core subsystem(s)

jasongin Apr 7, 2017

Choose a reason for hiding this comment

jasongin Apr 7, 2017

Choose a reason for hiding this comment

targos Apr 7, 2017

Choose a reason for hiding this comment

Trott left a comment • edited

Choose a reason for hiding this comment

targos commented Apr 10, 2017

gibfahn commented Jun 18, 2017

gibfahn commented Jun 18, 2017

targos commented Jun 19, 2017

Trott left a comment •

edited