GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — assert: respect assert.doesNotThrow message. by diversario · Pull Request #2407 · nodejs/node

Conversation

@diversario

@diversario diversario commented Aug 17, 2015

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brendanashworth brendanashworth added the assert Issues and PRs related to the assert subsystem. label Aug 17, 2015
@brendanashworth

brendanashworth commented Aug 26, 2015

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@diversario

diversario commented Aug 26, 2015

Copy link
Copy Markdown
Author

Good point, I'll update them.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg
rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55
@jasnell jasnell added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Nov 16, 2015
@jasnell

jasnell commented Nov 16, 2015

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@diversario

diversario commented Nov 16, 2015

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Nov 16, 2015

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Nov 16, 2015

Copy link
Copy Markdown
Contributor

I thought assert was being locked down?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Nov 16, 2015

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Nov 16, 2015

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Nov 16, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Nov 16, 2015

Copy link
Copy Markdown
Contributor

I'm cool with fixing actual bugs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rvagg
rvagg reviewed Nov 17, 2015
View reviewed changes
Comment thread lib/assert.js Outdated

@rvagg rvagg Nov 17, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@diversario diversario Apr 2, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rvagg

rvagg commented Nov 17, 2015

Copy link
Copy Markdown
Member

I'm fine with fixing bugs like this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00
@jasnell

jasnell commented Apr 2, 2016

Copy link
Copy Markdown
Member

@diversario ... still interested in this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@diversario

diversario commented Apr 2, 2016

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 4, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@diversario

diversario commented Apr 10, 2016

Copy link
Copy Markdown
Author

Pushed an update.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Apr 11, 2016
View reviewed changes
Comment thread doc/api/assert.markdown Outdated

@jasnell jasnell Apr 11, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Apr 11, 2016

Copy link
Copy Markdown
Member

Generally LGTM with a few nits
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@diversario

diversario commented Apr 13, 2016

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 15, 2016

Copy link
Copy Markdown
Member

Can you rebase and update?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ilya Shaisultanov and others added 2 commits April 15, 2016 11:31
Ilya Shaisultanov @diversario
assert: respect assert.doesNotThrow message.
83ec4b5 
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`.
@diversario
assert: clean up conditional for _throws, fix docs.
d741140 
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
Ilya Shaisultanov @jasnell
assert: respect assert.doesNotThrow message.
c1d82ac 
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 <[email protected]>
@jasnell jasnell removed the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Apr 18, 2016
@jasnell

jasnell commented Apr 18, 2016

Copy link
Copy Markdown
Member

Landed in c1d82ac
diversario reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Apr 18, 2016
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
Ilya Shaisultanov Myles Borins
assert: respect assert.doesNotThrow message.
a026b8a 
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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Ilya Shaisultanov Myles Borins
assert: respect assert.doesNotThrow message.
fde1348 
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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Ilya Shaisultanov Myles Borins
assert: respect assert.doesNotThrow message.
690192c 
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 <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
V5.10.2 proposal #6311
Closed
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Ilya Shaisultanov Myles Borins
assert: respect assert.doesNotThrow message.
14fcb1d 
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 <[email protected]>
This was referenced Apr 21, 2016
V5.11.0 proposal #6321
Closed
V5.11.0 proposal #6322
Closed
Audit commits not found on v4.x #5647
Closed
@MylesBorins

MylesBorins commented Apr 21, 2016

Copy link
Copy Markdown
Contributor

@jasnell lts?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 21, 2016

Copy link
Copy Markdown
Member

Yes
On Apr 20, 2016 11:50 PM, "Myles Borins" [email protected] 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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Ilya Shaisultanov @MylesBorins
assert: respect assert.doesNotThrow message.
85cb9bb 
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 <[email protected]>
@MylesBorins MylesBorins added the lts-watch-v4.x label Apr 21, 2016
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
Ilya Shaisultanov @joelostrowski
assert: respect assert.doesNotThrow message.
865bcfd 
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 <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Ilya Shaisultanov @jasnell
assert: respect assert.doesNotThrow message.
8b8746f 
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 <[email protected]>
@ChALkeR ChALkeR mentioned this pull request May 2, 2016
API stability level «Locked» is inaccurate #6528
Closed
MylesBorins pushed a commit that referenced this pull request May 3, 2016
Ilya Shaisultanov Myles Borins
assert: respect assert.doesNotThrow message.
a0223c6 
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 <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels May 3, 2016
MylesBorins pushed a commit that referenced this pull request May 6, 2016
Ilya Shaisultanov Myles Borins
assert: respect assert.doesNotThrow message.
c2eff54 
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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
Ilya Shaisultanov Myles Borins
assert: respect assert.doesNotThrow message.
59a977d 
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 <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
v4.4.5 proposal #6824
Merged
LinusU
LinusU reviewed Sep 27, 2016
View reviewed changes
Comment thread lib/assert.js
}

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

@LinusU LinusU Sep 27, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
LinusU reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments
+1 more reviewer
@LinusU LinusU LinusU left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@diversario @brendanashworth @jasnell @cjihrig @mscdex @rvagg @MylesBorins @LinusU @BridgeAR
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.