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

feat(coverage): add code coverage reporting #56

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

lkipke
Copy link
Contributor

@lkipke lkipke commented Sep 11, 2020

Change Summary

This allows for code coverage reporting. It gets coverage data using the brs API from sjbarag/brs#525, and generates the reports using IstanbulJS.

@lkipke lkipke added enhancement New feature or request brs Root cause is in the brs intepreter labels Sep 11, 2020
@lkipke lkipke self-assigned this Sep 11, 2020
Copy link
Contributor

@alimnios72 alimnios72 left a comment

Choose a reason for hiding this comment

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

Sweet deal

Copy link
Contributor

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

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

Great work! Let's think about how we can add some tests for our Istanbul integration in the future 😉

});

reporterStream.end();

if (coverageEnabled) {
coverage.report(coverageReporters);
Copy link
Contributor

Choose a reason for hiding this comment

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

This might cause issues for users who provide --reporter tap, --reporter json, or --reporter json-stream, since it causes stdout to contain both TAP (and TAP-in-JSON) and coverage output. I don't mind that behavior for roca — especially for our initial pass — but I'm curious how something like mocha handles that!

The biggest practical impact is that running roca --coverage-reporters=html > coverage-report.html won't produce a valid HTML file.

Copy link
Contributor Author

@lkipke lkipke Sep 14, 2020

Choose a reason for hiding this comment

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

Hmmm interesting. A couple of thoughts: if you didn't want TAP output to pollute coverage output, you could do --reporter silent (i agree that's an interim solution, not long term). But also, running --coverage-reporters=html actually generates a coverage/ folder in the project root, and the coverage report is written to coverage/index.html. So I don't know that there's necessarily a use case where you would need to pipe stdout to an html file! Of course, you may want to pipe other output formats, such as --coverage-reporters=text, which would still be an issue.

But yeah, i think this should work for a first pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

But also, running --coverage-reporters=html actually generates a coverage/ folder in the project root, and the coverage report is written to coverage/index.html.

Heh I actually didn't know it produced a separate coverage/ directory. --coverage-reporters=text is exactly the use-case I was thinking of 🙂

@lkipke lkipke merged commit 224e7a8 into hulu:master Sep 14, 2020
@lkipke lkipke deleted the features/add-coverage-reporter branch September 14, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brs Root cause is in the brs intepreter enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants