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: test-assert.js add 2nd argument to throws #11061

Closed
wants to merge 1 commit into from
Closed

test: test-assert.js add 2nd argument to throws #11061

wants to merge 1 commit into from

Conversation

Marlena
Copy link
Contributor

@Marlena Marlena commented Jan 29, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 29, 2017
cjihrig
cjihrig reviewed Jan 29, 2017
View reviewed changes
test/parallel/test-assert.js
@@ -102,9 +102,11 @@ assert.throws(makeBlock(a.deepEqual, /a/igm, /a/im),
/^AssertionError: \/a\/gim deepEqual \/a\/im$/);

{
const re1 = /a/;
const re1 = /a/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add the g?

@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Jan 29, 2017
@Marlena
Copy link
Contributor Author

Marlena commented Jan 29, 2017 via email

cjihrig
cjihrig approved these changes Jan 29, 2017
View reviewed changes
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.

Thanks for the link. It looks like the test can technically pass without adding the g flag, but I agree it should be included.

@Trott
Copy link
Member

Trott commented Jan 29, 2017 •
edited
Loading

Nit: You don't need to make the change I'm about to suggest, but the assert.throws() on line 241 (in the current version of the test, line 243 in the version of the file in this PR) is very similar. I think it's identical except that it is testing deepStrictEqual() instead of deepEqual(). Want to add the RegExp and the /g there as well?

@jasnell
Copy link
Member

jasnell commented Jan 30, 2017

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

@Trott
Copy link
Member

Trott commented Jan 31, 2017

Only CI failure is a known-flaky documented at #11041. So CI looks good.

Trott pushed a commit to Trott/io.js that referenced this pull request Jan 31, 2017
@Marlena @Trott
test: add 2nd argument to throws in test-assert
262400f 
PR-URL: nodejs#11061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member

Trott commented Jan 31, 2017

Landed in 262400f.
Thanks for the contribution! 🎉

@Trott Trott closed this Jan 31, 2017
@Marlena
Copy link
Contributor Author

Marlena commented Jan 31, 2017 via email

evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@Marlena @evanlucas
test: add 2nd argument to throws in test-assert
fe80bd9 
PR-URL: #11061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@Marlena @jasnell
test: add 2nd argument to throws in test-assert
22f7e23 
PR-URL: #11061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@Marlena @jasnell
test: add 2nd argument to throws in test-assert
1eeb38d 
PR-URL: #11061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@Marlena @MylesBorins
test: add 2nd argument to throws in test-assert
8cce295 
PR-URL: #11061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@Marlena @MylesBorins
test: add 2nd argument to throws in test-assert
c1f45ec 
PR-URL: #11061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
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

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Marlena @Trott @jasnell @addaleax @lpinca @cjihrig @mscdex @nodejs-github-bot