Major rework #18

meeber · 2017-11-26T22:20:48Z

This is related to chaijs/chai#1021. The purpose of these changes is to simplify the existing Chai .throw assertion and move a lot of the logic into this module for easier reuse. These changes also help Chai and its plugins to provide more consistent failed assertion messages related to errors.

NOTE: I added a temporary version number to the package.json file to allow other modules to install this module from github for the purposes of demonstrating how these changes impact chai and chai-as-promised. It should be removed before merging. (But in the long term, I don't think we should continue excluding the version field from package.json... it makes development harder.)

Also related: chaijs/chai-as-promised#235

BREAKING CHANGE: Remove `.compatibleInstance` in preparation for the upcoming addition of a `.checkError` method that'll require the thrown object to be an `Error` instance. With that requirement in place, a simple strict equality check takes the place of `.compatibleInstance`.

BREAKING CHANGE: Previously, `.getMessage` and `.compatibleMessage` accepted both `Error` instances and strings. This commit removes support for strings. This is being done in preparation for the upcoming addition of a `.checkError` method that'll require the thrown object to be an `Error` instance. With that requirement in place, it's no longer necessary to support strings in `.getMessage` and `.compatibleMessage`.

BREAKING CHANGE: Remove `.compatibleConstructor` in preparation for the upcoming addition of a `.checkError` function that'll require `errLike` to be either an `Error` instance or an `Error` constructor. With that requirement in place, a simple strict equality check followed by an `instanceof` check takes the place of `.compatibleConstructor`.

keithamus · 2017-11-27T18:00:27Z

package.json

@@ -19,6 +19,7 @@
    "check-error.js"
  ],
  "main": "./index.js",
+  "version": "2.0.0-wip",


We could change the version to something like 0.0.0-development which is a very loud signal that its not a valid practical version, but it is still would hopefully solve the issues you're seeing.

keithamus · 2017-11-27T18:07:50Z

README.md

+criteria = checkError.createCriteria(errObj);
+checkError.describeExpectedError(criteria); // "[Error: I like waffles]"
+checkError.checkError(new Error('i like waffles'), criteria); // false
+checkError.checkError(errObj, criteria); // true


I don't have time to give this the full review, but just from glancing at the readme I'm a little bit concerned over the notion of a criteria object. My initial questions are:

Why do I, as a consumer of this API, need to create an object before asserting on my error.

Why can't I just pass some arguments in place of criteria. E.g. checkError.checkError(errObj, new Error('I like waffles') or checkError.checkError(errObj, TypeError, /waffles/)

The .createCriteria method performs quite a bit of type validation and argument handling (since two method signatures are supported) that is needed by both .checkError and .describeExpectedError. So the motivation behind the criteria object is only performing that work once. This may be over-engineered. It may be better overall to just bite the bullet and perform the work twice for the purposes of a simpler API. Easy change but will wait for more feedback before taking action.

I figured this was the case; it just feels a little un-ergonomic to have to create arguments using this API, to then give that argument to another part of the API (although not unprecedented, see DataView).

Perhaps we could make the functions that take a criteria polymorphic enough to either accept a criteria, or a criteria signature? This way we get the benefits of both.

We could do something like:

// Each method that wants a `criteria` will also take a criteria signature and coerce as necessary: function checkError(errObj, ...criteriaLike) { const criteria = createCriteria(...criteriaLike) // ... } // `createCriteria(<Criteria>)` is essentially `identity` function createCriteria(errLike, errMsgMatcher) { if (arguments.length === 1 && isCriteria(errLike) return errLike // ... }

Seems reasonable to me.

@keithamus Node v4 is still supported until April. Do we need to implement that logic with ES5?

Sure, the arity semantics are likely of no concern so some sensible variant of function checkError(errObj, criteriaLikeA, criteriaLikeB) will be fine, right?

Excellent idea.
I totally agree with @keithamus. This makes the API a lot more straightforward.

lucasfcosta

For now this looks good, my only concern is with handling "non-Error" objects that might be thrown, such as strings.

According to our core docs we should support throwing anything.

Please correct me if I'm wrong, but if I'm not mistaken I think this line, for example, could be triggered and end up throwing an AssertionError if a String was thrown even though it matches the expected matcher.

