Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support AssertionError expected and actual values #3217

Merged
merged 9 commits into from
Apr 11, 2017

Conversation

kentcdodds
Copy link
Contributor

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:

screen shot 2017-03-28 at 7 12 17 am

And here's after:

screen shot 2017-03-28 at 7 12 04 am

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.

this.addExpectationResult(
false,
{
matcherName: '',
matcherName: 'blah blah',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

screen shot 2017-03-28 at 7 25 28 am

screen shot 2017-03-28 at 7 25 40 am

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.addExpectationResult(
false,
{
matcherName: '',
matcherName: 'blah blah',
message,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

const {expected, actual} = e;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes people do strange things like

throw null;

i wonder if Jest will blow up here

Copy link
Contributor Author

@kentcdodds kentcdodds Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.addExpectationResult(
false,
{
matcherName: '',
matcherName: 'blah blah',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

● assert.deepEqual

Error
expect(received).toBe(expected)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kentcdodds
Copy link
Contributor Author

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

@aaronabramov
Copy link
Contributor

@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:

screen shot 2017-03-29 at 3 22 10 pm

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
Copy link
Contributor Author

That sounds great. Any suggestions on the coloring issue?

@aaronabramov
Copy link
Contributor

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
Copy link
Collaborator

thymikee commented Apr 5, 2017

@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:

screen shot 2017-04-06 at 00 30 09

@kentcdodds
Copy link
Contributor Author

Oh awesome, thank you @thymikee!

@thymikee
Copy link
Collaborator

thymikee commented Apr 6, 2017

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:

screen shot 2017-04-06 at 11 14 36

This is how I print custom message assertion message:

screen shot 2017-04-06 at 11 15 39

And more complex matchers:

screen shot 2017-04-06 at 11 17 49

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:

screen shot 2017-04-06 at 11 20 18

@kentcdodds
Copy link
Contributor Author

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

@thymikee
Copy link
Collaborator

thymikee commented Apr 6, 2017

Got support for throwing matchers:

screen shot 2017-04-06 at 18 10 45

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

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            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.


assert.deepEqual(received, expected)

Expected value to deepEqual to:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Expected value to deeply equal:"

maybe?


assert.notDeepEqual(received, expected)

Expected value to notDeepEqual to:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"not to deeply equal"?


assert.deepStrictEqual(received, expected)

Expected value to deepStrictEqual to:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to deeply and strictly equal"?

But it didn't throw anything.

Message:
Missing expected exception..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is coming from assert, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cpojer
Copy link
Member

cpojer commented Apr 11, 2017

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).

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@thymikee
Copy link
Collaborator

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

@cpojer
Copy link
Member

cpojer commented Apr 11, 2017

Let's do it.

@cpojer cpojer merged commit 0d73c53 into jestjs:master Apr 11, 2017
@kentcdodds kentcdodds deleted the pr/assert branch April 11, 2017 13:30
skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017
* 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
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* 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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support the assert module more cleanly
6 participants