NYC doesn't work correctly with local node_modules

[hershmire]

Expected Behavior

NYC to not exclude local node_modules (i.e. src/node_modules/... OR src/somedirectory/node_modules/**) BUT to exclude root node_modules. We have a lot of Node applications at the company I work for that use the pattern of creating local node_modules as it helps modularize our code at an application/page-level and provides a nice way to require files instead of doing long relative paths to everything. Below is an example file structure many of our apps could see:

app/
  |- node_modules/ (root modules and we expect to exclude)
  |- src/
  |  |- node_modules/ (local to the app)
  |  |  |- local-module-1/
  |  |  |  |- test-node/
  |  |  |  |  |- file-test.js
  |  |  |  |- package.json
  |  |  |  |- file.js
  |  |- middleware
  |  |- pages/
  |  |  |- page-1/
  |  |  |  |- test-node/
  |  |  |  |  |- file-test.js
  |  |  |- file.js
  |  |  |  |- node_modules/ (local to page 1 of application)
  |  |  |  |  |- page-local-module-1/
  |  |  |  |  |  |- test-node/
  |  |  |  |  |  |  |- file-test.js
  |  |  |  |  |  |- package.json
  |  |  |  |  |  |- file.js
  |  |  |- page-2/
  ...

Observed Behavior

Since by default, NYC adds the glob **/node_modules/** it will exclude all node_modules, which in our case we don't want. To get around this problem, you have a negateExcludes section where we supply !src/**/node_modules/** but that doesn't quite work right as it will force to include our test files which live next to the files that are being tested since negateExclude is evaluated after the exclude rules.

Additionally, even with the all: true flag set in package.json, NYC doesn't appear to be picking up all files as I thought it would; especially the ones that I purposely left out of having tests point to – i.e. https://github.com/hershmire/nyc-test/blob/master/src/pages/page-1/components/Name.jsx & https://github.com/hershmire/nyc-test/tree/master/src/pages/page-no-tests-1.

It would be nice to not exclude all node_modules by default but just the root ones for starters. OR at least provide a easier way to turn that off. It gets quite complicated when you also have to take into account babel-plugin-istanbul and understand how to set both up properly.

Bonus Points! Code (or Repository) that Reproduces Issue

I created a quick demo app that shows what I'm running into here:

https://github.com/hershmire/nyc-test

After running yarn install, run yarn coverage

As you can see from the results, my test files under any test-node folders are picked up which they should be excluded.

Forensic Information

Operating System: the operating system you observed the issue on.
MacOS 10.13.6

Environment Information: information about your project's environment, see instructions below:

1. run the following script:

sh -c 'node --version; npm --version; npm ls' > output.txt

https://github.com/hershmire/nyc-test/blob/master/output.txt

[hershmire]

Anyone have time to take a look at this and provide some thoughts?

[leslc]

This is affecting our ability to run unit tests. Any solution, even a hacky workaround, would be appreciated. Or where the fix could be made with a PR.

[coreyfarrell]

This is not a simple issue, we cannot just stop ignoring **/node_modules/**, this is needed to support monorepo's (for example lerna repos). I'll have to look into a way to make this easier to override but the default is what it needs to be.

Sorry I don't have any suggestions for work-arounds, you are doing things in a way I've never thought of.

[hershmire]

@coreyfarrell Thanks for the reply! Let me know if there is anything we can do to help you out with this.

[leslc]

Thanks @coreyfarrell

Question on the glob.sync to return all file names.

EDIT: looking a bit closer

I see { ignore: this.exclude.exclude } is passed to glob.sync to pass only the exclude array.
"test-exclude" cuts this out into two arrays this.exclude and this.excludeNegated (removing the "!" character in front).
https://github.com/istanbuljs/istanbuljs/blob/master/packages/test-exclude/index.js#L62

At this point this.exclude.exclude has all negation values removed. I think { ignore: this.exclude.exclude } needs to include "this.exclude.exclude" and "this.exclude.excludeNegated" "excludeNegated needs to have all all values prepended with exclamation mark (!) again.

Does this seem like a plausible idea? If so, I can try it out locally and let you know if results are different.

[jweidler]

Unfortunately there are problems with the misunderstanding of how node_modules are not exclusively third-party modules on every corner. This is broken and won't be fixed. So we've moved away from using local node_modules to defining local dependencies via package.json and have npm create symlinks. This approach requires some wiring upfront but once it's set up it feels much nicer than having node_modules all over the place.

Here in detail how we solved it:
istanbuljs/nyc#833 (comment)

[hershmire]

Unfortunately there are problems with the misunderstanding of how node_modules are not exclusively third-party modules on every corner.

Yea, it seems there is a big misunderstanding of that fact indeed. Thanks for passing along your solution to NYC's limitation but unfortunately, we have too many applications that are using local node_modules and I'm concerned about the risks when building and deploying out to different environments. Until we can get some additional dialogue with the NYC owners for a proper solution, we might just fork the repo and handle the fix internally that way.

I'm happy to contribute to NYC's codebase for a fix but first need more feedback from the owners/contributors.

[coreyfarrell]

Unfortunately there are problems with the misunderstanding of how node_modules are not exclusively third-party modules on every corner.

I don't agree with this assessment. node_modules does have a specific purpose, to install external modules. It's possible to abuse it as you have, but node_modules is meant for stuff installed by npm - normally stuff that has it's own testing. I'm not against making it possible/easier for you to use NYC but I have to prioritize the normal way of working.

I'm open to making a boolean option to prevent test-exclude from forcing exclude of **/node_modules/**. So you would configure NYC to not force it, then configure your own excludes which has node_modules/** instead of **/node_modules/**.

[hershmire]

I'm open to making a boolean option to prevent test-exclude from forcing exclude of /node_modules/. So you would configure NYC to not force it, then configure your own excludes which has node_modules/** instead of /node_modules/.

@coreyfarrell I'll take a stab at this. From what I understand, looks like I'll need to make edits to three libraries – babel-plugin-istanbul, nyc, and test-exclude.

---

On a slightly separate note, I also noticed that the --all flag wasn't working as I would expect. I mentioned it in this issue's original description but since I'm doing some custom work with local modules I figured I would break it out in a new repo to test this case out in isolation. From what I can gather from looking over other issues and the documentation, it should work but I'm still not having any luck. I created a new issue that details that out here istanbuljs/nyc#911. Maybe I'm missing something 🤷‍♂️ ...

[leslc]

thanks @hershmire, this is exactly what we need. @coreyfarrell, thoughts on merging these PRs?

[coreyfarrell]

@leslc I have to ask for patience. I'm currently dealing with release related issues which need to be addressed first, I have limited time to work on NYC.

[hershmire]

@coreyfarrell Understandable that you're busy. Any idea when you'll be able to have time to look at the three PRs referenced above?

[theqabalist]

This seems to also match our use case at our job as well. So I would also appreciate any attention this matter could receive.

[bcoe]

👋 I've moved this issue to the istanbuljs repo, I think we should address this behavior in test-exclude if possible.

[coreyfarrell]

With the merge of #213, #351, istanbuljs/nyc#912 and istanbuljs/nyc#1053 this is now supported in nyc, documented by istanbuljs/nyc#1039. To get this feature you need to upgrade to [email protected].

istanbuljs/babel-plugin-istanbul#172 still pending, need to resolve a merge conflict. Further progress can be monitored on that PR if you need this functionality in the babel plugin.