lucasfcosta · 2017-11-29T13:19:51Z

README.md

+criteria = checkError.createCriteria(errObj);
+checkError.describeExpectedError(criteria); // "[Error: I like waffles]"
+checkError.checkError(new Error('i like waffles'), criteria); // false
+checkError.checkError(errObj, criteria); // true


Excellent idea.
I totally agree with @keithamus. This makes the API a lot more straightforward.

lucasfcosta · 2017-11-29T13:23:38Z

index.js

-    // If `errorLike` is a constructor that inherits from Error, we compare `thrown` to `errorLike` directly
-    return thrown.constructor === errorLike || thrown instanceof errorLike;
+function getMessage(errorLike) {
+  if (errorLike && errorLike.message) {


What if the thrown error is not an object but a String instead?

Will we handle that within chai-core?

lucasfcosta · 2017-11-29T13:32:23Z

index.js

+
+  if ((criteria.errMsgMatcherType === 'string' ||
+      criteria.errMsgMatcherType === 'regexp') &&
+      !compatibleMessage(errObj, criteria.errMsgMatcher)) {


As I've stated before, what happens if errObj is a string?

IMO if we plan on moving most of the logic to this module instead of keeping it in the core I think it would be suitable for us to deal with throwing Strings here too.

I'm basically making the argument with this PR and chaijs/chai#1021 that there shouldn't be direct support for throwing non-Error objects, as the logic required to support that bad practice is complex and dangerous. Dangerous because it allows the same assertion to pass in different situations. For example, expect(fn).to.throw('blah') would pass when fn is an Error object with the .message property containing "blah", or a string containing "blah", or just a regular object with a .message property containing "blah". As noted in the updated docs in chaijs/chai#1021, I believe people wishing to assert on non-Error objects should use assertion chaining instead.

This seems like a good reason to not support those.

Let's just not forget about updating our docs on the core then.

lucasfcosta · 2017-11-29T13:34:37Z

test/index.js

+      var criteria = checkError.createCriteria();
+      assert(checkError.checkError(notErrObj, criteria) === false);
+    });
+    /***************/


*Nitpick: can't we just use a linebreak instead of these separators?

meeber · 2017-12-27T01:55:55Z

@keithamus @lucasfcosta Just updated this PR based on prior comments

meeber · 2018-01-07T21:41:15Z

Bump!

keithamus · 2018-01-10T11:12:01Z

README.md

-* `compatibleMessage(err, errMatcher)` - Checks if an error message is compatible with an `errMatcher` RegExp or String (we check if the message contains the String).
+* `checkError(errObj[, errLike[, errMsgMatcher]])` or `checkError(errObj[, errMsgMatcher])` - Checks if an `Error` instance matches criteria defined by `errLike` and/or `errMsgMatcher`.
+    * If `errLike` is an `Error` constructor, then validate that `errObj` is an instance of `errLike`.
+    * If `errLike` is an `Error` instance, then validate that `errObj` is strictly (`===`) equal to `errLike`.


💭 Given some of the talk in the chai issue tracker, and around deep-eql, should we losen this restriction to say

If errLike is an Error instance, then validate that errObj has the same constructor, message and any other enumerable properties (excluding stack)?

I'm in favor of this change. But I think it'd need to be implemented in deep-eql and imported here. If one of the enumerable properties is an object, then I believe the normal deep equality algorithm should apply to that property.

Yes, well said, let's leave it out of scope here, and work on putting this in deep-eql and bubbling up through the codebase.

keithamus · 2018-01-10T11:12:43Z

README.md

+    * If `errLike` is an `Error` constructor, then validate that `errObj` is an instance of `errLike`.
+    * If `errLike` is an `Error` instance, then validate that `errObj` is strictly (`===`) equal to `errLike`.
+    * If `errLike` is a criteria object, then validate that `errObj` matches the criteria that was originally passed to `.createCriteria` in order to create the criteria object.
+    * If `errLike` is omitted or is explicitly `null` or `undefined`, then it defaults to the built-in `Error` constructor.


Nit: I'd like this at the top of this list, so the logic can fall down naturally - more reflective of the steps in the algorithm.

keithamus · 2018-01-10T11:14:27Z

README.md

+* `checkError(errObj[, errLike[, errMsgMatcher]])` or `checkError(errObj[, errMsgMatcher])` - Checks if an `Error` instance matches criteria defined by `errLike` and/or `errMsgMatcher`.
+    * If `errLike` is an `Error` constructor, then validate that `errObj` is an instance of `errLike`.
+    * If `errLike` is an `Error` instance, then validate that `errObj` is strictly (`===`) equal to `errLike`.
+    * If `errLike` is a criteria object, then validate that `errObj` matches the criteria that was originally passed to `.createCriteria` in order to create the criteria object.


This step should be moved to the top - and maybe criteria object can be changed to criteria object (such as that made by `createCriteria`)

keithamus · 2018-01-10T11:19:23Z

index.js

@@ -76,10 +56,11 @@ function compatibleConstructor(thrown, errorLike) {
 */

 function compatibleMessage(thrown, errMatcher) {
-  var comparisonString = typeof thrown === 'string' ? thrown : thrown.message;
-  if (errMatcher instanceof RegExp) {
+  var errMatcherType = type(errMatcher).toLowerCase();


If we are literally just comparing regexp and string, a typeof/instanceof check would be better (faster) here.

keithamus · 2018-01-10T11:21:53Z

index.js

@@ -101,7 +82,7 @@ function getConstructorName(errorLike) {
  var constructorName = errorLike;
  if (errorLike instanceof Error) {
    constructorName = getFunctionName(errorLike.constructor);
-  } else if (typeof errorLike === 'function') {
+  } else if (type(errorLike) === 'function') {


Similarly here - typeof would be better, as we closely control the flow here - type-detect is better put to use where we want to check object types. If we're only ever checking primitives, it's worth considering avoiding using type-detect entirely IMO

keithamus · 2018-01-10T11:22:45Z

index.js

+  var errLikeType = type(errLike).toLowerCase();
+  var errMsgMatcherType = type(errMsgMatcher).toLowerCase();
+  // Handle a criteria object being passed by just returning the same object.
+  if (errLikeType === 'object' && isCriteria(errLike)) {


Seems isCriteria could add the additional typeof errLike === 'object' check. Additionally - might want to avoid type-detect here.

keithamus · 2018-01-10T11:24:14Z

index.js

+  // Handle `errLike` being omitted and `errMsgMatcher` being passed as the
+  // second argument.
+  if (errLikeType === 'string' || errLikeType === 'regexp') {
+    if (errMsgMatcherType !== 'undefined' && errMsgMatcherType !== 'null') {


More type stuff here could use typeof/instanceof.

keithamus · 2018-01-10T11:26:09Z

index.js

+  var desc = '';
+  var constructor = getConstructorName(criteria.errLike);
+  var first = constructor.charAt(0).toLowerCase();
+  var vowels = [ 'a', 'e', 'i', 'o', 'u' ];


Maybe a premature optimisation, but this array doesn't need to be created each time. Could have it as a const

keithamus

Generally a great improvement. Super well done 👍! I really like how chai is shaping up these days.

I think we should drop type-detect - I don't think it brings much value here, and adds to the complexity/performance characteristics of this codebase and would also introduce maintainence burdens if chai ends up shipping multiple versions of type-detect (like it did with deep-eql for a while).

Additionally, something to think about (maybe not for this PR) is ES6ifying this codebase. I'd like us to move to rollup so chai can ultimately ship as a single compiled file (for performance reasons) and ES6 will get us there.

- Add `.createCriteria` - Add `.checkError` - Add `.describeExpectedError`

The initial implemention of `.checkError` and `.describeExpectedError` only accepted criteria objects for their criteria, which means that `.createCriteria` had to be manually called first. This update allows `errLike` and/or `errMsgMatcher` to be passed directly to those methods instead, in which case a criteria object is created internally.

meeber · 2018-01-21T20:55:17Z

@keithamus I just updated this PR based on your review. (I'll need to remove the "wip version num" commit before we merge this though.)

keithamus · 2018-01-22T11:07:05Z

I'll try to get a review done this week @meeber - but I might be too busy. Rest assured it's in my todo list to pick up as soon as I can.

BREAKING CHANGE: Previously, `Error` instances were compared using strict equality. This commit changes the comparison algorithm to deep equality. Users wishing to compare two `Error` instances via strict equality should just use `===` directly instead of this module.

BREAKING CHANGE: This commit drops support for versions of Node below v6, and updates the Travis config accordingly.

Previously, Travis was configured to cache `node_modules`, which was causing the build to break.

meeber added 3 commits November 25, 2017 01:36

This was referenced Nov 26, 2017

WIP: Update .throw and add .error chaijs/chai#1021

Closed

[WIP] feat: update .rejectedWith with new .checkError chaijs/chai-as-promised#235

Open

keithamus reviewed Nov 27, 2017

View reviewed changes

lucasfcosta reviewed Nov 29, 2017

View reviewed changes

meeber mentioned this pull request Jan 7, 2018

feat: Add the assert.fail([message]) interface chaijs/chai#1117

Merged

keithamus reviewed Jan 10, 2018

View reviewed changes

keithamus approved these changes Jan 10, 2018

View reviewed changes

meeber mentioned this pull request Jan 11, 2018

Concerns with deep equality algorithm chaijs/chai#907

Closed

meeber added 2 commits January 21, 2018 19:53

feat: add new methods

6438b02

- Add `.createCriteria` - Add `.checkError` - Add `.describeExpectedError`

meeber mentioned this pull request Feb 10, 2018

Deep equality check for Errors chaijs/chai#1065

Closed

meeber mentioned this pull request Jul 24, 2018

Cannot handle thrown if not derived from Error chaijs/chai#1175

Closed

meeber added 3 commits November 18, 2018 20:24

chore: update minimum supported Node version to v6

308017f

BREAKING CHANGE: This commit drops support for versions of Node below v6, and updates the Travis config accordingly.

chore: update dependencies

ce1a5cc

meeber added 5 commits November 18, 2018 20:24

chore: add package-lock.json

de5becf

style: fix linting errors

46a22a3

chore: add wip version num to package.json

c44a018

chore: replace PhantomJS with ChromeHeadless

e82f4ac

chore: update Travis config to cache $HOME/.npm

93fd1cc

Previously, Travis was configured to cache `node_modules`, which was causing the build to break.

meeber mentioned this pull request Dec 29, 2018

WIP: Overhaul .throw chaijs/chai#1220

Closed

keithamus deleted the branch chaijs:master October 8, 2021 09:51

keithamus closed this Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major rework #18

Major rework #18

meeber commented Nov 26, 2017 •

edited

Loading

keithamus Nov 27, 2017

keithamus Nov 27, 2017

meeber Nov 27, 2017

keithamus Nov 27, 2017

meeber Nov 27, 2017

meeber Nov 28, 2017

keithamus Nov 28, 2017

lucasfcosta Nov 29, 2017

lucasfcosta left a comment

lucasfcosta Nov 29, 2017

lucasfcosta Nov 29, 2017

lucasfcosta Nov 29, 2017

meeber Nov 29, 2017

lucasfcosta Nov 30, 2017

lucasfcosta Nov 29, 2017

meeber commented Dec 27, 2017

meeber commented Jan 7, 2018

keithamus Jan 10, 2018

meeber Jan 10, 2018

keithamus Jan 10, 2018

keithamus Jan 10, 2018

keithamus Jan 10, 2018

keithamus Jan 10, 2018

keithamus Jan 10, 2018

keithamus Jan 10, 2018

keithamus Jan 10, 2018

keithamus Jan 10, 2018

keithamus left a comment

meeber commented Jan 21, 2018 •

edited

Loading

keithamus commented Jan 22, 2018

Major rework #18

Major rework #18

Conversation

meeber commented Nov 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber commented Dec 27, 2017

meeber commented Jan 7, 2018

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

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

keithamus left a comment

Choose a reason for hiding this comment

meeber commented Jan 21, 2018 • edited Loading

keithamus commented Jan 22, 2018

meeber commented Nov 26, 2017 •

edited

Loading

meeber commented Jan 21, 2018 •

edited

Loading