test: wrap expectsError in mustCall

[BridgeAR]

To make sure that all expectsError calls are indeed called, wrap them in mustCall.
By doing so I found one obsolete expectsError call in the tests.

I think it's nice to have the exact number of calls but having a minimum would also be good if the churn is unwanted.

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)

test

[Trott]

@nodejs/testing

[refack]

Need docs https://github.com/nodejs/node/blob/master/test/common/README.md

[refack]

CI: https://ci.nodejs.org/job/node-test-commit/10996/

[BridgeAR]

Documentation added

[refack]

Pre land CI: https://ci.nodejs.org/job/node-test-pull-request/9042/

[refack]

@BridgeAR I've rebased at refack@0c31246 PTAL
https://ci.nodejs.org/job/node-test-commit-linuxone/7156/ 🔴 sanity test failed

[refack]

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

[refack]

Extra sanity on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7169/ ✔️

[jasnell]

I missed this PR because I was on vacation last week. I'm not a fan of this at all and now have to go through and refactor a number of http2 tests I've written to account for this. For example, there are a couple of tests where an error might be reported and should only be checked if it actually is. For those, I've done stream.on('error', common.expectsError(...)). The semantics of that check now change entirely because common.expectsError() is now mustCall() when it previously wasn't. For those checks, expectsError() is now unusable and I end up having to check the error objects manually.

What would have been better is adding an option such that:

common.expectsError({
  code: '...',
  type: Error,
  message: '...',
  mustCall: 2});

Wherein if the mustCall option is set to a number > -1, then, and only then, would the returned function be wrapped in a common.mustCall(fn, options.mustCall)

[BridgeAR]

@jasnell hm, instead of making the mustCall optional, I think it would be better to explicitly opt-out of that.

So maybe a good follow up PR would be to add a mustCall: false option?

[jasnell]

-1 to opt out. The semantics of common.expectsError() do not automatically imply common.mustCall().

[BridgeAR]

So far there was not a single test that was not a mustCall. I was not aware of the tests for http2 and the name expectsError somewhat imply to me that it's a mustCall.

[refack]

@BridgeAR maybe only implicitly activate in the new explicit fn case.

[refack]

expectsError somewhat imply to me that it's a mustCall.

Actually I agree with @BridgeAR expects is BDD for should happen.
@jasnell if it's just the extra work for http2 point to the right place, and I'll post a PR.

[jasnell]

I'm already fixing the tests. It's not all bad :-) ... it did help me find a bug in a test that I accidentally had not completed 100%, but there are a couple of cases where this change is a bit unfortunate. For instance, in one case, whether or not the 'error' event is actually thrown in dependent entirely on operating system specific timings. I had been doing stream.on('error', common.expectsError({...})) to validate the error if it is emitted, but there's no guarantee that the 'error' event would be emitted. Now that test has to change because common.expectsError() is no longer usable for that scenario.

[BridgeAR]

@jasnell as you already fixed the tests and also had a benefit because of this: do you still want to revert it?

[gibfahn]

I had been doing stream.on('error', common.expectsError({...})) to validate the error if it is emitted, but there's no guarantee that the 'error' event would be emitted.

I guess we could have an alternate function that doesn't do the mustCall for this case (e.g. common.checkError), and then have expectsError = mustCall + checkError.

[refack]

I had been doing stream.on('error', common.expectsError({...})) to validate the error if it is emitted, but there's no guarantee that the 'error' event would be emitted.
I guess we could have an alternate function that doesn't do the mustCall for this case (e.g. common.checkError), and then have expectsError = mustCall + checkError.

I don't want to 👎 but it seems like the case mentioned above is an exception to the rule, and shouldn't necessitate a new method. As usual the discussion if getting fragmented #14159 (comment). Gist:

1. All existing cases of expectsError (177 in total) we're compatible with a mustCall
2. Exception cases could be guarded with a simple if
3. expectsError is a kludge in general, and should be replaced with assert.deepStrictEqual (pending solution to enumeration of inherited properties)
4. @gibfahn will enjoy his vecation! 🌴 🌞 🍾

[gibfahn]

Agreed that if it's only used in 1 or 2 places it's probably not worth pulling it out into common.

I was thinking about whether you could just use deepStrictEqual, but AIUI in expectsError you don't exhaustively check all the properties, you choose to check one or more of code, type, message. So deepStrictEqual might not work for all cases.

👎 is fine, polite disagreement is what it's there for!

[addaleax]

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR.

[MylesBorins]

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

[BridgeAR]

This function does not exist in v.6 so backporting is obsolete.