Don't try to cover the bin/nyc.js file

[isaacs]

This makes it possible to self-test node-tap's coverage reporting and
coverage level checking when running in covered mode. Otherwise, doing
node {nycBin} check-coverage results in executing the nyc executable
script as the main file (or worse, some module named check-coverage,
which obviously doesn't exist).

[isaacs]

Added another 2 commits with a cleaner (imo) approach.

Now coverage should not interfere with running nyc itself, and nyc can cover things with report or check-coverage in the arguments.

Still not sure why it doesn't work to cover nyc with nyc, though.

[jamestalmage]

Still not sure why it doesn't work to cover nyc with nyc, though.

We moved away from that because there are just too many (potentially) overlapping parts:

1. Multiple spawn wraps.
2. Installing the require extension multiple times.
3. NYC generating reports on itself (messing up tests that count the number of tests created by a test fixture)
4. NYC_CWD environment conflicting with the one we want when we move into the fixture directory.

Too many potential traps. It's easier to just use the manual method we currently are.

[isaacs]

Fair enough.

But I still want to have node-tap be able to spawn nyc and have it able to run a report or coverage-check while coverage is enabled, so that I can get more test coverage of node-tap.

You could get around the conflicting NYC_CWD environ by not relying on an environment variable for that. If a positional argument was passed to spawn-wrap, it'd be able to have as many of those as necessary, and un-wrap at each level. Even if it was wrapped 50 times, it'd get progressively slower of course, but the wrapping would be fine, and could be using a different NYC_CWD at each level.

Nesting require extensions would be tricky, but wasn't there something in the works to support that anyway, for babel and such?

[jamestalmage]

Nesting require extensions would be tricky, but wasn't there something in the works to support that anyway, for babel and such?

It's in already (append-transform).
Prior to that fix we would actually clobber the old require extension. The new behavioral actually enables/honors multiple installations (thereby creating the problem).

[jamestalmage]

But I still want to have node-tap be able to spawn nyc and have it able to run a report or coverage-check while coverage is enabled, so that I can get more test coverage of node-tap.

That makes sense. This enables that, correct?

[bcoe]

@jamestalmage if this pull makes it easier to test tap, I see no reason not to land it; I'd like to test thoroughly though -- requiring that report occur at position 0 now concerns me the most, probably the correct logic, but is this a breaking change now I wonder?

[jamestalmage]

I'm not opposed to landing it. I was just providing background.

I haven't reviewed yet (on mobile). So I can't comment on if it's a breaking change. But if what you say is true, then yeah.

[isaacs]

requiring that report occur at position 0 now concerns me the most, probably the correct logic, but is this a breaking change now I wonder?

Yargs will already pull out --flags from the _ array. So this would only change nyc check-coverage report from being a report command to a check-coverage command, and allow running things like nyc npm run report for example.

[jamestalmage]

LGTM.

I like the use of wrap.js.

[isaacs]

@bcoe lgty?

[bcoe]

@isaacs smoke tests look good, merging; I'll let you know once a release has gone out.

[bcoe]

@isaacs if you give the next branch of nyc a spin, this work is now landed:

npm i nyc@next