Unable to reproduce coverage

[ghost]

​

[bcoe]

@kdex what platform are you running on, OSX?

[bcoe]

@kdex I was able to get things working partially by using nyc@next directly:

  "nyc": {
    "include": [
      "src/**/*.js"
    ],
    "exclude": [
      "**/test/**/*",
      "packages/**/*"
    ],
    "sourceMap": true,
    "instrument": true,
    "require": [
      "babel-register"
    ],
    "all": false
  }

rather than using babel-plugin-istanbul. digging into why babel-plugin-istanbul might have had issues.

@jamestalmage anything jump out at you?

[bcoe]

Did a bit more investigation, I'm actually unable to run tests on my machine due to the error:

Error: Cannot find module '/Users/benjamincoe/bcoe/coverage/map-game/dist/client/js/cache.js'

But ultimately, I think the path that gets passed to babel-plugin-istanbul has been mucked with in such a way that it gets excluded. I see this when I modify the exclude rules:

../../src/client/js/cache.js

which doesn't get instrumented, because test-exclude ignores files outside of the current project directory.

I think there's some work we need to do around the various interactions between: test-exclude, nyc, ava, and babel-plugin-istanbul.

Having said this, give things a spin using nyc directly 👍

[jamestalmage]

I haven't the slightest idea why this would be happening, but I would not be surprised to find out that ava-jspm-loader was involved. We are piling transforms on top of transforms here, so it seems a likely culprit. I don't think anybody on the AVA core team has really spent much time with it (I personally have no clue how it works), so your best bet is to ping @skorlir.

[jamestalmage]

// @dak

[fluffywaffles]

The loader is easy to debug if you're thinking it's at fault. Try "AVA_JSPM_LOADER_DEBUG=1 ava" and see if any of the output that occurs right before the exception looks suspicious.

[fluffywaffles]

@jamestalmage it's worth noting that ava-jspm-loader does not apply any transforms to the code. It creates a small (<100 line) wrapper around Module._load that tries to look up a module in the SystemJS project registry before forwarding it on to the normal Node module loader. If babel-plugin-istanbul is dependent on some sort of module loading behavior that works with Node but not with JSPM (e.g.; directly importing nested dependencies is not possible because SystemJS doesn't map them) then ava-jspm-loader will expose that bug. But it's not a bug in ava-jspm-loader per se.

For example: fluffywaffles/ava-jspm-loader#3 is caused by the aforementioned nested dependency resolution glitch. You cannot System.import('fbjs/lib/invariant'), even though fbjs is installed as a dependency of react because SystemJS can't resolve direct imports of nested dependencies.

[fluffywaffles]

In other words: this is more likely a istanbul + jspm/systemjs interop bug than something to do with ava-jspm-loader directly. But I could be wrong.

[jamestalmage]

Can you still require('fbjs/lib/invariant')? Or, more importantly: require('babel-runtime/some/nested/thing/that/should/exist').

[jamestalmage]

Hmm. I get an entirely different error from @bcoe

Error: Cannot find module '/Users/jamestalmage/WebstormProjects/forks/map-game/packages/npm/ws-promise-client@4.1.2'

It originates from this line.

That's obviously not a good path into node_modules.

Here is the output from ava-jspm-loader:

[loader:internal] ws-promise-client in System._loader.modules: false
[loader:internal] possible paths: ws-promise-client/
[loader:internal] jspmHasModule(ws-promise-client): true
[loader:internal] possible paths: ws-promise-client/
[loader:internal] successfully normalized: file:///Users/jamestalmage/WebstormProjects/forks/map-game/packages/npm/ws-promise-client@4.1.2
[loader:internal] parsed module path: /Users/jamestalmage/WebstormProjects/forks/map-game/packages/npm/ws-promise-client@4.1.2

[fluffywaffles]

Nested modules isn't the problem. If you directly install fbjs via jspm, like jspm i npm:fbjs, then importing fbjs directly will work fine. (Directly installing it makes JSPM add it to the SystemJS map.)

After transpilation, all imports => requires - and ava-jspm-loader intercepts calls to require and tries to load using SystemJS. If SystemJS says, "no, I don't have one of those," it forwards the module name on to the Node Module loader. (Basically.)

This is the situation that doesn't work:

• You have a dependency managed by jspm, say react, that has a subdependency - say, fbjs.
• You can import/require react directly and it works fine.
• You cannot import/require fbjs, however, because SystemJS will say "I don't have anything called fbjs" and will forward that request on to the Node Module loader. This is in spite of the fact that there exists a jspm_packages/npm/fbjs@x.y.z in your project - SystemJS simply doesn't allow you to import it directly.

[fluffywaffles]

@jamestalmage Your paths don't always go into node_modules, but sometimes into jspm_packages (or whatever you have named it) instead. (In this case, packages.)

SystemJS clearly thinks you have installed ws-promise-client via JSPM, so the lookup succeeds. Check if the module path actually exists. If it doesn't, that may be because you haven't run jspm install. SystemJS will successfully normalize a module path even if the path is empty - ie, if you haven't installed the module, but your config says that you should have it.

[fluffywaffles]

Essentially the entire job of ava-jspm-loader is just redirecting certain imports to jspm_packages instead of node_modules.

[jamestalmage]

I hadn't run jspm install, I have an accurate reproduction now (I wasn't kidding when I said I had no idea how jspm works).

[fluffywaffles]

Just think of it as an alternative to npm. SystemJS, then, is like... well, a universal module loader. It loads modules, transpiles them, supports AMD-style paths, etc, etc etc. So really JSPM is two things: 1) an alternative to npm and 2) a CLI for SystemJS so that you can build, bundle, configure, and so on from the terminal. :)

You can also read the sales pitch at http://jspm.io. It's fairly informative.

[jamestalmage]

So, I was able to generate coverage using the following steps:

1. Install Prerequisites:

   npm i -g jspm gulp-cli trash-cli
   

2. Clone the repo, install dependencies:

   git clone git@github.com:kdex/map-game.git
   cd map-game
   npm install
   jspm install
   

3. Build the dist dir with the test environment set

   NODE_ENV=test gulp build
   

4. Run the tests

   gulp test
   

---

That we are requiring from dist didn't make a lot of sense to me, so I made this change:

diff --git a/jspm.config.js b/jspm.config.js
index 02633c1..ae5678c 100644
--- a/jspm.config.js
+++ b/jspm.config.js
@@ -2,8 +2,8 @@ SystemJS.config({
   paths: {
     "npm:": "packages/npm/",
     "github:": "packages/github/",
-    "server/": "dist/server/js/",
-    "client/": "dist/client/js/"
+    "server/": "src/server/js/",
+    "client/": "src/client/js/"
   },
   browserConfig: {
     "baseURL": "/<%LINUX_USERNAME%>"

I deleted the dist dir, and ran the tests again:

trash dist
gulp test

The tests pass, but I still get no coverage.

I finally hunted it down to AVA's process.chdir() behavior (we change the CWD to the running test).

The default nyc instrumentor honors the NYC_CWD environment variable (which is set in the parent process using pkg-up). babel-plugin-istanbul does not honor that.

Manually hacking babel-plugin-istanbul in my node_modules directory as follows, finally got everything working:

// node_modules/babel-plugin-istanbul/lib/index.js

function shouldSkip(file) {
  if (!exclude) {
    exclude = testExclude({
-      cwd: getRealpath(process.cwd()),
+      cwd: process.env.NYC_CWD || getRealpath(process.cwd()),
      configKey: 'nyc',
      configPath: (0, _path.dirname)(findUp.sync('package.json'))
    });
  }

  return !exclude.shouldInstrument(file);
}

[jamestalmage]

@skorlir
Is it common practice to precompile to a dist dir and load from there? Having a pre-build step seems antithetical to the whole "loader" concept. If this is a common point of confusion, perhaps the AVA recipe should be extended with just enough info to show people the separate steps they would use to actually build their dist dir as part of a deploy (building isn't really an AVA concern, but maybe just enough example to show people how to combine the two in a way that doesn't conflict).

@sindresorhus @novemberborn
Yet another example of a bug from AVA's chdir behavior. We need to revert ASAP. Priority tag maybe?

@bcoe @novemberborn
Despite the fact that we intend to revert AVA's chdir behavior, NYC should really just handle it. That was the whole point of the NYC_CWD behavior in the first place. I'm not sure my hack mentioned above is the correct place to do it, but we should try to provide consistent behavior across the instrumentors.

[fluffywaffles]

@jamestalmage common practice only if you're using TypeScript, in my experience. The point of the loader is indeed to avoid having a pre-build step.

I wrote the recipe with the intention that it would encourage people to not prebuild before testing. In fact, I've not seen a setup before that creates a dist/ directory? None of my projects do this, and it's not directly possible with jspm (afaik) to build each file separately - it's bundle or nothing.

I'd almost prefer to not mention building in the recipe, seeing as if you're building to a bundle ahead of time it defeats the purpose of having a loader in the first place.

[jamestalmage]

I'd almost prefer to not mention building in the recipe, seeing as if you're building to a bundle ahead of time it defeats the purpose of having a loader in the first place.

Yes, agreed. But if you're using JSPM you're almost certainly targeting the browser, and you will build at some point before pushing to the server / cdn. I am wondering if we can't somehow make it clearer what the loader is capable of, and how it eliminates the prebuild step for testing.

[fluffywaffles]

Would it be sufficient to just say, during the preamble of the loader recipe, "The loader exists so that you don't have to build before running your unit tests"?

[bcoe]

@jamestalmage I'm comfortable with the NYC_CWD patch, this seems like something we should take into account with the plugin.

[fluffywaffles]

@kdex as I understand it, one would ordinarily use jspm bundle or jspm build to create a single distributable file (build.js) rather than compile the files into a directory. This cuts down even more on the repeated overhead, seeing as SystemJS can then more efficiently route requests (in the event of a bundle) or you can just pull in one static file (in the event of a build). Also if you use bundle, you can benefit from HTTP/2 if your server is set up for it.

[bcoe]

@kdex I've published a new version of babel-plugin-istanbul@1.0.3 with @jamestalmage's patch, seems to be working for me.

I'm also about to publish nyc@7.0.0 which has several improvements to ES6 instrumentation, etc.

[sindresorhus]

Yet another example of a bug from AVA's chdir behavior. We need to revert ASAP. Priority tag maybe?

@jamestalmage We really do... It's stuck needing a codemod. It's already labeled as priority.