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

Reporting never ends with mock-fs #324

Closed
matheuss opened this issue Jul 20, 2016 · 6 comments
Closed

Reporting never ends with mock-fs #324

matheuss opened this issue Jul 20, 2016 · 6 comments

Comments

@matheuss
Copy link

I've seen #246 and #205. Both are closed, but I'm facing this issue right now 😩

screen shot 2016-07-20 at 12 15 13 am
Notice the ^ C.

My code: https://github.com/matheuss/hpm/blob/master/test/test.js

@bcoe
Copy link
Member

bcoe commented Jul 20, 2016

@matheuss the last two times that we fixed this, it related to bundling using npm@v2 vs. npm@v3; since we now have significantly larger dependencies, I'd rather solve the problem for real and take advantage of de-duping.

any help that anyone can provide debugging this gnarly bug, would be greatly appreciated! CC: @novemberborn, @jamestalmage, @addaleax:

  1. bundling with npm@v2 (nested dependencies) seems to work with mock-fs.
  2. bundling with npm@v3 (top-level dependencies) seems to break.

I think it relates to a shared library, and singleton behavior -- this is a hunch.

@matheuss
Copy link
Author

matheuss commented Jul 20, 2016

I'm using HyperTerm and it is, sometimes when I run nyc ava, telling me this:

screen shot 2016-07-20 at 10 03 57 am

Through iTerm (before clicking ok), I found this:

❯ ~ cd .node-spawn-wrap-98907-3a79ae2824ed
❯ .node-spawn-wrap-98907-3a79ae2824ed ls -la
total 40
drwxr-xr-x    5 matheus  staff   170 Jul 20 10:06 .
drwxr-xr-x+ 112 matheus  staff  3808 Jul 20 10:10 ..
-rwxr-xr-x    1 matheus  staff  4189 Jul 20 10:06 iojs
-rwxr-xr-x    1 matheus  staff  4189 Jul 20 10:06 node
-rw-r--r--    1 matheus  staff   736 Jul 20 10:06 settings.json
❯ .node-spawn-wrap-98907-3a79ae2824ed cat settings.json
{
  "module": "/Users/matheus/.nvm/versions/node/v6.3.0/lib/node_modules/nyc/node_modules/spawn-wrap/index.js",
  "deps": {
    "foregroundChild": "/Users/matheus/.nvm/versions/node/v6.3.0/lib/node_modules/nyc/node_modules/foreground-child/index.js",
    "signalExit": "/Users/matheus/.nvm/versions/node/v6.3.0/lib/node_modules/nyc/node_modules/spawn-wrap/node_modules/signal-exit/index.js"
  },
  "argv": [
    "/Users/matheus/.nvm/versions/node/v6.3.0/lib/node_modules/nyc/bin/wrap.js"
  ],
  "execArgv": [],
  "env": {
    "NYC_CWD": "/Users/matheus/dev/node/hpm",
    "NYC_CACHE": "disable",
    "NYC_SOURCE_MAP": "enable",
    "NYC_INSTRUMENTER": "./lib/instrumenters/istanbul",
    "BABEL_DISABLE_CACHE": 1
  },
  "root": 98907
}

I think that in #205 or #246 I saw something about foreground-child 🤔

@addaleax
Copy link
Member

That last one looks like a race condition in the file watcher or in HyperTerm.

@addaleax
Copy link
Member

diff --git a/test/test.js b/test/test.js
index 762caa68f1a6..4d5941ff8906 100644
--- a/test/test.js
+++ b/test/test.js
@@ -2,9 +2,12 @@ import {homedir} from 'os';

 import test from 'ava';

-require('mock-fs')({
+const mockfs = require('mock-fs')
+mockfs({
        [`${homedir()}/.hyperterm.js`]: 'module.exports = {plugins: []};'
 });
+test.after(() => mockfs.restore());
+
 require('mock-require')(`${require('os').homedir()}/.hyperterm.js`, './_hyperterm-mocker');

 const api = require('../hyperterm');

should fix it, at least for your test case.

And yeah, sorry, but I don’t think this is something related to nyc’s dependency tree or even just something nyc can do anything about. mock-fs hooks into the fs module from core in a pretty invasive way, that is pretty much impossible to work around from nyc’s side.

There are two solutions for working with mock-fs that I can think of; documenting that calling mockfs.restore() is just plain necessary, or sending the coverage data to the parent nyc process and letting that one write the data to disk.

@matheuss
Copy link
Author

matheuss commented Jul 20, 2016

Thanks @addaleax! Fixed for me! Never thought of that.

Now with the solution and with what you say, makes sense to state that is not something from nyc. mock-fs indeed change node and we cannot prepare for this.

documenting that calling mockfs.restore() is just plain necessary

This is absolutely necessary. I'll submit a PR.

@jamestalmage
Copy link
Member

If someone wanted to take the time to implement tschaub/mock-fs#142, that would solve a lot of headaches as well.

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

4 participants