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

Multiple incompatible reports generated for the same file. #14

Closed
demurgos opened this issue Jul 29, 2018 · 4 comments · Fixed by #83
Closed

Multiple incompatible reports generated for the same file. #14

demurgos opened this issue Jul 29, 2018 · 4 comments · Fixed by #83

Comments

@demurgos
Copy link
Contributor

demurgos commented Jul 29, 2018

I am investigating a bug where the lcovonly reporter sometimes crashes. I cannot reproduce it reliably, but it happens about 60% of the time.

I was able to find part of the cause why it happens, but I need help to find the root cause and solve it.

The lcovonly report failed at this line because the branchMap and b properties of the file coverage had different keys. It is a broken invariant, meaning that the coverage map was malformed.

The bad file coverage object was for a file called test.esm.js. When looking into c8's tmp directory (coverage/tmp), I found two coverage file for test.esm.js: gist.
If you look at them, you see that they have different branchMap values. The first one has 2 properties, the second one has 4 properties.

When c8 builds its coverage map, it merges all the files in its temporary directory. One of the preconditions of the merge function is that if there are two reports for the same file, the file needs to have the same structure (ex. branchMap). https://github.com/istanbuljs/istanbuljs/blob/829e658dfa91e3a9533842be9ce940dbe7785c09/packages/istanbul-lib-coverage/lib/file.js#L246.


So here is the current state of my investigation: lcovonly breaks because it gets an invalid file coverage because multiple reports for the same file but with different structures are merged, thus breaking one of the preconditions of the merge function.

To solve this, c8 should ensure that the reports are unique for each file URL, and that they have the same structure.

@demurgos
Copy link
Contributor Author

In the code emitting the test reports, I added console.log(v8.url) and got:

/data/projects/various/turbo-gulp/build/test/test.esm.js
file:///data/projects/various/turbo-gulp/build/test/test.esm.js
file:///data/projects/various/turbo-gulp/build/test/e2e/node-lib/node-lib.spec.mjs
file:///data/projects/various/turbo-gulp/build/test/e2e/node-lib/meta.js
file:///data/projects/various/turbo-gulp/build/test/lib/project.mjs
file:///data/projects/various/turbo-gulp/build/test/lib/utils/node-async.mjs
file:///data/projects/various/turbo-gulp/build/test/lib/options/typescript.mjs
file:///data/projects/various/turbo-gulp/build/test/lib/options/tsc.mjs
file:///data/projects/various/turbo-gulp/build/test/lib/utils/utils.mjs
/data/projects/various/turbo-gulp/build/test/e2e/node-lib/meta.js
file:///data/projects/various/turbo-gulp/build/test/test/index.spec.mjs
file:///data/projects/various/turbo-gulp/build/test/test/utils/matcher.spec.mjs
file:///data/projects/various/turbo-gulp/build/test/lib/utils/matcher.mjs

As you see, the URLs are not consistent and test.esm.js is duplicated: once with file:// and once without. I do not understand why this inconsistency is there.

Extending the filter function with the following code fixes my issue:

if (!url.startsWith('file://')) {
  return false;
}

(ensure that the url has an explicit file protocol)

demurgos added a commit to demurgos/c8 that referenced this issue Jul 29, 2018
This commit ensures that the URL of the results starts with `file://`. Some files produce an extra result without the `file` protocol, this result interferes then without report merging.

See bcoe#14
demurgos added a commit to demurgos/c8 that referenced this issue Jul 29, 2018
This commit ensures that the URL of the results starts with `file://`. Some files produce an extra result without the `file` protocol, this result interferes then with report merging.

See bcoe#14
demurgos added a commit to demurgos/c8 that referenced this issue Jul 29, 2018
This commit ensures that the URL of the results starts with `file://`. Some files produce an extra result without the `file` protocol, this result interferes then with report merging.

See bcoe#14
@demurgos
Copy link
Contributor Author

demurgos commented Jul 29, 2018

This may be related to a CJS/ESM mismatch. The absolute paths seem to correspond to CJS files, and file:// to ESM files. The duplicated file is my entry point (CJS file using dynamic ES imports). I use --experimental-modules but it may be interpreted as CJS once? (meta.js is the only CJS file)

@TimothyGu
Copy link
Contributor

@demurgos The file:// version of CJS files is because of the Node.js CJS-ESM bridge.

@demurgos
Copy link
Contributor Author

demurgos commented Jul 30, 2018

Thanks. I assumed that the interface was created at a prior moment and only one file was evaluated, but it makes sense that both need to be passed to V8. It means that a reliable fix would be to detect this internal module and to not emit results for it.

I was mainly surprised because my entry point is a .js file, not .mjs.


I cannot reproduce it reliably, but it happens about 60% of the time.

This due to the use of UUIDs to generate unique file names. This introduces randomness in the system. If the file:// version is emitted with a name alphabetically before the output of the "absolute path" version, then it works, otherwise not. (If the one with "more" branches is read first, the second one can reuse them).

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

Successfully merging a pull request may close this issue.

4 participants