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

Feature Request: Instrument option to generate a baseline coverage file #1202

Open
AndrewFinlay opened this issue Oct 10, 2019 · 11 comments
Open

Comments

@AndrewFinlay
Copy link
Contributor

Problem

Currently when testing with files created by the standalone instrument command, if a file isn't hit it will not produce a coverage output file. This can lead to untouched files being excluded from the coverage report. Istanbul handled this by being able to produce a baseline coverage file with content similar to that produced by running nyc --all=true /usr/bin/true. When the baseline coverage file is included with test generated coverage files and fed to the reporter, files that aren't hit during tests are included in the report.

In hindsight I think this was the problem being faced in #1044.

Proposal

Add an option to the instrument command, 'baseline', that will produce a baseline coverage output json file similar to that produced by running nyc --all=true /usr/bin/true.

Workaround

You can generate a report that includes untouched files with:

  1. run nyc --all <options> /usr/bin/true against the uninstrumented code
  2. store the output coverage file
  3. instrument the source
  4. run tests
  5. combine baseline and test coverage files
  6. report on combined coverage files
@coreyfarrell
Copy link
Member

Could you test this with nyc 15 beta? In nyc 15 it should be possible to run nyc --all=true your-test-runner on instrumented code. nyc 14 did not support loading --all coverage from already instrumented files but this should be fixed in nyc 15 (among other fixes to the handling of --all).

@AndrewFinlay
Copy link
Contributor Author

I'm using the instrumented source as input to an e2e testing setup so I can't wrap your-test-runner in nyc unfortunately.

I'm currently working with 15 beta, and nyc --all=true /usr/bin/true works to generate a baseline coverage file on uninstrumented source.
It's a little flaky running the same command on already instrumented source and seems to not report a few files in the project I'm working with.

@AndrewFinlay
Copy link
Contributor Author

For completeness the old istanbul instrument switch was --save-baseline.

@coreyfarrell
Copy link
Member

Thanks for explaining I understand now. I'm open to such a feature as long as it's opt-in, --save-baseline seems like a fine idea.

It's a little flaky running the same command on already instrumented source and seems to not report a few files in the project I'm working with.

I would be interested in seeing an example of this. Running nyc --all=true /usr/bin/true should capture 0% coverage for all files.

@AndrewFinlay
Copy link
Contributor Author

Cool, I'll try and put a PR together soon to add --save-baseline.

Regarding the flakiness, I've tested this again this morning and can confirm there is a difference in behaviour between establishing a baseline against raw code and pre-instrumented code. It'll take a little while to put together a demonstration repo as I've only noticed this on our production source so far.

There seems to be two issues here,

  1. When running nyc --all=true <options> /usr/bin/true against already instrumented source some files are missed.
  2. Of the files that aren't missed when running nyc --all=true <options> /usr/bin/true there is a loss of fidelity wrt number of statements, branches, functions and lines as everything has been put in its instrumented form.

I'm not too sure what I expect nyc to do when running this baseline coverage task against pre-instrumented source though. How do you expect it to behave?

@coreyfarrell
Copy link
Member

When nyc --all=true is run I expect it to capture the correct coverage from pre-instrumented sources. This is a new thing for nyc@15 and I'm not really surprised that some edge cases do not work. I'd love to identify and fix those edge cases.

@AndrewFinlay
Copy link
Contributor Author

I'll try and put together a demo project but it might take a little time as I'm seeing the issue on internal source that I can't share without some redaction. I'll drop it into a new issue as it's a little off topic in here.

@AndrewFinlay
Copy link
Contributor Author

New issue for coverage differences #1208

@AndrewFinlay
Copy link
Contributor Author

Hi @coreyfarrell, I've got a couple of questions regarding this feature. I've got a rudimentary version of this going using the flag --save-baseline. It currently produces a standard coverage file as created by nyc.addAllFiles() and stores it as <temp-dir>/<uuid>.json.
The questions I need answered are:

  1. Is <temp-dir> a good location for the output?
  2. Is <uuid>.json a good name for the baseline coverage file?
  3. Do we need it to produce the <temp-dir>/processinfo directory and files?
  4. Should we allow --clean if we're saving to the <temp-dir>?

My gut feeling is that the above answers should be yes, no, yes, yes. But I'll wait to hear back before I proceed.

@coreyfarrell
Copy link
Member

  1. If it's not directly in <temp-dir> then it won't be found for reporting.
  2. Currently all files created by nyc will be <uuid>.json or in a subfolder. Creating a file not matching this pattern could conflict with something someone else is doing (they might overwrite it).
  3. This is tricky. It is currently expected that files in processinfo create a tree (with a single root). I'm not sure how the --save-baseline process would fit into this tree or if the existence an orphaned processinfo file will break anything.
  4. The ability to specify --clean will be essential if baseline is written directly to .nyc_output/<uuid>.json.

An alternative process I just thought of which might avoid the above issues:

  1. nyc instrument --save-baseline creates .nyc_output/baseline/coverage.json
  2. nyc --save-baseline npm t reads .nyc_output/baseline/coverage.json and records it into <uuid>.json of the nyc master process (the same one which would store --all coverage). Any coverage acquired for baseline files would override coverage found by --all (if both flags are used).

This might be the best way to work baseline coverage into the normal process, I think would be easier to document? I wouldn't argue against nyc instrument unconditionally creating .nyc_output/baseline/coverage.json, then we could just have nyc --baseline npm t (maybe also support nyc instrument --no-baseline)? I think pulling baseline coverage into the active coverage needs to be opt-in to avoid potentially using outdated information (for example if a project stops pre-instrumenting). For this to work I think --clean would need to ignore the .nyc_output/baseline folder.

@AndrewFinlay
Copy link
Contributor Author

I've been thinking a little bit about this feature and it seems to me that we really want is what nyc --all ... currently does. We should implement nyc instrument --all, then it's quite reasonable to expect the two --all commands to behave in the same way. No need for a separate baseline directory, the filename can stay <uuid>.json and the processInfo files would still be there if needed.

This also simplifies the interface and the documentation.

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

2 participants