Skip to content
This repository has been archived by the owner on Dec 8, 2024. It is now read-only.

ReferenceError: cov_${id} is not defined #922

Open
HaruKawamata opened this issue Oct 20, 2019 · 10 comments
Open

ReferenceError: cov_${id} is not defined #922

HaruKawamata opened this issue Oct 20, 2019 · 10 comments

Comments

@HaruKawamata
Copy link

HaruKawamata commented Oct 20, 2019

Context

At Tanda, we run tests on CircleCI as part of our deployment process. Recently when running with --coverage flag, our tests are only failing on CircleCI, not locally. @builtbyproxy and I spent a whole day walking through the process to identify what may throw the error mentioned below. These are our findings.

Prerequisites

package.json versions

jest 24.8.0
react-native 0.59.9
babel-jest 24.8.0
react: 16.8.3

We touch:

  • istanbul-lib-instrument
  • babel-plugin-istanbul
  • babel-jest
  • istanbul-reports

Description

When running tests in our local env, everything passes as intended. However, when deployed to CircleCI, we get an error in the form of ReferenceError: cov_14jyvcatev is not defined. The id that comes after the cov_ changes depending on which file it is referring to. The file referred to by the cov_${id} has (so far) never been the file that is being tested.

This only happens on CircleCI, and when the jest --coverage flag is on.

This also only happens to one test per test run. It's always the same test, and the cov_id is always the same (but does not refer to the test or the tested component that fails). For example, we test component B, and component J's cov_id will be the cause. We have had cases where the two components are completely unrelated.

image

Generating the cov_id

This cov_${id} is generated in istanbul-lib-instrument/dist/visitor.js using the function genVar. genVar is called in the constructor for the VisitState class, and is assigned to this.varName. A VisitState object is created in the function programVisitor and used in the enter and exit functions, which is the default export for istanbul-lib-instrument/dist/visitor.js

The cov_id's journey

With our programVisitor default export we found it is imported in istanbul-lib-instrument/dist/index.js and exported again as programVisitor.

Then it is imported by babel-plugin-istanbul/lib/index.js and is given types, realPath, and other options. With the result being assigned to this.__dv__:
image

Then it gets imported and exported by babel-jest/build/index.js and exports it as a part of the transformer (it's the babelIstanbulPlugin part)
image

We logged transformResult and got a big string with two consistent values of code and map. code looked something like this:

code: 'var cov_14jyvcatev = function () {\n  var path = "/usr/local/repo/src/components/Cards/ShiftAcceptance.js",\n...
// a LOT more lines

This gave us our first reference to the cov_${id} that was being used as a value, thus pointing to a potential ReferenceError: cov_id is not defined.

We searched our entire repo for instances of code being used and found the file istanbul-reports/lib/html/annotator.js running a map on the code variable after it's been split on newlines to generate an array of code lines. The istanbul-reports code looks like this:
image

When running the same split on the same code string from transformResult (as mentioned above), we got an array looking like this:
image

Furthermore, when scrolling through we noticed instances of cov_id value being specifically mutated. This is the only place we can find reason for a ReferenceError: cov_id is not defined. (Pictured is the babel-plugin-graphql-tag import example, however the cov_id increments are inserted throughout our code as well.)
image

istanbul-report generates an interactive directory of coverage reports. In these reports, there is a counter beside each code line which indicates how often it's called in tests. The GUI for this looks like this:
image

We believe that there is a race condition assigning the cov_${id} which isn't finishing before babel(or istanbul-reports or something?) tries to increment it.

@robertleeplummerjr
Copy link

robertleeplummerjr commented Oct 30, 2019

In GPU.js and Brain.js we generate/compile functions that can execute on either the GPU (webgl or headlessgl) or CPU (javascript) and these variable make it into new Function() and essentially break everything when code coverage is running. I was getting this issue all over the place and was at an impasse.

To this end I built https://www.npmjs.com/package/istanbul-spy, which allows you to get a means of identifying which global the value is from a string (we use name from the ast MemberExpression) that matched these the variable signature and append it to each function before they compile with new Function().

It would be super helpful if there was a means of either filtering these values out or intending them from a string more easily.

@HaruKawamata
Copy link
Author

Whoa... you're a real one @robertleeplummerjr <3

I've read through the code, but don't fully understand how to use it. Would you mind adding some usage docs to that repo's README?

@builtbyproxy
Copy link

Ah, this looks so good! @robertleeplummerjr ❤️

Also in addition to docs, specifically how to connect it with Jest in a package.json script would be a huge help too!
we run something similar to TZ=UTC yarn jest --coverage

@robertleeplummerjr
Copy link

I'll add documentation soon, but here is it in use in brain.js in the mean time: BrainJS/brain.js@dfb8fe7#diff-ead454b663ac7bc8efc3776e832e0d6fR14

It is literally just function (value: string) => string

@builtbyproxy
Copy link

Awesome thanks @robertleeplummerjr
We will report back shortly with our results

@HaruKawamata
Copy link
Author

From my understanding of what istanbul-spy is doing, I've attempted to patch the package for istanbul-reports (the dead end we hit, where the generated code failed).

const { getFileCoverageDataByName } = require('istanbul-spy');

...

function annotateSourceCode(fileCoverage, sourceStore) {
  let codeArray;
  let lineCoverageArray;
  try {
    const data = getFileCoverageDataByName(name);
    if (!data) {
      throw new Error(`Could not find istanbul identifier ${name}`);
    }
    const { path } = data;
    const variable = `const ${name} = __coverage__['${path}'];\n`;
    const sourceText = variable.concat(
      sourceStore.getSource(fileCoverage.path),
    );
    const code = sourceText.split(/(?:\r?\n)|\r/);

...

Having patched and pushed to our CircleCI, we still receive the same error (cov_{id} is undefined). @robertleeplummerjr Please let me know if I have the completely wrong idea about what should be happening.

@robertleeplummerjr
Copy link

For our use case, istanbul is simply incompatible: istanbuljs/istanbuljs#499 (comment)

They recommended using https://github.com/bcoe/c8. However, while I appreciate what the team has done, and the attention they gave to our issue, this renders a somewhat important part of javascript essentially incompatible with istanbul. Any time you use new Function, you can basically either:

  • write off istanbul for code coverage, unless you want to fill your code with tons of comments telling it what you want or don't want tested.
  • or use a tool such as https://www.npmjs.com/package/istanbul-spy, which strips out istanbul variables just in time.

@xuexb
Copy link

xuexb commented Jun 22, 2020

// src/xxx.js
testCov(text) {
    if (process.env.NODE_ENV !== 'development') return null;

    const key = text.match(/(cov_[0-9a-z]+)/i) ? RegExp.$1 : null;
    const cov = eval(`(${key})`);
    return new Function(key, `return ${text}`)(cov)();
}

// test
testCov(fn.toString());

@robertleeplummerjr
Copy link

robertleeplummerjr commented Dec 31, 2020

This issue is absolutely unacceptable, especially at the scale that Istanbul has been used. To further this, Istanbul has only gotten more complicated since this issue came up.

All that is needed is a simple ternary check on the part of Istanbul for each injected artifact, so that it doesn't cause thousands of ENTIRE projects to implode.

Before you dismiss that, as has been the case in the past, PLEASE think about it. If Istanbul wants to cover Javascript, you MUST support new Function(), and to support that simply use a simple ternary for each artifact so as to ensure they exist in the context they are executed. It isn't even a question, YOU MUST DO IT, because new Function() is a feature with FULL SUPPORT across the board including almost every version of node.

I'm trying to keep things professional here, but I've been dismissed in the past for bringing this up, and I've struggled around this "feature" in generated code (YES, even typescript can have generated code) but I've wasted months of my life successfully working around this, only to have that work be in vain, as this feature grows in complexity from Istanbul's part.

This is the single most frustration I've ever had with ANY project in my nearly 20 years programming. Please don't act like the projects that are now dependent on workflows from Jest and other mainstream that are dependent on Istanbul don't exist. They do, and because of the feature set that Javascript expose, namely new Function(), and the support that Istanbul is supposed to back: "a JS code coverage tool written in JS", this use case MUST be supported.

Lastly, a simple ternary is nothing.

What you have now:

(cov_14jyvcatev().f[0]++)

What we need:

(typeof cov_14jyvcatev!=='undefined'?cov_14jyvcatev().f[0]++:void)

@HaruKawamata
Copy link
Author

Hey @robertleeplummerjr, I totally agree - great comment. If you want to get more eyes on this, I think it would be best to post the same comment on this issue instead: istanbuljs/istanbuljs#499. This repo is the old istanbul repo which is no longer maintained.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants