assert.doesNotThrow, show error message #12167

krydos · 2017-04-02T17:46:00Z

As mentioned in #12079 doesNotThrow doesn't show actual error message but shows Got unwanted error which is not really helpful. As @mscdex said it will be better to show actual error message so this is what I have done here. parallel/test-assert.js also was changed a bit.

I hope it will be helpful.

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

Affected core subsystem(s)

assert

vsemozhetbyt · 2017-04-02T18:15:27Z

~~Commit messages nits (ref):~~

~~Use lower case imperative verb after subsystem prefix (something like assert: show thrown message in doesNotThrow() or assert: adapt ...).~~
~~Keep commit title length within 50 characters (currently 59 in the second commit).~~

vsemozhetbyt · 2017-04-02T18:16:40Z

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

krydos · 2017-04-02T19:20:33Z

@vsemozhetbyt thank you for the feedback. I changed the commit text plus squashed two commits.

vsemozhetbyt · 2017-04-02T19:20:34Z

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

vsemozhetbyt · 2017-04-02T19:31:52Z

Is this semver-major? If so, beware v.8 rc deadline will be closing in 2 days.

krydos · 2017-04-02T20:04:24Z

seems like test/arm tests failed...
the error seems for me like something related to Jenkins - https://gist.github.com/KryDos/927139a77ec9fb2618696b385d6450e2

Could someone please help me to solve it?

Trott · 2017-04-03T02:58:56Z

@krydos Given the nature of this change, I think we can ignore the Jenkins issue as unrelated. If someone really wants to, they can re-run CI.

Labeling semver-major out of abundance of caution.

jasnell · 2017-04-03T15:31:56Z

The Got unwanted exception bit is still useful to explain the situation. Not entirely happy with dropping that.

krydos · 2017-04-03T15:36:46Z

@jasnell what if I'll change it to Got unwanted exception (the error message)...? Probably it will be even better.

EDIT:
This was strange suggestion since "the error message" is actually not the message only but also concatenated expected message. (e.g thrown error: expected). So it will look very very strange.

mscdex · 2017-04-03T16:49:13Z

Well, I don't think just including the original error message is useful enough. The stack trace is usually more helpful, but if we use the original stack trace that may/may not be confusing with the modified exception message.

Perhaps if we separated with a newline that might be clearer? For example:

err.message = 'Got unwanted exception:\n' + err.message;
throw err;

Doing so would mean we would need to tweak fail()/AssertionError to use the original stack trace.

krydos · 2017-04-03T17:57:35Z

@mscdex excuse me, but I didn't really understand why do we need to change how fail or AssertionError work.

As you and @jasnell noticed, and I agree with, that error.message without Got unwanted exception may be confusing.

I also didn't understand the meaning of original stack trace just because I do not see that stack trace is getting modified somehow during doesNotThrow execution. So as for me stack trace is always original. Could you please help me to understand this? 😊

gibfahn · 2017-04-03T18:16:58Z

CI 3: https://ci.nodejs.org/job/node-test-commit/8845/ (all green)

mscdex · 2017-04-03T18:29:57Z

@krydos Nevermind, I suppose it works fine for this situation because it's synchronous.

krydos · 2017-04-04T08:37:55Z

In my last commit I returned Got unwanted exception since it is something that can help to understand what just happened. I hope it's ok.

EDIT:
just to help you understand how the output looks like, here is - https://gist.github.com/KryDos/5d528c4bfa12627822926ff99976056f

jasnell · 2017-04-04T16:23:23Z

test/parallel/test-assert.js

@@ -406,7 +406,7 @@ assert.doesNotThrow(function() { assert.ifError(); });

 assert.throws(() => {
  assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
-}, /Got unwanted exception: user message/,
+}, /Got unwanted exception:\ntest: user message/,
              'a.doesNotThrow ignores user message');


The indenting is off here

@jasnell a.doesNotThrow ignores... should be aligned with /Got unwanted exception.... Do I understand it correctly?

...probably not since jslint throws error about it.

@jasnell unfortunately I didn't really understand where indenting was off :( I hope it was fixed with new commit... but probably not.

joyeecheung · 2017-04-04T17:17:30Z

lib/assert.js

-    fail(actual, expected, 'Got unwanted exception' + message);
+    fail(actual, expected,
+         'Got unwanted exception:\n' +
+         actual.message + message);


Why is it 'Got unwanted exception:\n' + actual.message + message instead of 'Got unwanted exception: ' + message + '\n' + actual.message (more similar to what it used to be)? This way it would be:

Got unwanted exception: explain why it is unwanted. Message of the unwanted exception.

Looks really good.
But probably something like that

'Got unwanted exception' + message + '\n' + actual.message

(without : after exception) since message will have it (if message exists). Otherwise message will have just . (dot).

So, if message wasn't passed it will look like:

Got unwanted exception. Message of the unwanted exception.

I really like your idea.

@joyeecheung just pushed the changes you suggested. Thank you!

joyeecheung · 2017-04-04T17:31:37Z

test/parallel/test-assert.js

@@ -406,7 +406,7 @@ assert.doesNotThrow(function() { assert.ifError(); });

 assert.throws(() => {
  assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
-}, /Got unwanted exception: user message/,
+}, /Got unwanted exception:\ntest: user message/,
              'a.doesNotThrow ignores user message');


This message is no longer accurate...this test fails if a.doesNotThrow ignores user message or if it ignores the message of the unwanted exception..

@joyeecheung completely agree with your here. I added another test case that tests unwanted exception message. I hope it is ok.

krydos · 2017-04-08T03:22:39Z

@jasnell ping.
Could you please take a look and leave your feedback.

addaleax · 2017-04-14T22:01:19Z

test/parallel/test-assert.js

+assert.throws(() => {
+  assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
+}, /Got unwanted exception: user message\ntest/,
+              'a.doesNotThrow ignores message of the unwanted exception');


I think what @jasnell mean is that the opening ' should be directly below the opening /. (Or just drop the extra third argument here, I think I would prefer that.)

Thank you @addaleax ... yes, that's what I meant.

(I missed the earlier ping)

I actually tried to align ' with / but I was unable to pass linter in this case. It was saying I have wrong indentation.

Since it is ok to remove third argument this is exactly what I did.

Yeah I've seen that happen before, it wants to align with the function call's opening (.

Trott · 2017-05-10T06:14:13Z

test/parallel/test-assert.js

@@ -409,6 +409,10 @@ assert.throws(() => {
 }, /Got unwanted exception: user message/,
              'a.doesNotThrow ignores user message');

+assert.throws(() => {
+  assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
+}, /Got unwanted exception: user message\ntest/);


Can we add a test to confirm we don't get unhelpful strings (like "undefined") if the user leaves out the optional message argument?

Also, probably a good idea to include a test case for if a primitive is thrown rather than a proper Error. Like, maybe throw "big problem". You'll probably need to disable an eslint rule with a comment because we lint for throw being used with an unexpected type like that.

@Trott thank you for recommendations. I've added one more test to check if no user message provided.

As for second comment about throw primitive... it really displays undefined as error text. It goes to another branch and do not entering in the if that I touched in PR. It happens probably because util.isError is returning false since string isn't good Error object.
I'm still thinking about good way to fix it...

refack · 2017-07-09T18:34:00Z

Sanity of rebase: https://ci.nodejs.org/job/node-test-commit-linuxone/7173/ 🔴

assert.doesNotThrow() should show actual error message instead of "Got unwanted exception" which is not really helpful. PR-URL: nodejs#12167 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-07-09T18:48:22Z

@krydos Needs a rebase

refack · 2017-07-09T18:49:38Z

lib/assert.js

-    fail(actual, expected, 'Got unwanted exception' + message);
+    fail(actual, expected,
+         'Got unwanted exception' +
+         message + '\n' + actual.message);


needs a 4th argument caller should be either assert.throws or assert.doesNotThrow

refack · 2017-07-09T18:52:10Z

test/parallel/test-assert.js

@@ -409,6 +409,14 @@ assert.throws(() => {
 }, /Got unwanted exception: user message/,
              'a.doesNotThrow ignores user message');

+assert.throws(() => {


IMHO the check should be using try / catch otherwise we get into a weird situation where we use the function to test itself and Alex Turing will get mad, and say 🛑

@refack thanks for the feedback. throws calls innerThrows internally and doesNotThrow calls innerThrows internally as well. That's why we use function to test itself? Did I understand you correctly? Do we need just usual try / catch instead of assert.throws right?

refack · 2017-07-09T18:54:17Z

While trying to test if I rebased this correctly I got:

not ok 53 parallel/test-assert
  ---
  duration_ms: 0.90
  severity: fail
  stack: |-
    assert.js:544
          throw actual;
          ^
    
    AssertionError [ERR_ASSERTION]: Got unwanted exception: user message
    [object Object]
        at innerThrows (assert.js:549:7)
        at Function.doesNotThrow (assert.js:566:3)
        at assert.throws (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-assert.js:472:10)
        at tryBlock (assert.js:514:5)
        at innerThrows (assert.js:533:18)
        at Function.throws (assert.js:562:3)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-assert.js:471:8)
        at Module._compile (module.js:569:30)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:503:32)
  ...

Who throw the exception? The function we are testing, or the function we are using to assert the the result is as expected? 💫

krydos · 2017-07-11T18:39:01Z

I'm currently not sure why actual is [object Object] right now... It was differently before :)
Let me trace it to understand why it's like that and I'll come back with more info

krydos · 2017-07-15T06:26:14Z

Ok,
[object Object] is probably fine, because our error thrower accepts {} now instead of test string as it was previously.
I found some code that test against [object Object] as well. So based on this I made the same check.

I also could avoid using thrower function in my tests and use my own thrower. I think it's not bad... but I tried to keep my tests similar to others. Please update me if "custom thrower" will be better in this case.

p.s.
when I say "custom thrower" I mean the same thrower we have but with string passed into constructor instead of empty object {}.

joyeecheung · 2017-07-15T06:56:27Z

I think using the current thrower is fine, we could add a comment for why it's [Object object] for clarity but since this is validated in other tests as well we don't really have to do that ¯\(ツ)/¯

+1 to @refack 's idea of using try-catch instead of throws to test doesNotThrow since we are calling innerThrows in both functions

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

assert.doesNotThrow() should show actual error message instead of "Got unwanted exception" which is not really helpful.

krydos · 2017-07-15T17:38:25Z

@joyeecheung @refack thanks for explanation and suggestion to use try / catch instead of assert.throws to test assert.doesNotThrow. I've pushed new changes.

joyeecheung · 2017-07-16T03:43:53Z

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

refack · 2017-07-19T19:51:31Z

Quick CI just for freshness: https://ci.nodejs.org/job/node-test-commit-linuxone/7434/

assert.doesNotThrow() should show actual error message instead of "Got unwanted exception" which is not really helpful. PR-URL: nodejs#12167 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-07-19T20:02:40Z

Landed in c53db1e
🎉🎉🎉

krydos · 2017-07-20T07:06:52Z

Thanks everyone for help and feedback 🙌

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Apr 2, 2017

krydos changed the title ~~Better does not throw~~ assert.doesNotThrow, show error message Apr 2, 2017

krydos force-pushed the better-does-not-throw branch from 7b667a0 to 36d3cb3 Compare April 2, 2017 19:16

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 3, 2017

addaleax approved these changes Apr 3, 2017

View reviewed changes

hiroppy approved these changes Apr 3, 2017

View reviewed changes

krydos force-pushed the better-does-not-throw branch from 36d3cb3 to 166a4f9 Compare April 4, 2017 08:35

jasnell requested changes Apr 4, 2017

View reviewed changes

joyeecheung reviewed Apr 4, 2017

View reviewed changes

krydos force-pushed the better-does-not-throw branch from 166a4f9 to db9d871 Compare April 6, 2017 08:24

joyeecheung approved these changes Apr 6, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

addaleax reviewed Apr 14, 2017

View reviewed changes

krydos force-pushed the better-does-not-throw branch from db9d871 to 61ded4e Compare April 15, 2017 05:18

Trott reviewed May 10, 2017

View reviewed changes

krydos force-pushed the better-does-not-throw branch from 61ded4e to 1c9f79b Compare May 11, 2017 19:42

refack self-assigned this May 28, 2017

refack approved these changes Jul 9, 2017

View reviewed changes

refack suggested changes Jul 9, 2017

View reviewed changes

krydos force-pushed the better-does-not-throw branch from 1c9f79b to d081803 Compare July 15, 2017 06:19

assert: show thrown message in doesNotThrow()

d023feb

assert.doesNotThrow() should show actual error message instead of "Got unwanted exception" which is not really helpful.

krydos force-pushed the better-does-not-throw branch from d081803 to d023feb Compare July 15, 2017 17:36

joyeecheung approved these changes Jul 16, 2017

View reviewed changes

refack approved these changes Jul 19, 2017

View reviewed changes

refack closed this Jul 19, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

bradennapier mentioned this pull request Nov 3, 2017

[REQUEST]: Document Breaking Changes in 9.0 #16713

Closed

refack removed their assignment Oct 20, 2018

SimenB mentioned this pull request Dec 2, 2018

[v8.x] backport some assert commits #23223

Closed

4 tasks

assert.doesNotThrow, show error message #12167

assert.doesNotThrow, show error message #12167

Conversation

krydos commented Apr 2, 2017

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Apr 2, 2017 • edited Loading

vsemozhetbyt commented Apr 2, 2017

krydos commented Apr 2, 2017

vsemozhetbyt commented Apr 2, 2017

vsemozhetbyt commented Apr 2, 2017 • edited Loading

krydos commented Apr 2, 2017

Trott commented Apr 3, 2017

jasnell commented Apr 3, 2017

krydos commented Apr 3, 2017 • edited Loading

mscdex commented Apr 3, 2017 • edited Loading

krydos commented Apr 3, 2017

gibfahn commented Apr 3, 2017

mscdex commented Apr 3, 2017

krydos commented Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krydos commented Apr 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 9, 2017 • edited Loading

refack commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 9, 2017 • edited Loading

krydos commented Jul 11, 2017

krydos commented Jul 15, 2017

joyeecheung commented Jul 15, 2017 • edited Loading

krydos commented Jul 15, 2017

joyeecheung commented Jul 16, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017

krydos commented Jul 20, 2017

vsemozhetbyt commented Apr 2, 2017 •

edited

Loading

vsemozhetbyt commented Apr 2, 2017 •

edited

Loading

krydos commented Apr 3, 2017 •

edited

Loading

mscdex commented Apr 3, 2017 •

edited

Loading

krydos commented Apr 4, 2017 •

edited

Loading

joyeecheung Apr 4, 2017 •

edited

Loading

Trott May 10, 2017 •

edited

Loading

refack commented Jul 9, 2017 •

edited

Loading

refack commented Jul 9, 2017 •

edited

Loading

joyeecheung commented Jul 15, 2017 •

edited

Loading