feat: support for instrumenting files outside of current working directory

[j03m]

We're currently leveraging the index.js programmatically via require("c8") for a build system that is bit non standard in that multiple dist directories for various projects can be leveraged in a workspace that is outside the cwd of where the test are run.

This change modifies the interface to Report but not the CLI in the following ways:

• It allows src to be supplied as a Array of string paths.
• If src isn't supplied, src is overridden to the default cwd, the main CLI doesn't currently allow these directories to be supplied to avoid over complicating the CLI interface.
• An additional flag for allowExternal is supplied and passed to test-exlude to indicate that not all paths will be relative. Not supplying this flag causes test-exclude to reject anything outside of cwd.

Please don't merge this just yet as I'm still testing, writing tests. But I wanted to get your thoughts if this was a feasible enhancement.

Edit: Okay I think this is ready to go.

Checklist

• npm test, tests passing
• npm run test:snap (to update the snapshot)
• tests and/or benchmarks are included
• documentation is changed or added

[coveralls]

Pull Request Test Coverage Report for Build 570

• 44 of 45 (97.78%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 94.413%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/source-map-from-file.js	1	2	50.0%

Totals
Change from base Build 556:	0.2%
Covered Lines:	574
Relevant Lines:	597

---

💛 - Coveralls

[bcoe]

This looks like a great start, just let me know when you're ready for review.

[j03m]

@bcoe I think this is ready to go. Let me know what you think.

[bcoe]

@j03m awesome, I'll work on reviewing this weekend hopefully.

[j03m]

@bcoe rebased

[j03m]

I will investigate the cause of these test failures.

[j03m]

@bcoe it looks to me like node 10 tests are broken on master. I'm looking at it separately.

That wasn't the case, I was just trying to test with v10.0.0 when I matched the CI it was fine.

[j03m]

Looks like some of these numbers changed as we use test/*.js as the glob. So I updated the snap and adjusted the threshold in a test to keep the spirit of the test correct.

[j03m]

@bcoe this is in good state again.

[bcoe]

@j03m this is looking pretty reasonable to me. If I'm following, does this PR make it so that folders outside of cwd will be tracked, both if they're executed normally, or if they're found via --all? Or did this behavior already work when files were actually covered, but not for --all?

If this PR is adding support for both, perhaps we should add a test for the case where the files in multidir1 and multidir2 are actually covered?

---

I also think that it might be worth thinking about how we would expose this behavior from the CLI, but I'd be willing to land this, as long as we have a tracking ticket to do this work.

[j03m]

I believe it to be both and I think your instincts about the test case are correct.

There are two "levers" so to speak that this change adds:
https://github.com/bcoe/c8/pull/206/files#diff-b52768974e6bc0faccb7d4b75b162c99R20

First is the src array and second is the allowExternal.

src is primarily used to expand the surface area of where I will look for files to include with --all during the all pass. It allows me to pass in say multiple workspace directories that are outside of my cwd.

allowExternal is unlocking behavior inside test-exclude https://github.com/bcoe/c8/pull/206/files#diff-9afa048c4e54cc16b510e587c5833099R36. Inside of test-exclude there is a block code where we check if the current file is outside of cwd if it is, then it will be automatically ignored when we test via shouldInstrument. See: https://github.com/istanbuljs/test-exclude/blob/master/index.js#L87.

The allowExternal flag allows us to disable that block of code so that paths we find inside of v8 json blobs that are also outside of cwd can be included in the report.

I will add the additional tests.

[j03m]

Once I started writing the additional tests, I realize this would be much more seamless if I did a full integration pass.

As suchI took a stab at what the ux for the cli would look like as well. I will push it as a second commit. Seems managable.

I scaffolded this in and am working out tests .

Any thoughts on two additional options and wording:

 .options('allowExternal', {
      default: false,
      type: 'boolean',
      describe: 'supplying --allowExternal will cause c8 to allow files from outside of your cwd. This applies both to' +
        'files discovered in coverage temp files and also src files discovered if using the --all flag.'
    })
    .options('src', {
      default: undefined,
      type: 'string',
      describe: 'supplying --src will override cwd as the default location where --all looks for src files. --src can be ' +
        'supplied multiple times and each directory will be included. This allows for workspaces spanning multiple projects'
    })

I wrestled with the idea of calling the 2nd option src for a while. However considering recent conversations about src vs executable and knowing that we want to land a PR from @robpalme soon re: #224 I think that src is the right name as it will carry foward.

[j03m]

Pushed but looks like node10 failed. Investigating (passed locally?)

[j03m]

My failures on node10 seemed to be related to tmp files from previous runs hanging around. I added a clean to some of my tests that seem to be impacted, but I'm noticing many tests have an explicit flag to clean=false. I was wondering why that is?

[bcoe]

@j03m historically, I believe, the test suite was doing a worse job of isolating its own coverage from the coverage being tested. I think the idea of clean=false was that you wouldn't delete c8's own temporary files when testing behavior -- I think we now address this problem by specifying a different /tmp directory, and should be fine.