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: add rawMessage to error messages #19723

Closed
wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 1, 2018

The rawMessage provides the message with a guarantee not to include colors.

This was requested recently by @SimenB. I personally am not a fan of this. But I thought this way we can have a proper discussion about adding this property or not.

If a user sets the util.inspect 'colors' default options to true, assertion error messages are currently going to contain colors. The same applies to multiple errors coming from the assert strict mode.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Apr 1, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 1, 2018

@@ -386,6 +386,7 @@ class AssertionError extends Error {
this.actual = actual;
this.expected = expected;
this.operator = operator;
this.rawMessage = lazyInternalUtil().removeColors(this.message);
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this could be a prototype getter, for performance?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... I'm not sure about this. Once we get a bit further along with assigning the error codes to the errors, I'd like to pursue adding these kinds of non-standard properties off a new Error.prototype.info bucket rather than directly off the Error object. "Why?", you may ask? The reason is to give a consistent common location for non-standard Additional Stuff.

Copy link
Member

Choose a reason for hiding this comment

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

That is, to say, if this PR, for instance, added an info property with the actual, expected, operator and rawMessage properties to an Error.prototype.info and made the existing properties directly off the Error object into aliases that we could deprecate in 11.0.0, I'd be much happier with this.

Copy link
Member

Choose a reason for hiding this comment

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

That (having access to the properties you mention) would actually be even better for our use case in Jest of transparently supporting assert but making it look native to Jest, so huge 👍 to that!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell I guess the reason to have all additional properties on a single property is that it might conflict less with properties added else wise? I do not think that we should move all properties to a single property but we do lack documentation and more important out of my perspective: we should definitely export all our errors. That way the users could easily identify errors from Node.js core by just checking for being of the NodeError class. I think that is actually a better way to solve this problem than by moving everything to info.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to a prototype getter and setter.

@SimenB
Copy link
Member

SimenB commented Apr 2, 2018

Thanks for doing this! 🙂 Will make interop much cleaner

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 2, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

🎉

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 2, 2018

@SimenB I am not exactly sure in what way it will actually improve the interop. The messages could change due to being further improved and I strongly advice against relying on the error message by simply parsing it. If you want to verify input with a assert error message there is actually a good way to do that as described here: https://github.com/nodejs/node/pull/19724/files

@addaleax addaleax added assert Issues and PRs related to the assert subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 2, 2018
@SimenB
Copy link
Member

SimenB commented Apr 2, 2018

It'll improve it in that it allows us to not have to strip away redundant (as we have our own way of conveying the same thing) information. Some combination of strip-ansi and substring on the message could probably achieve the same thing, but this should be cleaner.

We have a way of separating the message from the stack if it's in both places which maybe will be wonky after this, but that should be easier to deal with thanks to the code property already present

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 2, 2018

I am still not sure I can follow. Let's say we have the following:

assert.strict.deepStrictEqual([1,1,1,1,1,2,2,2,2], [1,1,1,1,1,2,2,2])
// AssertionError [ERR_ASSERTION]: Input A expected to deepStrictEqual input B:
// + expected - actual ... Lines skipped

//   [
//     1,
// ...
//     1,
// -   2,
//     2,
// ...
//     2
//   ]

This would include colors. If the default util.inspect settings are set to contain colors, the numbers will all also be colored. But what can you do with this message? Don't you generate a complete new message in Jest? And what would happen if the message format is slightly changed again?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2018
@addaleax
Copy link
Member

addaleax commented Apr 2, 2018

@BridgeAR Side question: This is labeled author ready and the “docs included” checkbox is checked … Are we not documenting this because we don’t document other properties of these errors too, or am I missing something?

I don’t think it’s a terrible thing if it doesn’t happen in this PR, but as I understand it these properties are intended for public consumption, so we probably should document them eventually, right?

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 2, 2018

@addaleax in general: absolutely. So far we did not document any of those properties. So I went ahead and opened a PR for that: #19724. As soon as either of them lands, the other one needs a update.

@SimenB
Copy link
Member

SimenB commented Apr 4, 2018

Don't you generate a complete new message in Jest?

We keep the original message (but tweak the stack) in addition to a custom message. See https://github.com/facebook/jest/blob/1eb02fd41ad6840350a5b2c0636151e2da8bcd8b/integration-tests/__tests__/__snapshots__/failures.test.js.snap#L124-L512 for example of all of assert's functions within Jest

BridgeAR added 3 commits April 4, 2018 15:28
The `rawMessage` provides the message with a guarantee not to include
any colors.
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2018

I added the documentation after a rebase, since I just landed #19724.

I am still not convinced about adding this functionality. @SimenB I am going to look at the implementation in Jest as I believe there is probably a better way to directly handle this there.

Marking as blocked because of that.

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Apr 4, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2018

@SimenB
Copy link
Member

SimenB commented Apr 4, 2018

@SimenB I am going to look at the implementation in Jest as I believe there is probably a better way to directly handle this there.

Implementation lives here: https://github.com/facebook/jest/blob/1eb02fd41ad6840350a5b2c0636151e2da8bcd8b/packages/jest-jasmine2/src/assert_support.js

@refack
Copy link
Contributor

refack commented Apr 4, 2018

I'm not convinced this change adds real value either.

  1. util.inspect.defaultOptions.colors = false
  2. Cleaning ANSI codes is simple

So I'll -1

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Implementation seems 💯
Not convinced about value.

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2018

@refack please be aware that it is actually not a good idea to change the util.inspect.defaultOptions. I try to fix that at some point but that turned out to be more work than I originally anticipated. The reason that this is bad, is that it changes the behavior of all code. So a module could actually change that and the user is surprised with the output.

@refack
Copy link
Contributor

refack commented Apr 4, 2018

So a module could actually change that and the user is surprised with the output.

Agreed, but still a valid solution for root applications i.e. tools like jest

@BridgeAR
Copy link
Member Author

Ping @refack

@refack
Copy link
Contributor

refack commented Apr 12, 2018

IMHO rawMessage should be the only message. That is message shouldn't include anything but text.

So

function inspectValue(val) {
return util.inspect(
val,
{ compact: false, customInspect: false }
).split('\n');
}

Should be:

return util.inspect( 
     val, 
     { compact: false, customInspect: false, colors: false } 
   ).split('\n');

@addaleax
Copy link
Member

How about going with @refack’s suggestion, and then, for color support, add a custom inspect override for assertion errors that could provide the right colorization, if that subsequent inspect call requests it?

@BridgeAR
Copy link
Member Author

@addaleax I am not sure I understand your implementation suggestion correct. Can you please elaborate?

Besides that: I personally believe using colors is a good idea and we should in general try to use more colors in core. It improves the readability a lot out of my perspective. Right now the error message will only contain colors if the TTY supports colors and colors did not get deactivated by the user or the user (or a module) explicitly changed the util.inspect.defaultOptions to always use colors.

@addaleax
Copy link
Member

@BridgeAR The idea is:

  • No color output in .message
  • Still, generate a colorized version of the message, and store it on the error object in some way
  • Add a [util.inspect.custom](depth, ctx) override on the assertion error which that decides based on ctx.colors

That way we don’t have to deal with color support checking while creating the error, where we can’t yet know how it’s going to end up being used.

@SimenB
Copy link
Member

SimenB commented Apr 12, 2018

FWIW, the thing I want is no colors and no diff, and colors are easy to strip out (https://github.com/chalk/strip-ansi). But the diff forces us to manually split up .message to remove it, so just removing colors from .message doesn't really solve Jest's use case at all.

@BridgeAR
Copy link
Member Author

@SimenB

FWIW, the thing I want is no colors and no diff, and colors are easy to strip out (https://github.com/chalk/strip-ansi). But the diff forces us to manually split up .message to remove it, so just removing colors from .message doesn't really solve Jest's use case at all

That sounds like you want to know about the actual and expected properties from the AssertionError. There are more properties that are currently documented on master that will probably help you out.

@addaleax thanks for explaining your idea in more detail. I am going to give this another thought but right now I still think it would be nicer to add colors as a default even if colors is not explicitly activated.

@SimenB
Copy link
Member

SimenB commented Apr 12, 2018

We want actual and expected yes (and it's great that they're there), but we also want the message provided instead of providing our own. Especially since assert supports a third message argument (e.g. assert.equal('hello', 'world', 'string should match')).

So the rawMessage property suggested here (which allows us to keep using assert the way we currently use it) is perfect.

@addaleax
Copy link
Member

@addaleax thanks for explaining your idea in more detail. I am going to give this another thought but right now I still think it would be nicer to add colors as a default even if colors is not explicitly activated.

Can you explain why you think that?

@addaleax
Copy link
Member

@BridgeAR I played around with my suggestion but noticed that we don’t actually use util.inspect() for printing errors in most situations :/

I still think it would be nicer if we didn’t add color support based on feature detection for stdout if we don’t know whether it’ll end up there, but I think given the current constraints your implementation makes sense, so feel free to ignore my comments above :)

@BridgeAR
Copy link
Member Author

@addaleax

Can you explain why you think that?

Most users rely on default values and those are often very old and not optimal. Some times that is for historical reasons but most often just because originally there was no better solution. Now that there is a rough color detection, we should start relying on that so we can provide colors when we determine that the user is capable of handling those. The user can still deactivate those in general.

@refack please take another look. I would like to go ahead and land this if you do not feel strongly against it.

@BridgeAR
Copy link
Member Author

@refack PTAL

@SimenB
Copy link
Member

SimenB commented Apr 23, 2018

The changed error message (diff and the wording around the operator) is the only thing breaking Jest's test suite using 10-rc.1. It'd be great to land a solution for this one way or the other. The changed wording we can work around in our test, but the double diff looks weird, so that'll require a code change in Jest. rawMessage would be perfect, but we can probably slice up the error message manually as well if needed.

To be clear, for consumers of Jest this is not really an issue (double diff won't break anybody, it just takes up more space in the terminal - although we'll want to remove it in the long run), it's just Jest's own assertions that the interop works which break.

(This message looka kinda passive-aggresive even to my own eyes. That's not my intention at all ❤️).

EDIT: Most of the discussion in this PR has been about the colors - those aren't really an issue for us (we can use strip-ansi). It's the diff that I want to avoid 🙂

@BridgeAR
Copy link
Member Author

@SimenB I am not sure what you mean with "double diff". I guess you mean Jest creates a diff and Node.js creates a diff as well? The rawMessage as it is in here would contain a diff as well and would not change anything about that. It would only make sure there are no color codes in there.

I still recommend to just rely on the properties of the thrown error instead of relying on the message. That seems to be best for Jest. To be more independent in tests see e.g., https://github.com/eslint/eslint/pull/10182/files.

@SimenB
Copy link
Member

SimenB commented Apr 23, 2018

@SimenB I am not sure what you mean with "double diff". I guess you mean Jest creates a diff and Node.js creates a diff as well?

Correct. See this screengrab using rc.1:

image

The flipped around + and - for instance is not really acceptable - it's super-confusing for consumers (not to start a discussion about which notation is most correct).

The rawMessage as it is in here would contain a diff as well and would not change anything about that.

Oh, I though rawMessage would basically be the message in node <=9 (I haven't looked at the actual diff here). If it's just colors then it doesn't really do anything for Jest's use case at all.

I still recommend to just rely on the properties of the thrown error instead of relying on the message.

While setting up some code examples to show what I mean, I found the property generatedMessage which is false when message is provided by the user. That means we can actually just remove message if generatedMessage === true. That should solve our problem! 🎉

generatedMessage is not documented here: https://nodejs.org/dist/latest-v8.x/docs/api/assert.html, it probably should be.

Let me make a quick test to see if we can just move to that, and I'll report back.

EDIT: Yeah, that's enough for Jest. Just doing a e.stack.replace(e.message, '') removes the diff from the stack as well, at which point we're good!

So, at least for Jest's use case, this is not needed 🙂

@BridgeAR
Copy link
Member Author

@SimenB

The flipped around + and - for instance is not really acceptable

That is a very valid point. I am going to check what is most commonly used pattern and improve that if it there is a "standard" to it that is actually flipped to the current implementation.

generatedMessage is not documented here

The docs landed recently as I pointed out above (#19723 (comment), #19723 (comment) & #19723 (comment)).

That means we can actually just remove message if generatedMessage === true. That should solve our problem!

🎉 🎈

If it's just colors then it doesn't really do anything for Jest's use case at all.

Yes, I guess then there is no real reason to actually land this. I am going to close the PR therefore.

Something in general: Thanks a lot for the constructive feedback. There were some great findings and I am going to try to keep an closer eye on Jest.

@BridgeAR BridgeAR closed this Apr 23, 2018
@SimenB
Copy link
Member

SimenB commented Apr 23, 2018

Thank you for the patience!

For posterity, PR in Jest for node 10 support: jestjs/jest#6055

EDIT: Also working on getting Jest into CITGM, should highlight potential issues at least 🙂 nodejs/citgm#560

@refack refack added the stalled Issues and PRs that are stalled. label Nov 6, 2018
refack added a commit to refack/node that referenced this pull request Nov 7, 2018
PR-URL: nodejs#24204
Fixes: nodejs#24193
Refs: nodejs#19723
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24204
Fixes: #24193
Refs: #19723
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24204
Fixes: nodejs#24193
Refs: nodejs#19723
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #24204
Fixes: #24193
Refs: #19723
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #24204
Fixes: #24193
Refs: #19723
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
PR-URL: #24204
Fixes: #24193
Refs: #19723
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR BridgeAR deleted the add-raw-message branch April 1, 2019 23:40
@BridgeAR BridgeAR restored the add-raw-message branch April 1, 2019 23:40
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants