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

Add output option to JSON reporter (#4131) #4607

Merged
merged 2 commits into from Aug 20, 2021

Conversation

dorny
Copy link
Contributor

@dorny dorny commented Mar 14, 2021

Extends JSON reporter with option to write output directly to file instead of a console.

closes #4131

Description of the Change

  • If output option was provided to JSON reporter:
    • On EVENT_RUN_END it will write JSON output to given file - single call to fs.writeFileSync()
    • Directory structure will be recursively created if needed - fs.mkdirSync()
    • If test runs in browser it will throw unsupported error
  • Unit tests has been updated
    • Verifies if reporter writes to given file same JSON content as it sets on the runner instance
    • Verifies if reporter throws error if output option is provided and test runs in browser
    • Diff is bigger unless you hide whitespace only changes because existing tests were put into describe() group
  • Documentation has been updated

Alternate Designs

xunit reporter uses write stream and produces output continuously during test execution.
Similar behavior is provided by json-stream reporter.
json reporter outputs a single json when the tests have completed so I've used single call to fs.writeFileSync() instead of creating write stream.

I've also considered refactoring of json.spec.js.
Ideally unit test should execute JSONReporter directly instead of using real Runner instance.
Also tests should capture standard output and run assertion on it instead of using testResults property on runner which doesn't look to be used anywhere else.

I've tried to rewrite tests using createMockRunner() from test helpers but it didn't work.
Mock runner doesn't supports simulation of multiple events and reporter state is not preserved between multiple runs.
Therefore it was not possible to use it for json reporter without bigger changes.
I still think refactoring the tests would improve code quality but it is out of scope of this PR.

Why should this be in core?

  • It's a common feature, widely supported in other test frameworks (JavaScript or other languages)
  • Same option is already supported in xunit reporter
  • JSON output is usually processed by another tool and any noise on standard output would break it

Benefits

Compared to redirecting report output to file using shell pipe:

  • valid JSON will be written to file even if some test produces noise on standard output
  • together with mocha-multi-reporters enables having both console output and file output

@coveralls
Copy link

coveralls commented Mar 14, 2021

Coverage Status

Coverage increased (+0.02%) to 94.403% when pulling eff4daa on dorny:issue/4131-add-json-output-option into bbf0c11 on mochajs:master.

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.

@dorny thank you for this PR.
Yes, json.spec.js could benefit from some refactoring ...
I just have one remark, otherwise looks like a very diligent work.

Edit: the error is swallowed only for a failing test. For a passing test the error is printed to the console. This is a bug in our uncaught exception handler, which I will investigate later.
Edit II: no, it doesn't matter, whether the test fails or passes, the error gets swallowed.

lib/reporters/json.js Outdated Show resolved Hide resolved
@juergba
Copy link
Member

juergba commented Mar 18, 2021

The root of the error being swallowed is, that we have two EVENT_RUN_END listeners, with incorrect order:
a) one is registered within the runner and sets runner.state to stopped (should run first)
b) one is registered within the reporter (runs first, should be second)

So I see three options:

  • we use try/catch in json reporter
  • we use emitter.prependListener for registering a), but I'm unsure about browser compability.
  • we delay the execution of b) with setImmediate()

Edit: or we move the file operations to reporter.done.

@juergba
Copy link
Member

juergba commented Mar 20, 2021

Yes, and there is Mocha in parallel mode, quite a recent feature. I'm unsure about the implications, but our docs states:
In parallel mode, reporter output is buffered; reporting will occur after each file is completed.

We have to make sure there will be only one file created.

@dorny
Copy link
Contributor Author

dorny commented Mar 20, 2021

@juergba thanks for your support. Right now I'm recovering from eye surgery so I've to reduce my screen time significantly for next few days. I will get back to this afterwards. Meanwhile feel free to modify PR yourself if you find solution.

@github-actions
Copy link

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

@github-actions github-actions bot added the stale this has been inactive for a while... label Jul 19, 2021
@github-actions github-actions bot closed this Aug 2, 2021
@juergba juergba reopened this Aug 17, 2021
@juergba juergba added type: feature enhancement proposal area: reporters involving a specific reporter semver-minor implementation requires increase of "minor" version number; "features" and removed stale this has been inactive for a while... labels Aug 17, 2021
@juergba juergba force-pushed the issue/4131-add-json-output-option branch from 5d18965 to 6a93d47 Compare August 17, 2021 13:56
@juergba juergba force-pushed the issue/4131-add-json-output-option branch from d5162f3 to eff4daa Compare August 18, 2021 14:22
@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label Aug 18, 2021
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Aug 18, 2021
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.

@dorny thank you

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-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add output option to JSON reporter
3 participants