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

XUnit reporter should handle exceptions during diff generation #4068

Merged
merged 1 commit into from Dec 22, 2019
Merged

XUnit reporter should handle exceptions during diff generation #4068

merged 1 commit into from Dec 22, 2019

Conversation

rgroothuijsen
Copy link
Contributor

@rgroothuijsen rgroothuijsen commented Oct 14, 2019

Description of the Change

This bugfix addresses a crash in the XUnit reporter, which produced no output because the assertion library used in the ticket provided numerical diff values rather than string values, causing an error during diff generation. To safeguard against unexpected crashes such as these, the same input-checks that the Base reporter uses have been implemented in the XUnit reporter, and exceptions during XUnit diff generation are returned by the reporter.

Applicable issues

Fixes #4022.

@jsf-clabot
Copy link

jsf-clabot commented Oct 14, 2019

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Oct 14, 2019

Coverage Status

Coverage increased (+0.01%) to 92.913% when pulling a43def2 on rgroothuijsen:MOCHA-4022 into d9f5079 on mochajs:master.

@craigtaub craigtaub added type: bug a defect, confirmed by a maintainer area: reporters involving a specific reporter labels Nov 23, 2019
@craigtaub
Copy link
Contributor

craigtaub commented Nov 23, 2019

Managed to replicate myself. The fix LGTM. @mochajs/core any other thoughts?

@juergba
Copy link
Member

juergba commented Nov 27, 2019

When this exception is thrown in XUnit reporter, the output is truncated and aborted without any error message. That exception shouldn't be swallowed.

Could be related to #980.

@rgroothuijsen
Copy link
Contributor Author

@juergba I'm not sure if I understood entirely. Is this particular fix incorrect? Is there something I should add/modify?

@juergba
Copy link
Member

juergba commented Dec 1, 2019

This issue has two parts. The diff exception and then second the reporter aborting without any message.
It would be helpful to know the cause of the second part in order to decide wether this PR is complete.

@rgroothuijsen
Copy link
Contributor Author

@juergba Uncaught exceptions are intercepted by the runner at this point. Looking at the method in question, I see that the exception is logged, but only at the debug log level.

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@rgroothuijsen thank you.

I'm not happy mainly with the existing implementation of diff in XUnit reporter.

  • Base.generateDiff: exceptions are not catched and propagate upto the uncaughtException handler.
  • condition when to generate diff is much softer in XUnit than in Base. eg. there is no check for identical type of actual and expected.
  • I'm unsure wether this PR is IE11 compatible.

lib/reporters/base.js Outdated Show resolved Hide resolved
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

We have an EVENT_TEST_FAIL handler in Base which calls stringifyDiffObjs. Since XUnit extends Base, this handler is called also for XUnit.
So the problem is not the missing stringifyDiffObjs in XUnit, but the conditions when to generate a diff, are not identical in Base and XUnit.

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

I added some change requests to your code:

  • we catch an exception while generating diff
  • we stringify only in the fail event handler
  • we synchronize conditions when diffs should be generated over Base, XUnit and fail event handler.

I haven't tested my proposition, so if you find anything, please go ahead ...

lib/reporters/xunit.js Outdated Show resolved Hide resolved
test/reporters/xunit.spec.js Outdated Show resolved Hide resolved
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

Those mocking tests are quite complex, so please check my code carefully.

lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/xunit.js Outdated Show resolved Hide resolved
test/reporters/xunit.spec.js Show resolved Hide resolved
test/reporters/xunit.spec.js Show resolved Hide resolved
@juergba juergba added this to the v7.0.0 milestone Dec 21, 2019
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@rgroothuijsen thank you!
Could you please rebase, squash with your commit message and edit/update this PR's description?

@rgroothuijsen
Copy link
Contributor Author

@juergba Will do. In retrospect, do you think the title still works for this PR?

@juergba
Copy link
Member

juergba commented Dec 22, 2019

@rgroothuijsen No, not really. Maybe "XUnit reporter: fix crash during diff generation" or similar, but the title is upto you.

@rgroothuijsen rgroothuijsen changed the title XUnit reporter should handle non-string diff values XUnit reporter should handle exceptions during diff generation Dec 22, 2019
@juergba juergba merged commit 1412dc8 into mochajs:master Dec 22, 2019
@juergba juergba added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Dec 22, 2019
@rgroothuijsen rgroothuijsen deleted the MOCHA-4022 branch December 22, 2019 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reporter xunit output report incomplete
5 participants