test: refactor expectsError #14058

BridgeAR · 2017-07-03T13:50:55Z

So far common.expectsError only tested type, message and code instead of all properties.

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

tniessen · 2017-07-03T14:07:12Z

Where do we use this behavior?

BridgeAR · 2017-07-03T14:09:31Z

@tniessen every error thrown by assert have a couple more properties and currently they are tested only for assert.fail. In general I think it makes sense to test for everything that is requested for and not only for some of these properties.

DavidCai1111 · 2017-07-05T03:36:32Z

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

refack · 2017-07-05T03:16:26Z

test/common/index.js

+          assert.strictEqual(error.message, message);
+        }
+      } else {
+        assert.strictEqual(error[key], options[key]);


I'm thinking this needs a custom message The ${key} field of ${error} was ${error[key]} and not {options[key]} as expected (or a little bit shorter)
Otherwise we get an AssertionError: a !== b thrown from deep within the harness.
So the same applies for L716 and the message strings.

refack · 2017-07-05T03:16:29Z

test/common/index.js

-      assert.strictEqual(error.message, message);
-    }
+    Object.keys(options).forEach((key) => {
+      if (key === 'type') {


~~Would a switch look better?~~
New code looks real pretty 💅

refack · 2017-07-05T03:41:03Z

test/common/index.js

+          assert(error instanceof type,
+                 `${error} is not the expected type ${type}`);
+        }
+      } else if (key === 'message') {


If we keep the if/else change to (key === 'message' && options.message instanceof RegExp) and let the string case fall to the else

benjamingr

I'm (not strongly) -1 on this change. I don't think every single property should match the options.

It complicates the code a little and I'm not sure this is a good idea.

lpinca · 2017-07-05T16:15:10Z

Well I guess the point is to make it flexible. Instead of forcing at max code, type, and message it optionally allows to match other properties. I find it useful.

benjamingr · 2017-07-05T16:26:07Z

I understand the point and appreciate the effort - but I think it makes the code more confusing for no good reason at the moment - and I can see the pitfalls of tests failing after an object is passed and a property is added unintentionally.

If it was called .expects and not .expectsError or just deepEqual I would understand.

lpinca · 2017-07-05T16:38:44Z

I can see the pitfalls of tests failing after an object is passed and a property is added unintentionally.

It will fail also as is with unintentionally added properties (code or type) to the input object :)

BridgeAR · 2017-07-05T16:58:23Z

@benjamingr I ran into this with another PR as I wanted to test additional properties than just code, type and message and I expected a wrong value in one of the properties and didn't realize that at first as the test passed even though it shouldn't have.

You have to explicitly list every property that you want to test for and if you do not want to test any besides e.g. code and type it'll work as expected:

const err = new Error('foobar')
err.code = 'RANDOM'
err.additional = true
err.foobar = 'is ignored'

// Tests only code
common.expectsError({
  code: 'RANDOM'
})(err)

// Tests other properties
common.expectsError({
  code: 'RANDOM',
  additional: true
  type: Error
})(err)

refack

💯

refack · 2017-07-05T17:17:41Z

I understand the point and appreciate the effort - but I think it makes the code more confusing for no good reason at the moment - and I can see the pitfalls of tests failing after an object is passed and a property is added unintentionally.

As I see it the crux of the issue is that the new NodeErrors are "internal" and we can't easily generate them in test code, so we need a helper to generate mock Errors. So as I see it this helper needs to generate/validate that the expected Errors are stable since they are API, so the unintentionally case is a pro, not a con.

The main problem with the current code is the destructuring of the formal agrument: exports.expectsError = function expectsError({code, type, message}) { that silently swallows extra properties...
So we either go with this PR — or validate that the passed object contains only {code, type, message}

P.S. @benjamingr IMHO the new version of the code is almost as succinct as the code before... and fixes the problem of unuseful messages.. so⚖️

Ref: #13686

refack · 2017-07-05T17:19:25Z

Also relevant #13937 if message change are semver-major so should changes to all other properties of thrown Errors

refack · 2017-07-05T17:22:09Z

test/common/index.js

+      const actual = error[key];
+      const expected = options[key];
+      if (key === 'type') {
+        if (expected !== undefined) {


If it was called .expects and not .expectsError or just deepEqual I would understand.

That made me think, remove the if and that will make options.type mandatory.
Also add ~~assert(expected instanceof Error)~~ if (!(expected instanceof Error)) throw new TypeError('`options.type` must inherit from `Error`');

That does not work as the class is passed to the function and not the instance. The simplest check I could though of is: expected !== Error && !Error.isPrototypeOf(expected)

refack · 2017-07-05T19:51:18Z

@BridgeAR I don't want to mix 🍎s and 🍊s, but we should actually add a validation that code is passed (re:#13686 (comment)).. so maybe that should go into a new PR?
Also we still need to convince @benjamingr this is a good and important improvement, and need to get consensus that expectsError is only to be used to check for NodeErrors (or at least Error's with an assinged code).

BridgeAR · 2017-07-09T10:15:01Z

Rebased due to conflicts and I updated the documentation as I missed that.

@benjamingr would you mind having another look?

benjamingr · 2017-07-09T10:21:08Z

I'm fine with asserting that it's an instance of an error - but I'm not sure about testing all properties.

I'm concerned about cases where a "dirty" error object would be passed and it would verify incorrect things by mistake. I'm not sure I see the value this adds to be honest.

The method is already a little footgunny for verifying things by convention - going over all object properties and treating some of them differently doesn't sound like good design to me.

BridgeAR · 2017-07-09T10:30:21Z

I'm not sure I understand what you mean with the "dirty" error object. Do you mean the common.expectsError settings argument or the error passed to the returned function?

As you can see there is no change needed for any test and the function will only test properties explicitly asked for in the settings object. How could that verify incorrect things by mistake?

For me the current situation was a minor footgun by not testing properties that I wanted it to test for.

benjamingr · 2017-07-09T10:38:03Z

I'm not sure I understand what you mean with the "dirty" error object. Do you mean the common.expectsError settings argument or the error passed to the returned function?

I mean someone passing a TypeError that has a property added to it by mistake or something similar.

refack · 2017-07-09T22:19:03Z

The method is already a little footgunny for verifying things by convention - going over all object properties and treating some of them differently doesn't sound like good design to me.

Ack. Probably the best thing would be to add --expose-internals to all tests and validate against the real deal + Have 1 test file that tests the proper construction of all the errors.
Currently as I see it there is as issue that the NodeErrors have other properties that we do not assert the stability of, and thay are in effect part of the public API that we don't tests for regressions.
Maybe a good compromise would be not to use the keys of expected but to iterate on a const set — for Error, TypeError, and RangeError it's [message, name, code] and for AssertionError there's also [generatedMessage, name, code, actual, expected, operator] and that stackStartFunction is not in .stack.

jasnell · 2017-07-10T17:08:01Z

test/common/README.md

  * `type` [&lt;Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function)
-    expected error must be an instance of `type`
+    expected error must be an instance of `type` and should be [subclassed by
+    the `internal/errors` module](https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md#api).


This additional bit subclassed by the 'internal/errors' module doesn't really make sense. The thing must be an instance of type and must be an Error subclass. Whether it is a subclass of one of the internal/errors.js classes is somewhat inconsequential.

jasnell · 2017-07-10T17:08:33Z

test/common/README.md

-
-The expected error should be [subclassed by the `internal/errors` module](https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md#api).
+* return [&lt;Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function)
+  If `fn` is provided, it will be passed to `assert.throws` as first argument


Need a blank line after the * return line... and it should be * Returns:

The * Returns: would be inconsistent with all other entries in this file. Shall I change all other entries as well?

benjamingr

New version not using Object.keys looks good to me

BridgeAR · 2017-07-11T12:36:34Z

I completely reworked the code as suggested by @refack
IMHO this is not as nice as it was before but I hope this is a good compromise.

refack · 2017-07-11T13:11:01Z

New version not using Object.keys looks good to me

I completely reworked the code as suggested by @refack
IMHO this is not as nice as it was before but I hope this is a good compromise.

I had this whole spiel about flattening error and settings then using deepStrinctEqual, but that should be a new PR...

use the code here to generate a mock Error that could be compared
mock should have:
1. const keys = [name, message];
2. If (settings.Type.name === 'AssertionError') keys.push(...['generatedMessage', 'actual', 'expected', 'operator']);
3. const mockError = keys.reduce((s,k) => Object.assign(k, {[k]: settings[k]}), {});
error could be flattened with const flatActual = keys.reduce((s,k) => Object.assign(k, {[k]: error[k]}), {});
const name = `${settings.type.name} [${settings.code]}]`; - https://github.com/nodejs/node/blob/master/lib/internal/errors.js#L34
two properties will need special treatment (but IMHO that should go into assert somehow):
1. mockError.stack = (s) => s.includes(settings.stackExcludes) === false;
2. mockError.message = (s) => (settings.message instanceof RegEx ? settings.message.test(s) : settings.message === s;
We don't have to compare Type because it's embedded in name, just that error instanceof Error
finally: assert.deepStrinctEqual(flatActual, flatExpected);

BridgeAR · 2017-07-18T17:45:50Z

Rebased. @refack could this land or do you want me to still change something?

refack · 2017-07-19T13:26:31Z

Didn't see that the CR was dismissed.
Pre-land CI: https://ci.nodejs.org/job/node-test-commit/11255/

PR-URL: nodejs#14058 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

refack · 2017-07-19T19:18:45Z

Landed in 2a621d4

refack · 2017-07-19T19:19:48Z

Extra sanity run on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7431/

MylesBorins · 2017-11-16T23:07:13Z

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 · 2017-12-11T06:25:06Z

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

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 3, 2017

lpinca approved these changes Jul 4, 2017

View reviewed changes

refack suggested changes Jul 5, 2017

View reviewed changes

benjamingr requested changes Jul 5, 2017

View reviewed changes

refack approved these changes Jul 5, 2017

View reviewed changes

refack reviewed Jul 5, 2017

View reviewed changes

refack approved these changes Jul 5, 2017

View reviewed changes

refack mentioned this pull request Jul 7, 2017

errors, buffer: Migrate buffer errors to use internal/errors #13976

Merged

4 tasks

BridgeAR force-pushed the refactor-expects-error branch from 01d4c28 to a635532 Compare July 9, 2017 10:12

refack self-assigned this Jul 9, 2017

jasnell reviewed Jul 10, 2017

View reviewed changes

BridgeAR force-pushed the refactor-expects-error branch from a635532 to 82d6c48 Compare July 11, 2017 12:31

benjamingr approved these changes Jul 11, 2017

View reviewed changes

gibfahn approved these changes Jul 11, 2017

View reviewed changes

test: validate all properties in expectsError

00479f6

BridgeAR force-pushed the refactor-expects-error branch from f411221 to 00479f6 Compare July 18, 2017 17:44

refack closed this Jul 19, 2017

refack mentioned this pull request Jul 24, 2017

[v8.x] test: improve the test common documentation #14454

Closed

addaleax added dont-land-on-v8.x and removed dont-land-on-v8.x labels Jul 27, 2017

jasnell mentioned this pull request Oct 30, 2017

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

Merged

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

BridgeAR added dont-land-on-v6.x and removed backport-requested-v6.x labels Dec 11, 2017

refack removed their assignment Oct 12, 2018

BridgeAR deleted the refactor-expects-error branch April 1, 2019 23:36

test: refactor expectsError #14058

test: refactor expectsError #14058

Conversation

BridgeAR commented Jul 3, 2017

Checklist

Affected core subsystem(s)

tniessen commented Jul 3, 2017

BridgeAR commented Jul 3, 2017

DavidCai1111 commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

lpinca commented Jul 5, 2017

benjamingr commented Jul 5, 2017

lpinca commented Jul 5, 2017 • edited Loading

BridgeAR commented Jul 5, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

refack commented Jul 5, 2017

refack commented Jul 5, 2017

refack Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 5, 2017 • edited Loading

BridgeAR commented Jul 9, 2017

benjamingr commented Jul 9, 2017 • edited Loading

BridgeAR commented Jul 9, 2017

benjamingr commented Jul 9, 2017

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

benjamingr left a comment

Choose a reason for hiding this comment

BridgeAR commented Jul 11, 2017

refack commented Jul 11, 2017

BridgeAR commented Jul 18, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017

MylesBorins commented Nov 16, 2017

BridgeAR commented Dec 11, 2017

refack Jul 5, 2017 •

edited

Loading

lpinca commented Jul 5, 2017 •

edited

Loading

BridgeAR commented Jul 5, 2017 •

edited

Loading

refack Jul 5, 2017 •

edited

Loading

refack commented Jul 5, 2017 •

edited

Loading

benjamingr commented Jul 9, 2017 •

edited

Loading