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

Handle when an error occurs in a beforeEach block of a mocha test. #9

Merged
merged 7 commits into from Jan 5, 2018

Conversation

hswolff
Copy link
Contributor

@hswolff hswolff commented Dec 20, 2017

Hello, it's me again! Apologies for the PR's without issues!

This adds support to surface when an error occurs in a beforeEach block, before any individual tests run.

Without this change this is what is shown:

image

With this change we show:

image

runner.on('fail', test => failures.push(test));
runner.on('fail', (test, err) => {
test.err = err;
failures.push(test);
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should make them all Set instead of Array, and then simply call tests.add(test) here

Copy link
Owner

@rogeliog rogeliog left a comment

Choose a reason for hiding this comment

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

Looks good apart of a small comment

@rogeliog
Copy link
Owner

Thanks for reporting and sorry for the late response, I was sick

@hswolff
Copy link
Contributor Author

hswolff commented Dec 27, 2017

No worries about being sick! Appreciate the update!

Just pushed change to using Set, let me know if that's what you had in mind otherwise I'm happy to update it to your liking.

src/runMocha.js Outdated
const pending = [];
const failures = [];
const passes = [];
const tests = new Set();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mocha 3 works down to node versions that lack Set entirely; if we’re going to use it, let’s be sure to be using a polyfill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gadzooks, mocha 3 supports all the way back to node 0.10.

My personal preference would be to either:

  • Revert this last commit and keep functionality w/o Set
  • Bump mocha to v4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The former is much better.

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 shall await @rogeliog's decision. (This was an async comment, cuz I await'ed in it. I hope my humor transcends the coldness of this comment box. :))

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with @ljharb

@hswolff
Copy link
Contributor Author

hswolff commented Jan 4, 2018

Reverted the change to Sets for node compatibility.

I also included another small change that hopefully is ok to include in this PR. I noticed (when trying to use https://github.com/palmerj3/jest-junit) that the type of failureMessages wasn't what that library was expected.

I did some looking and the type object that is defined in Jest also expects the property to be an Array. So I fixed that here too.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM

@rogeliog rogeliog merged commit b293dd8 into rogeliog:master Jan 5, 2018
@rogeliog
Copy link
Owner

rogeliog commented Jan 5, 2018

Thanks a lot!

@hswolff hswolff deleted the errors-in-before-each branch January 6, 2018 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants