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 coverage of Base reporter cursor #2937

Closed
wants to merge 1 commit into from

Conversation

jsocol
Copy link

@jsocol jsocol commented Jul 28, 2017

This gets coverage of lib/reporters/base.js to over 92% on branches, statements, functions and lines.

I had to add isatty to exports to be able to control it during tests.

@jsf-clabot
Copy link

jsf-clabot commented Jul 28, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@jsocol
Copy link
Author

jsocol commented Jul 28, 2017

The test failures seem like flakes: an ECONNRESET during npm install; a timeout in a run where a lot of tests were slower than normal; a bad tarball header.

I should also probably add a second set of tests, for isatty = false cases.

@ScottFreeCode
Copy link
Contributor

I had to add isatty to exports to be able to control it during tests.

We shouldn't do this unless there's a non-test use case for allowing users to override it.

Otherwise, if we really want to test those branches by forcing isatty to one value or another, we should use something like Proxyquire to do it.

However, I'm inclined to think it would be more correct to cover those branches by testing Mocha in both TTY and non-TTY environments, since this is essentially not part of the interface (except for the --colors/--no-colors override) but just internal compatibility handling.

That may happen automatically between adding coverage to all environments and later switching from the Makefile to NPM scripts (there's a test in the Makefile that seems like it would cover this but is suppressing whether the tests actually pass, but if we changed it into an NPM script with && chains that would eliminate the result suppression and allow it to be added to the overarching test command).

@ScottFreeCode
Copy link
Contributor

(P.S. Thanks for the PR -- we will review the coverage improvements here in general, in any case.)

@jsocol
Copy link
Author

jsocol commented Aug 5, 2017

I was kind of surprised none of this code got executed in any of the reporter tests—even if the statement coverage wasn't 100% I thought the line coverage would be there. That makes me wonder if this just isn't used at all by the built-in reporters? Especially if it's "not part of the interface", maybe it's dead code?

Sorry to leave this hanging, I'm going on vacation for a couple weeks. If you want to go another direction feel free to close this out. It's probably not the right PR to add a TTY/non-TTY harness. Otherwise I'll happily try to help move it along when I get back!

@jsocol
Copy link
Author

jsocol commented Aug 25, 2017

Seems like this is the wrong direction so going to close it to avoid lingering PRs, but let me know if it's worth picking up again!

@jsocol jsocol closed this Aug 25, 2017
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.

3 participants