Revert "test: common.expectsError should be a must call"

[jasnell]

This reverts commit 1b2733f. (#14088)

Using common.expectsError() should not necessarily require
common.mustCall() and this changes unnecessarily breaks a
number of test cases in the nodejs/http2 repo. There are
better ways of doing this.

Checklist

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

Affected core subsystem(s)

test

[cjihrig]

Just out of curiosity, can you point me to a test that breaks with the existing change.

[jasnell]

See here: https://github.com/nodejs/http2/blob/master/test/parallel/test-http2-client-stream-destroy-before-connect.js#L14-L22

Specifically:

// Do not mustCall the server side callbacks, they may or may not be called
--
// depending on the OS. The determination is based largely on operating
// system specific timings
server.on('stream', (stream) => {
  stream.on('error', common.expectsError({
  code: 'ERR_HTTP2_STREAM_ERROR',
  type: Error,
  message: 'Stream closed with error code 2'
}));
//....

Depending on the operating system, the 'stream' event may or may not fire and the 'error' event may or may not fire. This is currently a bit non-deterministic because it depends entirely on async timings at the operating system level. I'm working on making it more deterministic but this is what we have currently.

While refactoring this is minimal, it's also a bit pointless -- that is, changing common.expectsError() to always be a common.mustCall() is unnecessary since it is trivial to do common.mustCall(common.expectsError()) as necessary.

[Trott]

LGTM if CI is still green. I'm a fan of less magic in common anyway.

[refack]

I'm a fan of less magic in common anyway.

I would not consider this "magic", rather being more explicit... As can be seen in 1b2733f (#14088) all existing cases of expectsError (177 in total) we're compatible with a mustCall. Also, the term expects implies something should happen.
The case demonstrated above can be easily solved with an if since it's the exception to the rule.

[Trott]

I would not consider this "magic"

FWIW, if I had my way, common.expectsError() wouldn't exist at all. If I'm new to the project and I see a function called expectsError(), what does that even do? I would expect it to act like assert.throws(). I would expect the first argument to be a function that is expected to cause an error.

Our tests would be clearer and more self-contained if we simply wrote out the function that verifies the error. DRY in a test can be an anti-pattern.

That said, my views on what is and isn't appropriate for common are increasingly out of step with the project, it seems. I suspect there will be some 😕 and 👎 on this comment.

[refack]

I would expect the first argument to be a function that is expected to cause an error.

Which it does now 😄
I agree in principle, we should either expose the internal/Error or create a mockNodeError(type, code, message) and do deepStrictEqual

[Trott]

Which it does now 😄

Optionally. Which: ugh. But yes, you are correct and my comment there misses the mark.

I agree in principle, we should either expose the internal/Error or create a mockNodeError(type, code, message) and do deepStrictEqual

In principle, yes. Unfortunately, deepStrictEqual() does not work on errors because of non-enumerable properties:

assert.deepStrictEqual(new Error('foo'), new Error('bar')); // surprise! this passes!

On the upside, there's no reason someone can't submit a PR to fix that shortcoming.

[refack]

On the upside, there's no reason someone can't submit a PR to fix that shortcoming.

Ack. That's a good idea, so we stop patchwork cycle.

[gibfahn]

Marking as Request changes as I think there was some discussion in #14088 (comment) that hasn't been finished (to make sure this doesn't get landed by mistake).

Happy for this to land if @jasnell still wants this after reading that (feel free to dismiss this review).

[gibfahn]

While refactoring this is minimal, it's also a bit pointless -- that is, changing common.expectsError() to always be a common.mustCall() is unnecessary since it is trivial to do common.mustCall(common.expectsError()) as necessary.

My only feeling on this would be that having mustCall as the default might help to catch errors where you forgot to include that, kinda seems like a safer default. Then you could have an option to explicitly opt out.

[jasnell]

Not all that happy about this but it's not important enough to fight :-)