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 the assert module more cleanly #3173

Closed
kentcdodds opened this issue Mar 18, 2017 · 25 comments · Fixed by #3217
Closed

Support the assert module more cleanly #3173

kentcdodds opened this issue Mar 18, 2017 · 25 comments · Fixed by #3217

Comments

@kentcdodds
Copy link
Contributor

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

Jest doesn't output a very good message when using assert.equal from node's built-in assert module when you compare it to other testing frameworks like Mocha. This is a problem for me because ESLint's tester uses the assert.equal for its error messages.

What is the expected behavior?

I want it to output the error.expected and error.actual like Mocha does.

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.

I'll do you one better. Here's a repo: https://github.com/kentcdodds/jest-assert

And here's a screenshot:

screen shot 2017-03-18 at 8 23 24 am

Reference: eslint/eslint#8173

@kentcdodds
Copy link
Contributor Author

I'm willing to implement this if I could get some direction.

@thymikee
Copy link
Collaborator

thymikee commented Mar 19, 2017

This one needs patching our test runner. Which means, we would need to further alter Jasmine code (which is ok I guess, because we're currently rewriting it). So, I would wait for @DmitriiAbramov to weigh in on this before sending a PR.

The code you'll likely need to change sits here (be aware that @wtgtybhertgeghgtwtg is going to refactor this module to a class). You'll need to adjust the error and build similar messages to these. Formatting methods like matcherHint or printExpected will be available, since jest-jasmine2 depends on jest-matcher-utils.

@aaronabramov
Copy link
Contributor

ah yeah. i ran into the same issue with eslint a while ago. and yes, we need to catch assertion errors (they're pretty easy to distinguish from all other errors) and pretty-print/re-throw them with better formatting!

previous issues:
#2237
#1230

@kentcdodds
Copy link
Contributor Author

I'm working on this right now (just fudging with my local node_modules to start) and I'm finding that jest-jasmine2 has an odd sort of build or something because even though the file you referenced is src/jasmine/Spec.js, the file that code is in is vendor/jasmine-2.5.2.js and I'm unable to require anything in it (require is undefined). I need to get diff (from jest-diff), matcherHint, printExpected, and printReceived. Any tips on a good workflow for developing this?

@kentcdodds
Copy link
Contributor Author

Also, when I add a message to the addExpectationResult, I don't see any change in the output, but if I change the matcherName to some value other than the empty string it is now, then I see the message I provided. Sounds like black magic. Any tips on what I should do there?

@kentcdodds
Copy link
Contributor Author

Oh, and also jest-jasmine2 does not currently depend on jest-diff so I'll need to add that to the dependencies. Any reasons to not?

@thymikee
Copy link
Collaborator

I'm working on this right now (just fudging with my local node_modules to start)

Oh, please don't do it to yourself! You'll be better of forking Jest and linking it (instructions in CONTRIBUTING.md), because since last release we rewritten our Jasmine fork to be modular.

@kentcdodds
Copy link
Contributor Author

HOORAY!!!! Thank you @thymikee, @DmitriiAbramov, and @cpojer! This is going to make my ASTs Workshop soooo much better. Developing the fixer function for ESLint will be much improved. Thank you thank you :)

On that note, when's the next scheduled release? 😸

@thymikee
Copy link
Collaborator

@cpojer I think we should start making prereleases (e.g. from master), like Yarn does.

@cpojer
Copy link
Member

cpojer commented Apr 11, 2017

On board with pre-releases but the next major for Jest may be in a month from now unfortunately.

@kentcdodds
Copy link
Contributor Author

Ah, too bad. If I could get a pre-approved release with this that'd be great 😀

@oliviertassinari
Copy link

Anyone aware if that feature is still working? I'm seeing AssertionError: XXX error in my console when using assert, not the fancy diff.

import assert from 'assert'

@aaronabramov
Copy link
Contributor

@oliviertassinari it should work. we have a snapshot test for these errors.
can you provide a repro case?

@kentcdodds
Copy link
Contributor Author

I can verify that it works for me.

@oliviertassinari
Copy link

Yes, I can try to provide a reproduction, I have also tried to use it with power-assert with no luck (I was curious).

@aaronabramov
Copy link
Contributor

@oliviertassinari do you mind opening an issue with the repro case?

@oliviertassinari
Copy link

oliviertassinari commented Jun 27, 2017

I couldn't reproduce it on a simple example. That's just working. I must have something on my project breaking this feature. Thanks for the answer. I'm gonna dig into it. Will eventually use the expect API otherwise.

@v1adko
Copy link

v1adko commented Aug 2, 2017

@kentcdodds, this is great, thank you!

How much time and effort do you think it's gonna require to handle chai assertion errors similarly? Or does it need to be done on the chai side and not jest?

Currently what we have with common-js assert:

In chai-js assert:

And the thrown objects seem very similar

Common:

Chai:

I am also willing to work on this with some direction. I could probably use your #3217 as a reference.

@kentcdodds
Copy link
Contributor Author

I expect it would be a matter of slightly modifying the code to support assert. So hopefully pretty straightforward 👍

@aaronabramov
Copy link
Contributor

it seems like this whole system should be pluginized, because it can get out of control if we start adding all kind of use cases :)
we should probably add something like

jest.addErrorFormatter(assertFormatter);
jest.addErrorFormatter(chaiFormatter);

@aaronabramov
Copy link
Contributor

or maybe rather specify it in the config file

jest: {
  errorFormatters: ['jest-error-formatters-chai', 'jest-error-formatters-assert']
}

@v1adko
Copy link

v1adko commented Aug 2, 2017

@aaronabramov Yeah, plugins are a nice idea. 👍
I like the explicit config better, similar to eslint and babel plugins - feels more natural, I think.

@G-Rath
Copy link
Contributor

G-Rath commented Mar 18, 2019

Whatever happened to this?

I'm trying out Jest for the first time, and the biggest pain for me is that when using Mocha + Chai I'd be able to see the actual difference for things like asserting array elements, and IntelliJ would provide me a diff based on the results.

In switching to Jest, I just have a single line in the documentation to go:

If you like chai, you can upgrade to Jest and continue using chai.

and error messages that are not even close to the detail I had with Mocha + Chai:

AssertionError: expected true to equal false
   < ... ugly stacktrace ... >

&

AssertionError: expected [ Array(10) ] to be a superset of [ 'line 1', 'line 2' ]
      < ... ugly stacktrace ... >

From my understanding, the issue lies with Jest not taking any advantage of information provided by Chai (if it's important, I'll try and dig up the Chai issue about this).

I think an errorFormatters plugin system makes perfect sense, but can't see any issues or PRs linked to this one?

This is going to be the biggest stopper I think for me to not pick up Jest :)

@SimenB
Copy link
Member

SimenB commented Mar 18, 2019

We have no special handling of chai, no. Can you open up a new feature request? This issue is about assert, which we do handle

@G-Rath
Copy link
Contributor

G-Rath commented Mar 18, 2019

Sorry about that! I just saw people talking about chai near the end, and figured the proposed solution overlapped with both chai & assert 😅

I've made #8152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants