Issue with jest --coverage

[stereobooster]

Reproducible demo: https://github.com/stereobooster/babel-macro-issue/tree/import-all

yarn

yarn test    # Ok

yarn test:ci # Ok

But

yarn test:coverage
yarn run v1.5.1
$ CI=true react-scripts test --env=jsdom --coverage
FAIL src/App.test.js
  ● Test suite failed to run

    import-all.macro: This is not supported: ``. Please see the import-all.macro documentation Learn more: https://www.npmjs.com/package/import-all.macro

      at node_modules/import-all.macro/dist/macro.js:28:13
          at Array.forEach (<anonymous>)
      at prevalMacros (node_modules/import-all.macro/dist/macro.js:20:22)
      at macroWrapper (node_modules/babel-plugin-macros/dist/index.js:54:12)
      at applyMacros (node_modules/babel-preset-react-app/node_modules/babel-plugin-macros/dist/index.js:164:5)
      at PluginPass.ImportDeclaration (node_modules/babel-preset-react-app/node_modules/babel-plugin-macros/dist/index.js:82:9)
      at newFn (node_modules/@babel/core/node_modules/@babel/traverse/lib/visitors.js:243:21)
      at NodePath._call (node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:65:18)
      at NodePath.call (node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:40:17)
      at NodePath.visit (node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:100:12)

Related pveyes/raw.macro#5

[kentcdodds]

Yeah, I'm thinking we should add a test that can reproduce this issue. If you could come up with a simple isolated example and add a test here I can work on addressing it. This issue of babel plugins not working with the coverage babel plugin has been a real pain for me for a long time. preval has the same problem.

[stereobooster]

At the moment I have no idea what exactly causing it. I guess babel-plugin-istanbul which is dependency of babel-jest, but I can not reproduce it without whole CRA setup.

[kentcdodds]

Yes, that's what's causing it. Check this out. I'm fairly certain that we just need to handle a SequenceExpression

babel-plugin-istanbul converts this:

var x = foo()

to something like this:

var x = (c++, foo())

So we need to figure out a good way to handle that SequenceExpression. I'm pretty sure our test case could be something like:

import importAll from '../macro'
const a = ('whatever', importAll.sync('./fixtures/*.js'))

With that we should be able to figure out what's wrong and fix this bug.

[stereobooster]

the smallest reproducible demo I can come with

const filename = "test.js";
const src = `import importAll from "import-all.macro";
const all = importAll("./components/*.js");`;
const options = {
  presets: [
    [
      require("@babel/preset-env").default,
      {
        targets: { node: "6.12" }
      }
    ]
  ],
  plugins: [
    require("babel-plugin-macros"),
    [
      require("babel-plugin-istanbul").default,
      {
        cwd: __dirname,
        exclude: []
      }
    ]
  ]
};
require("babel-core").transform(src, { filename, ...options });

https://github.com/stereobooster/babel-macro-issue/tree/minimal-example

[fatfisz]

Hey-o, I did some research on my own (I ❤️ problems like this), and I think we could do something even easier.

Here's some stuff that I printed out while debugging:

generator(referencePath.parent, {}, null).code importAll("./components/*.js")
generator(referencePath.parentPath.node, {}, null).code cov_260za5r4o5.s[0]++, importAll("./components/*.js")

It seems to me that while the paths are up-to-date with the transforms, the relationship between the nodes is preserved. I'm positive that if referencePath.parentPath.type was replaced with referencePath.parent.type it would just work.

Of course then referencePath.parentPath.node.property.name needs to become referencePath.parent.property.name and so on.

Are there any strong arguments for using parentPath (a path) instead of parent (a node)?

[fatfisz]

Btw. handling SequenceExpression looks like a very complicated thing - what if the comma operator was put in the code by a programmer and not by the coverage plugin?

[kentcdodds]

Thanks for that. I'll dig in!

[fatfisz]

Further findings:

• babel-plugin-istanbul sets a visitor for the Program type, so babel-plugin-macros comes in after the declarations are added; this makes the situation less problematic than it could be ✨
• Nodes in babel don't have the API to manipulate themselves, so paths are absolutely necessary.

I'll continue looking into that in parallel, maybe we'll come up with a solution quicker.

[kentcdodds]

Awesome! Thank you for your help @fatfisz!

[fatfisz]

It looks like babel is not doing a great job at updating paths after

path.replaceWith(T.sequenceExpression([increment, path.node]));

is called in istanbul-lib-instrument (from babel-plugin-istanbul).

It seems that if a plugin is doing path.replaceWith(something(path.node)) it will most certainly mess things up (if a macro call is the replaced node).

---

Based on this I experimented with traversing the whole program again (the idea was to look for the identifiers manually instead of using the scope), and the most magical thing has happened. When the following piece of code is put inside applyMacros:

state.file.scope.path.traverse({
  Identifier() {}
});

everything starts to work! It doesn't matter if you put it before the path.scope.getBinding call, or just before macro({ references: referencePathsByImportName, ... }). The reference paths somehow correct themselves and I can see the successful result:

yarn run v1.6.0
$ set CI=true && react-scripts test --env=jsdom --coverage
 PASS  src\App.test.js
  √ renders without crashing (31ms)

  console.log src\App.js:7
    { './components/a.js': { default: [Function: A] } }

----------------|----------|----------|----------|----------|-------------------|
File            |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------------|----------|----------|----------|----------|-------------------|
All files       |       50 |      100 |    66.67 |    44.44 |                   |
 src            |       50 |      100 |      100 |     37.5 |                   |
  App.js        |      100 |      100 |      100 |      100 |                   |
  index.js      |        0 |      100 |      100 |        0 |         1,2,3,4,6 |
 src/components |       50 |      100 |        0 |      100 |                   |
  a.js          |       50 |      100 |        0 |      100 |                   |
----------------|----------|----------|----------|----------|-------------------|
Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        5.641s
Ran all test suites.
Done in 6.70s.

What's even more interesting is that just calling

state.file.scope.path.traverse({});

doesn't fix the problem - Identifier need to be one of the visitors. Babel is funny 😂

The thing I don't like about this finding is that it is a hack and the relationship between the line of code that fixes all and the actual underlying cause is not clear. The good thing about it though is that it should handle all the cases, not only when the path is replaced with itself inside a SequenceExpression.

[fatfisz]

Also that finding makes things clear: babel is at fault, because calling traverse like this won't actually call any plugins, so the only thing at work during that call is babel's internals.

[kentcdodds]

@fatfisz, would you like to open a PR on babel-plugin-macros with your changes and a comment explaining what it's there for. Then we can at least get this fixed for people using macros for now. Then we can maybe open an issue on Babel to see why this works around the problem and how to actually fix it in babel.

Thank you so much for all the digging!

[fatfisz]

Ok, I'll try to come up with a minimal reproduction test and prepare the PR. It's quite late where I live though, so I'm not sure if I'll get it done today or tomorrow.

[fatfisz]

I'll come back to this tomorrow. For now here's a minimal reproduction of the same path behavior:

'use strict';

const { transform } = require('babel-core');
const { default: generator } = require('babel-generator');
const { default: traverse } = require('babel-traverse');
const types = require('babel-types');

const visitedPaths = new Set();

const badVisitor = {
  VariableDeclarator: {
    enter(path, state) {
      const initPath = path.get('init');

      initPath.replaceWith(
        types.sequenceExpression([
          types.stringLiteral('foobar'),
          initPath.node,
        ]),
      );
    },
  },
};

transform(
  `
    import foo from 'foo';
    const bar = foo();
  `,
  {
    plugins: [
      () => ({
        visitor: {
          Program: {
            enter(path) {
              path.traverse(badVisitor);
            },
            exit(path, state) {
              console.log(generator(state.file.scope.path.node, {}, null).code);
            },
          },
          ImportDeclaration(path) {
            const varPath = path.scope.getBinding('foo').referencePaths[0];
            console.log(
              generator(varPath.node, {}, null).code,
            );
            console.log(
              generator(varPath.parentPath.node, {}, null).code,
            );
            console.log(
              generator(varPath.parent, {}, null).code,
            );
            // The last two should print the same thing
          },
        },
      })
    ],
  },
);

[kentcdodds]

This is great work! Well done. Thank you @fatfisz and @stereobooster!

[kentcdodds]

Just FYI @hzoo and @loganfsmyth, this will likely turn into a babel issue soon :)

[fatfisz]

I hit multiple issues trying to run the tests on Windows - switching to Ubuntu VM...

[kentcdodds]

Thanks for fixing that issue! It's been merged and will be released in a minute. Then @stereobooster, if could test this out with the latest version of babel-plugin-macros that would be super!

[stereobooster]

Verified fix with small example. Opened PR in create-react-app.

@fatfisz is a hero

[vinhlh]

babel/babel#7596 This is the original issue we found before.
I was trying to fix the issue in Babel by referring parent paths to right ones, but not done yet.