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

Proposal: classify uncaught errors in tests as "errors" not as "failures" #5958

Closed
palmerj3 opened this issue Apr 10, 2018 · 14 comments
Closed

Comments

@palmerj3
Copy link
Contributor

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

Feature

What is the current behavior?

Jest treats failed assertions and uncaught errors in a test case both as failures.

What is the expected behavior?

I maintain jest-junit, a testResultsProcessor that provides valid jUnit xml files for CI & reporting systems. This spec differentiates a test failure and a test error.

A test error, according to the junit spec, is classified as:

An errored test is one that had an unanticipated problem, for example an unchecked throwable; or a problem with the implementation of the test.

What I am proposing is that Jest adds an additional test status of "error" for these cases.
The current documentation for testResultsProcessors shows the following test statuses:

"status": "failed" | "pending" | "passed",

I am more than happy to contribute and willing to take this work on myself, if necessary. But before I do I wanted to get feedback from Jest maintainers and the community if this would cause problems.

Please provide your exact Jest configuration

System:
OS: macOS Sierra 10.12.6
CPU: x64 Intel(R) Core(TM) i7-4980HQ CPU @ 2.80GHz
Binaries:
Node: 8.5.0
Yarn: 1.5.1
npm: 5.3.0
npmPackages:
jest:
wanted: ^22.4.3
installed: 22.4.3

@palmerj3
Copy link
Contributor Author

I created a repository that highlights the issue behind this proposal clearly. In it, you'll see that an uncaught error in a test and a failed assertion in a test all result in the same output for a testResultsProcessor.

https://github.com/palmerj3/jesterrorproposalsamplerepo

@rickhanlonii
Copy link
Member

+1 and it would be great to also have "skipped"

@palmerj3
Copy link
Contributor Author

@rickhanlonii yeah jest-junit assumes "pending" is skipped

@SimenB
Copy link
Member

SimenB commented Apr 11, 2018

Might be an issue that we throw from expect, so we'd need some instanceof checks and whatnot. Same with assert, if people use that.

I'm 👍 on the idea, though 🙂

@palmerj3
Copy link
Contributor Author

@SimenB @rickhanlonii @mjesun what are your final thoughts on this?

I'm happy to start working on a PR for this if there is approval but I haven't gotten much reception here.

@SimenB
Copy link
Member

SimenB commented Apr 26, 2018

I think it makes sense 🙂 Richer reporting is always nice. Might be something https://github.com/Raathigesh/majestic can use as well

@rickhanlonii
Copy link
Member

Still +1, but now I'm curious about @SimenB's comment about throwing - we wouldn't want to limit assertions to the Jest expect library so how would we implement this with breaking third party assertions people are using?

One option would be to check for a blacklist of errors like SyntaxError which should not be thrown from matchers. What do you think @palmerj3?

@palmerj3
Copy link
Contributor Author

@rickhanlonii implementation detail :)

If there is general agreement that this would be a nice feature then I'm sure we'll figure it out through the PR process. The blacklisting sounds reasonable.

@SimenB
Copy link
Member

SimenB commented Apr 26, 2018

Using whatever mechanism https://www.npmjs.com/package/bounce uses for system errors seems reasonable to me.

EDIT: I checked, and they have a list of errors using instanceof. We have issues with instanceof, see #2549. But checking constructor.name or something should suffice.

@SimenB
Copy link
Member

SimenB commented Apr 27, 2018

@palmerj3 This seems related to this request for a warn status: jest-community/jest-runner-eslint#21. I wonder if we could somehow make this pluggable, so runners can inject their own "extra" statuses? If not, adding warn or warning or something in addition to error makes sense.

@rogeliog @ljharb thoughts?

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2018

Yes, it would be amazing if runners could inject their own statuses, and colors for those statuses.

Basically, jest is really two things that would be really nice to make more separable: a test framework (a jasmine fork) and a super awesome parallel test runner/watcher.

@palmerj3
Copy link
Contributor Author

palmerj3 commented May 7, 2018

Ok I'm going to consider this community supported :) I have a long plane ride this week so I plan on starting work on it.

@palmerj3 palmerj3 closed this as completed May 7, 2018
@SimenB
Copy link
Member

SimenB commented May 19, 2018

@palmerj3 Were you able to look into it?

FWIW, I support error in addition to failed, but if we could implement it in a way which allows us to also support other statuses, such as warning, that's be awesome.

@github-actions
Copy link

This issue 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 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants