Support AssertionError expected and actual values #3217

kentcdodds · 2017-03-28T13:13:08Z

Summary

Closes #3173 "Support assert module more cleanly" - basically include error.expected and error.actual in the error message.

Test plan

I've got a project that's using ESLint's tester (which uses the assert.equal method). Here's a before:

And here's after:

As you can see, I could use some color fixes. I'll also note a few places in the code I'm unsure of and could use some help with.

kentcdodds · 2017-03-28T13:14:32Z

packages/jest-jasmine2/src/jasmine/Spec.js

  this.addExpectationResult(
    false,
    {
-      matcherName: '',
+      matcherName: 'blah blah',


If I leave matcherName as an empty string then what I get is the same thing as if I had not made any changes at all. I'm unsure of what this should be set to though.

Note that when the matcherName is blah blah it does not fail any tests either 🤔

JKJK, when the matcherName is changed, in addition to when the message is not undefined a bunch of snapshots fail because messages are better now:

interesting. I think it's changing the way we deal with other types of errors too. Let me check out the branch and see what's going on

kentcdodds · 2017-03-28T13:16:21Z

packages/jest-jasmine2/src/jasmine/Spec.js

  this.addExpectationResult(
    false,
    {
-      matcherName: '',
+      matcherName: 'blah blah',
+      message,


I've verified that adding this property (even when it's undefined in most cases) does not cause a negative impact on the other tests. Though the tests are failing due to snapshot issues noted in #3216

cpojer · 2017-03-28T14:03:22Z

packages/jest-jasmine2/src/jasmine/Spec.js

+      `Received:\n` +
+      `  ${printReceived(actual)}` +
+      (diffString ? `\n\nDifference:\n\n${diffString}` : '');
+  }


Any way we can make the call to diff and the message lazy, as in message = () => {…}? Otherwise I'm a bit worried here about perf in the case where we don't actually need the message.

I tried that earlier and the process just hung forever which I thought was odd...

aaronabramov · 2017-03-28T21:08:06Z

packages/jest-jasmine2/src/jasmine/Spec.js

@@ -121,10 +127,26 @@ Spec.prototype.onException = function onException(e) {
    return;
  }

+  const {expected, actual} = e;


sometimes people do strange things like

throw null;

i wonder if Jest will blow up here

Ah, yeah, that's totally weird, and it probably would blow up. We can add || {} to that.

aaronabramov · 2017-03-28T21:14:08Z

packages/jest-jasmine2/src/jasmine/Spec.js

  this.addExpectationResult(
    false,
    {
-      matcherName: '',
+      matcherName: 'blah blah',


interesting. I think it's changing the way we deal with other types of errors too. Let me check out the branch and see what's going on

aaronabramov · 2017-03-29T02:24:12Z

hey @kentcdodds
i also added a few snapshot tests for node assertion errors #3225
you can use it to test your changes here, just update the snapshots when you change the messages :)

kentcdodds · 2017-03-29T03:13:23Z

I rebased @DmitriiAbramov's commit from #3225 because I figure that his change will be merged first and it's probably useful to see how this will integrate with it.

kentcdodds · 2017-03-29T03:15:12Z

integration_tests/__tests__/__snapshots__/failures-test.js.snap

+  ● assert.deepEqual
+
+    Error
+      expect(received).toBe(expected)


Looks like toBe is probably the wrong thing to use for the matcher hint... 🤔

kentcdodds · 2017-03-29T12:30:53Z

To be clear, I'm looking for some direction on this pull request. Any help appreciated :)

aaronabramov · 2017-03-29T22:36:39Z

@kentcdodds i was thinking that we should probably pull this functionality out into a separate module that's responsible for node assert errors only.
so in the jasmine hook that you used it will be:

const assertionErrorMessage = formatNodeAssertionError(error);
if (assertionErrorMessage) {
  return this.addExpectationResult(
    false,
    {
      matcherName: SOME_CONSTANT_THAT_HAS_AN_ASSERTION_MATCHER_VALUE,
      message: assertionErrorMessage,
      passed: false,
      // ...
    },
  );
} else {
  // do wathever the default jasmine behavior is
}

this way whenever we kill jasmine (it is happening) we can still reuse the logic and it'll be fairly independent.

further on the assertion formatting, i think we can create a completely separate set of messages for each of the assertion failures.

i tried to throw a few errors in the console using this:

try { assert.deepEqual(false, 1) } catch (error) { console.log(error) }

and i found that you can differentiate them using the operator property:

so i think we can format messages like:

assert.equal(first, secord)

Asserted first value:
  'abc'
to equal second value:
  'cde'
Difference:
  <DIFF>

what do you think?

kentcdodds · 2017-04-01T02:49:57Z

That sounds great. Any suggestions on the coloring issue?

aaronabramov · 2017-04-03T20:29:56Z

hm. it looks like the entire message is dimmed. I'm not sure, but i think the reset ASCII code should fix it. (chalk.reset)

thymikee · 2017-04-05T22:32:32Z

@kentcdodds just a heads-up – I'm working on refining this PR so that errors have proper stack traces and indenting. Will update it tomorrow or so.

Here's a preview:

kentcdodds · 2017-04-05T22:37:15Z

Oh awesome, thank you @thymikee!

thymikee · 2017-04-06T09:20:51Z

I've taken all possible functions from Assert docs.

Since there's no way to differentiate form assert.equal() assert() and its alias assert.ok I've came up with this hint:

This is how I print custom message assertion message:

And more complex matchers:

They now look pretty similar to our expect messages – this is why I've put the assertionErrorMessage function in jest-matchers.

Not so happy with assert.ifError assert.doesNotThrow and assert.throws but at least we have helpful stack trace:

kentcdodds · 2017-04-06T15:00:07Z

I think this is great! Thank you so much @thymikee! What can I do to help get it across the finish line?!

thymikee · 2017-04-06T16:12:46Z

Got support for throwing matchers:

@kentcdodds thanks! I think this is ready for a review.
cc: @DmitriiAbramov

codecov-io · 2017-04-07T21:53:06Z

Codecov Report

Merging #3217 into master will decrease coverage by 0.34%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #3217      +/-   ##
=========================================
- Coverage   64.24%   63.9%   -0.35%     
=========================================
  Files         175     176       +1     
  Lines        6436    6471      +35     
  Branches        4       4              
=========================================
  Hits         4135    4135              
- Misses       2300    2335      +35     
  Partials        1       1

Impacted Files	Coverage Δ
packages/jest-jasmine2/src/jasmine/Spec.js	`0% <0%> (ø)`	⬆️
packages/jest-matchers/src/assert-support.js	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b93f9c6...4332df0. Read the comment docs.

cpojer · 2017-04-11T11:11:21Z

integration_tests/__tests__/__snapshots__/failures-test.js.snap

+
+    assert.deepEqual(received, expected)
+
+    Expected value to deepEqual to:


Is this just like ==, but deep? In that case, how about:

"Expected value to deeply equal:"

maybe?

cpojer · 2017-04-11T11:11:35Z

integration_tests/__tests__/__snapshots__/failures-test.js.snap

+
+    assert.notDeepEqual(received, expected)
+
+    Expected value to notDeepEqual to:


"not to deeply equal"?

cpojer · 2017-04-11T11:11:53Z

integration_tests/__tests__/__snapshots__/failures-test.js.snap

+
+    assert.deepStrictEqual(received, expected)
+
+    Expected value to deepStrictEqual to:


"to deeply and strictly equal"?

cpojer · 2017-04-11T11:12:30Z

integration_tests/__tests__/__snapshots__/failures-test.js.snap

+    But it didn't throw anything.
+
+    Message:
+      Missing expected exception..


This is coming from assert, right?

Yes, assert generates this message and marks that it's not generated 🤷‍♂️, that's why it's shown here.

cpojer · 2017-04-11T11:13:50Z

packages/jest-jasmine2/src/jasmine/Spec.js

    return;
  }

+  if (error instanceof require('assert').AssertionError) {
+    const assertionErrorMessage = require('jest-matchers/build/assert-support');


We shouldn't do this. Can we move assert-support out of jest-matchers? Can it be in jest-jasmine2 instead?

cpojer · 2017-04-11T11:15:01Z

This is nice! I would recommend not to put it into jest-matchers, though. It's a jest feature, not a jest-matchers feature (to be renamed to expect).

cpojer

See comments.

thymikee · 2017-04-11T13:19:57Z

Updated the messaging and moved assert-support to jest-jasmine2.

cpojer · 2017-04-11T13:20:55Z

Let's do it.

* node `assert` snapshot tests * WIP: getting close to a working version * Refactor assert error messages * Remove jest-diff from jest-jasmine2 deps * Support throws and doesNotThrow assert matchers * Update snapshots * Fix snapshots across different node verions * Human readable messages

github-actions · 2021-05-13T19:12:52Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label Mar 28, 2017

kentcdodds commented Mar 28, 2017

View reviewed changes

cpojer reviewed Mar 28, 2017

View reviewed changes

aaronabramov reviewed Mar 28, 2017

View reviewed changes

kentcdodds force-pushed the pr/assert branch from c9d00be to 6efe6f1 Compare March 28, 2017 21:30

aaronabramov and others added 2 commits March 28, 2017 21:08

node assert snapshot tests

a4dc6bd

WIP: getting close to a working version

d413b63

kentcdodds force-pushed the pr/assert branch from 6efe6f1 to d413b63 Compare March 29, 2017 03:12

kentcdodds commented Mar 29, 2017

View reviewed changes

Refactor assert error messages

90497d5

thymikee mentioned this pull request Apr 6, 2017

node 'assert' errors snapshot tests #3225

Closed

thymikee requested a review from aaronabramov April 6, 2017 09:21

thymikee added 2 commits April 6, 2017 11:25

Merge branch 'master' into pr/assert

ba9e4b5

Remove jest-diff from jest-jasmine2 deps

0acdaf7

thymikee added 2 commits April 6, 2017 18:10

Support throws and doesNotThrow assert matchers

8969ed8

Update snapshots

cc43225

Fix snapshots across different node verions

4332df0

cpojer reviewed Apr 11, 2017

View reviewed changes

cpojer requested changes Apr 11, 2017

View reviewed changes

Human readable messages

f6d7126

cpojer merged commit 0d73c53 into jestjs:master Apr 11, 2017

kentcdodds deleted the pr/assert branch April 11, 2017 13:30

kentcdodds mentioned this pull request Apr 11, 2017

Improve error message for differing output in RuleTester eslint/eslint#8173

Closed

thymikee mentioned this pull request Jun 8, 2017

jest-circus test failures #3770

Merged

v1adko mentioned this pull request Aug 2, 2017

Support the assert module more cleanly #3173

Closed

jwalton mentioned this pull request Oct 12, 2018

expectBody not verbose enough jwalton/node-supertest-fetch#16

Open

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support AssertionError expected and actual values #3217

Support AssertionError expected and actual values #3217

kentcdodds commented Mar 28, 2017

kentcdodds Mar 28, 2017

kentcdodds Mar 28, 2017

kentcdodds Mar 28, 2017

aaronabramov Mar 28, 2017

kentcdodds Mar 28, 2017

cpojer Mar 28, 2017

kentcdodds Mar 28, 2017

aaronabramov Mar 28, 2017

kentcdodds Mar 28, 2017 •

edited

aaronabramov Mar 28, 2017

aaronabramov commented Mar 29, 2017

kentcdodds commented Mar 29, 2017

kentcdodds Mar 29, 2017

kentcdodds commented Mar 29, 2017

aaronabramov commented Mar 29, 2017

kentcdodds commented Apr 1, 2017

aaronabramov commented Apr 3, 2017

thymikee commented Apr 5, 2017

kentcdodds commented Apr 5, 2017

thymikee commented Apr 6, 2017

kentcdodds commented Apr 6, 2017

thymikee commented Apr 6, 2017

codecov-io commented Apr 7, 2017

cpojer Apr 11, 2017

cpojer Apr 11, 2017

cpojer Apr 11, 2017

cpojer Apr 11, 2017

thymikee Apr 11, 2017

cpojer Apr 11, 2017

cpojer commented Apr 11, 2017

cpojer left a comment

thymikee commented Apr 11, 2017

cpojer commented Apr 11, 2017

github-actions bot commented May 13, 2021


		assert.deepEqual(received, expected)

		Expected value to deepEqual to:


		assert.notDeepEqual(received, expected)

		Expected value to notDeepEqual to:


		assert.deepStrictEqual(received, expected)

		Expected value to deepStrictEqual to:

Support AssertionError expected and actual values #3217

Support AssertionError expected and actual values #3217

Conversation

kentcdodds commented Mar 28, 2017

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

kentcdodds Mar 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronabramov commented Mar 29, 2017

kentcdodds commented Mar 29, 2017

Choose a reason for hiding this comment

kentcdodds commented Mar 29, 2017

aaronabramov commented Mar 29, 2017

kentcdodds commented Apr 1, 2017

aaronabramov commented Apr 3, 2017

thymikee commented Apr 5, 2017

kentcdodds commented Apr 5, 2017

thymikee commented Apr 6, 2017

kentcdodds commented Apr 6, 2017

thymikee commented Apr 6, 2017

codecov-io commented Apr 7, 2017

Codecov Report

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

cpojer commented Apr 11, 2017

cpojer left a comment

Choose a reason for hiding this comment

thymikee commented Apr 11, 2017

cpojer commented Apr 11, 2017

github-actions bot commented May 13, 2021

kentcdodds Mar 28, 2017 •

edited