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

assert: respect assert.doesNotThrow message. #2407

Conversation

diversario
Copy link

Addresses #2385.
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if actual value is an error use
util.isError instead of instanceof.

@brendanashworth brendanashworth added the assert Issues and PRs related to the assert subsystem. label Aug 17, 2015
@brendanashworth
Copy link
Contributor

Would you mind fixing up the docs for throws and doesNotThrow? They lack some information:

The docs for throws() don't mention what message really does, so you're forced to look in the code. The docs for doesNotThrow() are even worse.

@diversario
Copy link
Author

Good point, I'll update them.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@diversario ... is this still something you'd like to pursue?

@diversario
Copy link
Author

I would; I actually asked a question here about it because I'm not quite sure how to proceed.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@nodejs/ctc ... any thoughts on this one?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2015

I thought assert was being locked down?

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@cjihrig good point... does that only include API changes or also behavioral fixes like this one?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2015

I would have to go back and look at the CTC meeting notes, but I thought the idea was to only take patches that were relevant to Node's tests.

@mscdex
Copy link
Contributor

mscdex commented Nov 16, 2015

I thought we would still accept actual bug fixes just not new features?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2015

I'm cool with fixing actual bugs.

if (!shouldThrow &&
actual instanceof Error &&
typeof message === 'string' &&
expectedException(actual, expected) ||
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to see a more liberal use of parens to make it explicit what the evaluation order is that you're using here, mixing && and || without parens makes this very difficult to parse. The number of conditions in here adds to that difficulty and it'd be nice to reduce it with some compacting variables—although not essential, just would be nice to make this less terse.

Copy link
Author

Choose a reason for hiding this comment

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

Breaking out the || part into a var sounds good.

@rvagg
Copy link
Member

rvagg commented Nov 17, 2015

I'm fine with fixing bugs like this

@jasnell
Copy link
Member

jasnell commented Apr 2, 2016

@diversario ... still interested in this?

@diversario
Copy link
Author

Oof... Is this still relevant? It's been so long. If it's still OK to go to master and there's no further PR feedback I can make the change @rvagg suggested and push up.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

@diversario ... yeah, still relevant. Just needs updated per @rvagg's comments and a couple of review sign offs.

@diversario
Copy link
Author

Pushed an update.

@@ -51,7 +51,7 @@ Tests for deep inequality. Opposite of `assert.deepStrictEqual`.
## assert.throws(block[, error][, message])

Expects `block` to throw an error. `error` can be constructor, `RegExp` or
validation function.
validation function. When assertion fails, `message` will be included in the `AssertionError` message.
Copy link
Member

Choose a reason for hiding this comment

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

Can you line wrap these so each line is <= 80

@jasnell
Copy link
Member

jasnell commented Apr 11, 2016

Generally LGTM with a few nits

@diversario
Copy link
Author

Thanks for the feedback! I tried the ES6 syntax but I get a syntax error; I think this branch is off an old master that didn't have support for that yet.

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

Can you rebase and update?

Ilya Shaisultanov and others added 2 commits April 15, 2016 11:31
Addresses nodejs#2385.
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.
Addresses nodejs#2407.
Break out conditional into named variables for readability.
Explain the purpose of `message` in throws/doesNotThrow assertions.
jasnell pushed a commit that referenced this pull request Apr 18, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell removed the stalled Issues and PRs that are stalled. label Apr 18, 2016
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Landed in c1d82ac

@jasnell jasnell closed this Apr 18, 2016
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@jasnell lts?

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

Yes
On Apr 20, 2016 11:50 PM, "Myles Borins" notifications@github.com wrote:

@jasnell https://github.com/jasnell lts?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2407 (comment)

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: nodejs#2385
PR-URL: nodejs#2407
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 3, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 6, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
@@ -330,7 +330,14 @@ function _throws(shouldThrow, block, expected, message) {
fail(actual, expected, 'Missing expected exception' + message);
}

if (!shouldThrow && expectedException(actual, expected)) {
const userProvidedMessage = typeof message === 'string';
Copy link
Contributor

Choose a reason for hiding this comment

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

A year late to the party here but won't this always evaluate to true since message is forced into a string 7 lines up (line 326)?

ping @diversario, @jasnell

Copy link
Member

Choose a reason for hiding this comment

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

Just because I stumbled upon this comment right now: this is fixed in newer versions.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants