test: improve common.expectsError #19797

BridgeAR · 2018-04-04T15:05:34Z

The output is now improved by showing most properties all at once.
Besides that this adds a warning to use assert.throws instead
due to a better output.

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

BridgeAR · 2018-04-04T15:06:18Z

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

jasnell · 2018-04-04T18:50:26Z

test/common/index.js

+    console.error(
+      'Please use `assert.throws()` instead of `common.expectsError()`. ' +
+      'To verify the error type, compare the error name.'
+    );


I'm actually -1 on this bit. Adding common.expectsError() was deliberate to differentiate between checking ordinary JavaScript throws and uses of internal/errors. If the output for assert.throws() is better, then the better thing to do is improve the output for common.expectsError() accordingly.

I am -1 as well. It's fairly easy to change common.expectsError() to meet whatever we need, but changing assert.throws comes with much more implications - IIRC assert should've not been exposed to users for the same reason, and it used to have a locked stability index. It has been changed to stable because we were getting rid of the locked index altogether but I don't think we have actually discussed that changes to the public assert are encouraged.

I am not sure I can follow. I can not remember the original implementation but the overloading that triggers assert.throws is implemented by me and that was done because assert.throws did not have the same capabilities as common.expectsError at that point of time. Since then I added support for objects in assert.throws and improved the error output significantly.

Right now there is still a valid use case for common.expectsError: in case it is used as callback function. But in all other cases I believe it is better to use assert.throws due to multiple reasons and it is also much more intuitive for new users. And the warning is there for exactly that use case: use assert.throws directly if possible.

@joyeecheung as far as I know there were multiple discussions about this (in a general term). I believe code should never be "locked" as it is important to further improve things. It was the reason that the module became a wart in Node.js because people used it and stumbled upon deficiencies of it. Now the user experience is actually quite nice and people can use it out of my perspective. Even though there are of course more things that can be improved.

As far as I see it, you are against adding this comment. Are you against the PR in general? I have no strong feelings besides believing that we should use assert.throws and not common.expectsError but I will go with what ever is the opinion of others here.

@BridgeAR I love the work you did on assert.throws() when I used them in our own tests, but I am still skeptical about actually changing the public assert significantly, given that it used to be locked - we did promise our users that it won't receive such a drastic change but we are now breaking the promise. Had I notice the changes to assert I would suggest to keep the changes in our internal test utilities for a while and have some discussion about this before actually exposing them to users, sorry.

BTW this is thread about removing the locked stability index. #11200

@joyeecheung I am not aware of any bugs in assert but I am very happy to address such immediately, when they come up :-). It confuses me though, that implementing new features in assert has to be harder than implementing new features somewhere else in the code base?

assert is also one of the best covered modules: https://coverage.nodejs.org/coverage-fdb35d89605de642/root/assert.js.html
https://coverage.nodejs.org/coverage-fdb35d89605de642/root/internal/util/comparisons.js.html
https://coverage.nodejs.org/coverage-fdb35d89605de642/root/internal/errors.js.html

That also applies to the used code from assert (e.g., util.inspect).

And if requested, I can also improve the coverage to 100%.

@BridgeAR #19106 and #19221 are still open. To be honest I don't really think there are good solutions to any of them, even the solutions I proposed are opinionated and users will always end up having to change their code to make it look right again in certain situations. It's not that implementing new features in public API is harder, but it's more disruptive to have opinionated, complex changes in the public API.

I opened #19467 as a start for #19106. I plan on addressing that soon after :-). #19221 is something I would also like to have. Have you tried the suggestions?

@BridgeAR The thing is, the more feature that assert receives, the more weird the whole things seem weird to me. As a user, I would not expect a good diff and color output from the built in assert module, at most I would just expect that it provides something that empowers user land to built such features on top of (e.g. properties on the Error object), and just be dumb and print everything by default. I also don't see any good way out with the EOL issue to be honest, .gitattributes should be improved of course, but fixing it to make the assert work seems odd to me. We can't expect users all have proper .gitattributes set, or use git to deploy their running code at all.

Hm, I do not see this as a issue. It is all well documented in the docs. So I think it depends on when the user read the docs the last time. My main point on this is: everything in core should not only "work" but it should also provide a good user experience.

The EOL issue is something that does not seem specific to assert but indicates for me that we have a issue somewhere else and it is just visible in assert because we verify the message output all the time.

The output is now improved by showing most properties all at once. Besides that this adds a warning to use `assert.throws` instead due to a better output.

BridgeAR · 2018-04-19T17:25:20Z

I just rebased, removed the message and updated the code further to actually compare all input at the same time from now on. So it now will not matter if common.expectsError or assert.throws is used: it will always show the whole comparison and not only a single property. I used some tricks to make that possible and hope you like it.

BridgeAR · 2018-04-19T17:25:34Z

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

joyeecheung

Not a big fan of tossing the properties around different objects, but since it's just internal test utils, LGTM

BridgeAR · 2018-04-19T21:51:12Z

@joyeecheung I totally agree. I would not have done that if it would not be the internal tests.

I just pushed another commit with some minor adjustments that I missed earlier. PTAL

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

BridgeAR · 2018-04-19T21:56:35Z

Note: the commit message has to be fixed while landing.

jasnell · 2018-04-20T17:21:29Z

Still LGTM!

The output is now improved by showing most properties all at once. Besides that this adds a warning to use `assert.throws` instead due to a better output. PR-URL: nodejs#19797 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR · 2018-04-23T13:34:22Z

Landed in f85d599

The output is now improved by showing most properties all at once. Besides that this adds a warning to use `assert.throws` instead due to a better output. PR-URL: #19797 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR requested a review from joyeecheung April 4, 2018 15:05

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 4, 2018

jasnell requested changes Apr 4, 2018

View reviewed changes

BridgeAR mentioned this pull request Apr 12, 2018

assert: improve assert rejects #19885

Closed

4 tasks

BridgeAR force-pushed the improve-common-expects-error branch from 976efe3 to 719b61f Compare April 19, 2018 17:20

test: improve common.expectsError

e72a028

The output is now improved by showing most properties all at once. Besides that this adds a warning to use `assert.throws` instead due to a better output.

BridgeAR force-pushed the improve-common-expects-error branch from 719b61f to e72a028 Compare April 19, 2018 17:24

jasnell approved these changes Apr 19, 2018

View reviewed changes

joyeecheung approved these changes Apr 19, 2018

View reviewed changes

fixup: remove now obsolete code

8a76c1a

BridgeAR force-pushed the improve-common-expects-error branch from 3f93fc2 to 8a76c1a Compare April 19, 2018 21:48

BridgeAR closed this Apr 23, 2018

BridgeAR deleted the improve-common-expects-error branch January 20, 2020 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: improve common.expectsError #19797

test: improve common.expectsError #19797

BridgeAR commented Apr 4, 2018

BridgeAR commented Apr 4, 2018

jasnell Apr 4, 2018

joyeecheung Apr 4, 2018 •

edited

Loading

BridgeAR Apr 4, 2018

BridgeAR Apr 4, 2018

joyeecheung Apr 4, 2018

BridgeAR Apr 4, 2018 •

edited

Loading

joyeecheung Apr 4, 2018

BridgeAR Apr 5, 2018

joyeecheung Apr 5, 2018 •

edited

Loading

BridgeAR Apr 10, 2018

BridgeAR commented Apr 19, 2018

BridgeAR commented Apr 19, 2018

joyeecheung left a comment

BridgeAR commented Apr 19, 2018

BridgeAR commented Apr 19, 2018

jasnell commented Apr 20, 2018

BridgeAR commented Apr 23, 2018

test: improve common.expectsError #19797

test: improve common.expectsError #19797

Conversation

BridgeAR commented Apr 4, 2018

Checklist

BridgeAR commented Apr 4, 2018

jasnell Apr 4, 2018

Choose a reason for hiding this comment

joyeecheung Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Apr 4, 2018

Choose a reason for hiding this comment

BridgeAR Apr 4, 2018

Choose a reason for hiding this comment

joyeecheung Apr 4, 2018

Choose a reason for hiding this comment

BridgeAR Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Apr 4, 2018

Choose a reason for hiding this comment

BridgeAR Apr 5, 2018

Choose a reason for hiding this comment

joyeecheung Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Apr 10, 2018

Choose a reason for hiding this comment

BridgeAR commented Apr 19, 2018

BridgeAR commented Apr 19, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

BridgeAR commented Apr 19, 2018

BridgeAR commented Apr 19, 2018

jasnell commented Apr 20, 2018

BridgeAR commented Apr 23, 2018

joyeecheung Apr 4, 2018 •

edited

Loading

BridgeAR Apr 4, 2018 •

edited

Loading

joyeecheung Apr 5, 2018 •

edited

Loading