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

WIP: Reporter Output via Mixins #3874

Closed
wants to merge 4 commits into from
Closed

Conversation

plroebuck
Copy link
Contributor

Description of the Change

Created class mixins to allow a reporter to support multiple output destinations.
Hope would be to eventually make reporters use these mixins to standardize
built-in reporters across the board.

This PR transformed XUnit first, since it already had unit tests dealing with file output.
Would like both JSON and TAP to get the "treatment" soon.

Looking for comments on the approach taken.

Alternate Designs

We could add the same "XUnit" code to each reporter that desired file output.

Why should this be in core?

Magnets!!!

Benefits

Trivializes adding support for file output to another reporter.

Possible Drawbacks

None found in XUnit thusfar.

Applicable issues

semver-minor

Created reporter output mixins for file, stdout, and console.log.
Revamped reporter to use mixin instead, removing output-specific code. Added class method
`XUnit.getSuiteName`. Now uses `XUnit.prototype.writeln`.
Updated to use 'sinon' stubs. No longer screws with `process`. Other additions.
@plroebuck plroebuck added type: feature enhancement proposal area: reporters involving a specific reporter semver-minor implementation requires increase of "minor" version number; "features" labels Apr 13, 2019
@plroebuck plroebuck self-assigned this Apr 13, 2019
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 91.625% when pulling a16d815 on plroebuck/reporter-fileio into f1fe632 on master.

*/
this.writeln = function(str) {
this.write(str);
console.log(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth saving a reference like the timer? (let users stub)

Copy link
Contributor Author

@plroebuck plroebuck Apr 20, 2019

Choose a reason for hiding this comment

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

I hadn't bothered integrating with your PR at this point, but it would be done in this one file for any reporter implementing ReportWriter. It's quite honestly trivial to implement for a reporter; it's writing the unit tests that takes all the time...

var delay = 1000; // One second

this.writeln();
setTimeout(cb, delay);
Copy link
Contributor

@craigtaub craigtaub Apr 19, 2019

Choose a reason for hiding this comment

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

Why does console writer need a timeout? Why 1s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this method is invoked immediately prior to Mocha.run finishing up, the delay is there to (hopefully) allow completion of flushing any buffered (i.e., unwritten) output to the screen. The '1s' time is arbirtrary for the moment; it's rather long for CPU-related tasks, but as this is IO-related I'm uncertain just what a good value would be.

Be happy to change the comment to "arbitrary", as I usually would have...

@craigtaub
Copy link
Contributor

Few questions. Approach makes sense to me.

@boneskull boneskull closed this Apr 30, 2020
@boneskull boneskull deleted the plroebuck/reporter-fileio branch April 30, 2020 23:43
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.

4 participants