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

tools: enforce two arguments in assert.throws #12270

Merged
merged 2 commits into from
Apr 13, 2017

Conversation

targos
Copy link
Member

@targos targos commented Apr 7, 2017

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

@targos targos requested a review from Trott April 7, 2017 14:22
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Apr 7, 2017
@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Apr 7, 2017
assert.throws(() => { test_object.readonlyValue = 3; });
const expected = new RegExp(
'^TypeError: Cannot assign to read only property \'readonlyValue\' of ' +
'object \'#<MyObject>\'$'
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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: /);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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 (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 targos force-pushed the assert-throws-requiretwo branch 2 times, most recently from ae3e425 to efdf030 Compare April 10, 2017 06:26
@targos
Copy link
Member Author

targos commented Apr 10, 2017

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>
@targos targos merged commit a4b9c58 into nodejs:master Apr 13, 2017
@targos targos deleted the assert-throws-requiretwo branch April 13, 2017 10:32
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

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
Copy link
Member

gibfahn commented Jun 18, 2017

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

@targos
Copy link
Member Author

targos commented Jun 19, 2017

Backport PR: #13785

gibfahn pushed a commit that referenced this pull request Jun 19, 2017
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>
gibfahn pushed a commit that referenced this pull request Jun 19, 2017
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>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants