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

document and test nyc merge functionality #515

Closed
bcoe opened this issue Feb 6, 2017 · 17 comments
Closed

document and test nyc merge functionality #515

bcoe opened this issue Feb 6, 2017 · 17 comments
Assignees

Comments

@bcoe
Copy link
Member

bcoe commented Feb 6, 2017

@addaleax brought up a couple enhancements to nyc, in a conversation here that would make instrumentation of Node.js itself easier.

The first feature request was functionality similar to istanbul-merge.

nyc already does this internally, by outputting multiple coverage reports into a .nyc_output folder, and then merging them together while reporting. Having said this:

  • we don't document an easy way to do this over multiple test runs (I believe this is what you're looking for essentially @addaleax?).
  • we haven't benchmarked our approach vs. istanbul-merge (apparently istanbul-merge has great performance).

Expected Behavior

An easy, fast, documented, way to collect together multiple coverage.json files into a single report.

Help Wanted

@CurryKitten, @addaleax, do you have a repo we could work from while implementing this; also am I missing anything in this issue?

@addaleax
Copy link
Member

addaleax commented Feb 6, 2017

I think this is pretty much it, yes. Implementation-wise, one thing that I’ve considered is that it might actually make sense to spawn multiple subprocesses for reading the files and merging the coverage reports to have better CPU utilization. (Interestingly, most of the time here was spent running JSON.parse! :o)

do you have a repo we could work from while implementing this

Not sure – I think code-wise pretty much everything we have is in the linked PR.

@bcoe
Copy link
Member Author

bcoe commented May 14, 2017

@yangli1990 this is similar to what you were looking for I believe.

@JaKXz
Copy link
Member

JaKXz commented May 18, 2017

@ljharb I think we were having a conversation about this in a react-dates PR. If I understand this issue correctly this functionality is theoretically possible with nyc... but not much beyond theory is proven, I think.

@ljharb
Copy link
Contributor

ljharb commented May 19, 2017

Awesome! I'd love for istanbul-merge to no longer have to exist; I use it extensively in https://npmjs.com/mocha-wrap and https://npmjs.com/jest-wrap (where i need to run tests in mocha and jest respectively, but where i can't properly test the mocha/jest hooks unless i use tape).

@boneskull
Copy link
Contributor

Mocha could also use this.

@boneskull
Copy link
Contributor

sorry; didn't realize this issue is not "implement such a thing" because it already appears implemented. just needs more tests (I found a problem; see #664) and docs. otherwise, it seems to work!

@Morikko
Copy link

Morikko commented Oct 23, 2018

For those like than struggled to understand the merge functionality, just some precisions.
There are:

  1. nyc merge (only output json)
  2. nyc report (do it implicitly)

In my company, we get the coverage on a running server and on Jest. Then, we move the resulting coverage files to the same folder called for example coverage-full.

Finally, you just have to run npx nyc report --report-dir=where-to-export-results/ --reporter=html -t coverage-full

nyc will read all the json files and merge them to a new one.

@ljharb
Copy link
Contributor

ljharb commented Jan 5, 2019

bump; staleness should be irrelevant to issues staying open.

@stale stale bot removed the wontfix label Jan 5, 2019
@bcoe
Copy link
Member Author

bcoe commented Jan 6, 2019

@ljharb I disagree, my experience with yargs and nyc has been that:

  1. many issues stay open without a response or reproduction from the person who opened them.
  2. may have been fixed by updates, but missed being updated.
  3. having a huge number of open issues is overwhelming an hurts the overall health of a project.

I think using stale bot is worth a shot, and will hopefully do a better job of keeping conversation on repos centered around active work.

@ljharb
Copy link
Contributor

ljharb commented Jan 6, 2019

It’s your project, but i find number 3 very false, and find bots closing issues immensely harmful to users and thus the project itself.

@bcoe
Copy link
Member Author

bcoe commented Jan 6, 2019

this feature is implemented.

@bcoe bcoe closed this as completed Jan 6, 2019
@bcoe
Copy link
Member Author

bcoe commented Jan 6, 2019

@ljharb you're entitled to your opinion, but I'd like to experiment with an approach to managing GitHub conversations on Istanbul that keeps conversation centered around active development. If this approach works, will consider moving forward with it on other projects ... if it doesn't seem to make development more vibrant, will move away from the approach.

@JaKXz
Copy link
Member

JaKXz commented Jan 6, 2019

@ljharb FWIW, as a maintainer/triager it's overwhelming when there are many issues open because I don't know at a glance what's most important to people since I'm not on the project full time. In the short of amount of time since adding the stale bot we've seen some healthy movement IMHO because the people who have the most vested interest and are actively contributing are responding and helping move the project forward.

@ljharb
Copy link
Contributor

ljharb commented Jan 6, 2019

Labels serve that purpose without sending users the message that you don’t care about their issue.

@JaKXz
Copy link
Member

JaKXz commented Jan 6, 2019

I don't think that's the message being sent. In fact, labels don't create a notification, which in turn can lead to issues being forgotten about (again, when this isn't your full time job), which sends the message that the user's issue isn't cared about. Stale bot does the opposite, IMHO, it says I care enough to follow up or ask for more information before I go to prioritize more active development.

We could probably bikeshed our different opinions forever, so I'm going to stop here. I did want to explicitly address

you don’t care about their issue.

and basically rebuke that. We do care about these issues, and are taking steps to get to as many as are reasonable that we are able to get to.

@coreyfarrell
Copy link
Member

@JaKXz I think it would be good to change the label. I think stale would be a better label, wontfix should be reserved for cases where we are explicitly rejecting an issue or PR.

@sindresorhus
Copy link
Member

I also find the use of "auto-closing issue" bots harmful. If you want to encourage responses from users, add a "ping" bot instead. But naively closing issues just leads to a lot of annoyance for users.

Relevant Twitter thread: https://twitter.com/sindresorhus/status/1082145283285250048

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants