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

Files required through node --eval or node --require do not get instrumented #144

Open
ehmicky opened this issue Nov 30, 2018 · 8 comments

Comments

@ehmicky
Copy link

ehmicky commented Nov 30, 2018

Demo repository

Expected Behavior

Files required through node --eval or node --require should be instrumented.

Observed Behavior

They do not get instrumented.

The demo repository contains two empty files index.js and other.js.

Running the file instrument it correctly:

$ nyc node index.js 
----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |        0 |        0 |        0 |        0 |                   |
 index.js |        0 |        0 |        0 |        0 |                   |
----------|----------|----------|----------|----------|-------------------|

Requiring it through --eval does not instrument it:

$ nyc node -e 'require("./index")'
----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |        0 |        0 |        0 |        0 |                   |
----------|----------|----------|----------|----------|-------------------|

Requiring it through --require does not instrument it:

$ nyc node -r ./index.js other.js 
----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |        0 |        0 |        0 |        0 |                   |
 other.js |        0 |        0 |        0 |        0 |                   |
----------|----------|----------|----------|----------|-------------------|

Forensic Information

Operating System: Ubuntu 18.04
Environment Information: node v11.3.0, npm 6.4.1, nyc 13.1.0

Notes

nyc has a --require flag. However this only works when node is fired as the top-level command, not as a child process.

For example I am testing a library that has a register.js file that performs monkey patching on load (like @babel/register does). The only way to test it without polluting the global environment of the other tests is to run it as a child process. I.e. my unit test runs childProcess.spawn() using --require register.js.

I can work around the problem and avoid --eval or --require by loading a file that fires require() instead, but there might be other use cases that this limitation of nyc might impact.

@ehmicky
Copy link
Author

ehmicky commented Jan 29, 2019

Bumping this so the issue does not get stale.

@istanbuljs istanbuljs deleted a comment from stale bot Jan 29, 2019
@ehmicky
Copy link
Author

ehmicky commented Mar 30, 2019

Bumping again

@coreyfarrell
Copy link
Member

I can answer part of this bug, it's not possible for files required via --require to be automatically instrumented. To get this working you must pre-instrument the file to be included via --require, along with any sources it require's (if you want those files to be covered). The --require options are processed before nyc can hook anything, partly due to startup order and partly due to the fact that --require options can actually be used to perform the instrumentation (see ES2015 tutorial for example).

The way that I know will work is if you have a module where sources are in ./lib/*.js and you want to test node --require ./lib/register.js, you would have to use the nyc instrument command to build an instrumented copy of the lib folder. Then you would tests with node --require ./instrumented/register.js.

As for node --eval, I'm not sure if it's possible to hook this. Like --require this is run very early in the process of starting a node process, before nyc can do anything. Again I think the only way to accomplish coverage testing of stuff loaded so early is to pre-instrument it.

@coreyfarrell
Copy link
Member

I've marked this issue so the stale bot will not touch it anymore, but understand that no code fix is planned. The intent is to create a tutorial at istanbul.js.org for how to perform coverage testing of a --require script.

@ehmicky
Copy link
Author

ehmicky commented Mar 30, 2019

I understand, thanks for explaining the reason behind this. I do think this is a limitation in the current hooking system that should be documented.

A workaround I am doing which does not require an extra build step is to add a require() at the beginning of the first file to be executed. This only works if you can modify that file though (which is my case).

Another thing: I have seen that there has been some work in Istanbul towards offering NODE_V8_COVERAGE as an alternative for instrumentation. This still seems too early to use for me (no way to disable coverage through comments, and implementations still a little unstable from my own experience). But in the long run, would this solve that problem?

@coreyfarrell
Copy link
Member

Native V8 coverage can be processed by c8 in place of nyc, it only uses istanbul to generate reports. From what I can tell it doesn't yet support any kind of comment hints for disabling coverage. I'd assume that native v8 coverage would pick up on node --require ./something.js, but I've never tried.

@ehmicky
Copy link
Author

ehmicky commented Mar 31, 2019

Yes exactly.

Thanks for your help, I am going to close this issue as this probably won't be fixed. However thanks for checking it out.

I am also going to delete the demo repository, please let me know if that is a problem.

@ehmicky ehmicky closed this as completed Mar 31, 2019
@istanbuljs istanbuljs deleted a comment from stale bot Mar 31, 2019
@JaKXz JaKXz transferred this issue from istanbuljs/nyc Mar 31, 2019
@JaKXz JaKXz reopened this Mar 31, 2019
@JaKXz
Copy link
Member

JaKXz commented Mar 31, 2019

@ehmicky thanks for your patience but I have transferred this issue to our website repo so that it exists in the right place as a need to be documented. Feel free to unsubscribe from the thread if you don't want notifications on this any more. I have also forked your demo repository, so it should be fine to delete... though it doesn't do much harm to keep. Either way, thanks so much for that!

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

No branches or pull requests

3 participants