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

Inconsistent output with --coverage #7176

Open
azz opened this issue Oct 16, 2018 · 16 comments
Open

Inconsistent output with --coverage #7176

azz opened this issue Oct 16, 2018 · 16 comments

Comments

@azz
Copy link
Contributor

azz commented Oct 16, 2018

🐛 Bug Report

When code-covered files exist in the root directory, the --coverage output will include the name of the directory. This is not desirable in CI scenarios where the working directory will vary.

Actual

https://github.com/azz/jest-istanbul-issue/tree/master

When a file exists at root (above), the output is:

$ jest --coverage
 PASS  ./index.test.js
 PASS  src/file.test.js
-------------------------|----------|----------|----------|----------|-------------------|
File                     |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-------------------------|----------|----------|----------|----------|-------------------|
All files                |      100 |      100 |      100 |      100 |                   |
 jest-istanbul-issue     |      100 |      100 |      100 |      100 |                   |
  index.js               |      100 |      100 |      100 |      100 |                   |
 jest-istanbul-issue/src |      100 |      100 |      100 |      100 |                   |
  file.js                |      100 |      100 |      100 |      100 |                   |
-------------------------|----------|----------|----------|----------|-------------------|

Test Suites: 2 passed, 2 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        1.983s
Ran all test suites.
Done in 2.98s.

Note the jest-istanbul-issue above.

Expected

https://github.com/azz/jest-istanbul-issue/tree/good-case

When it doesn't:

$ jest --coverage
 PASS  src/file.test.js
  √ it works (1ms)

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |      100 |      100 |      100 |      100 |                   |
 file.js  |      100 |      100 |      100 |      100 |                   |
----------|----------|----------|----------|----------|-------------------|
Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        1.708s
Ran all test suites.
Done in 2.76s.
@SimenB
Copy link
Member

SimenB commented Oct 16, 2018

IMO the second case should include /src as well. So everything is always relative to the root of the project rootDir.

@SimenB
Copy link
Member

SimenB commented Oct 19, 2018

@bcoe could you help out here? Is it possible to change the base reporting directory? I found https://github.com/gotwarlost/istanbul/blob/bc84c315271a5dd4d39bcefc5925cfb61a3d174a/lib/util/file-matcher.js, but setting a breakpoint in it doesn't seem to change anything.

Our code: https://github.com/facebook/jest/blob/22f67d49ffcce7a5b6d6891438b837b3b26ba9db/packages/jest-cli/src/reporters/coverage_reporter.js#L90-L104

I tried this, makes no difference:

diff --git i/packages/jest-cli/src/reporters/coverage_reporter.js w/packages/jest-cli/src/reporters/coverage_reporter.js
index 7d3fffe2a..9711378b5 100644
--- i/packages/jest-cli/src/reporters/coverage_reporter.js
+++ w/packages/jest-cli/src/reporters/coverage_reporter.js
@@ -21,7 +21,7 @@ import type {Context} from 'types/Context';
 import type {Test} from 'types/TestRunner';
 
 import {clearLine, isInteractive} from 'jest-util';
-import {createReporter} from 'istanbul-api';
+import {config as istanbulConfig, createReporter} from 'istanbul-api';
 import chalk from 'chalk';
 import istanbulCoverage from 'istanbul-lib-coverage';
 import libSourceMaps from 'istanbul-lib-source-maps';
@@ -87,7 +87,12 @@ export default class CoverageReporter extends BaseReporter {
       this._coverageMap,
     );
 
-    const reporter = createReporter();
+    // TODO: Figure out the common base of all of them
+    const root = Array.from(contexts)[0].config.rootDir;
+
+    const cfg = istanbulConfig.loadFile(null, {instrumentation: {root}});
+
+    const reporter = createReporter(cfg);
     try {
       if (this._globalConfig.coverageDirectory) {
         reporter.dir = this._globalConfig.coverageDirectory;

@flovilmart
Copy link
Contributor

The reporter is text and the printed name make use of getRelativeName.
From reading in the implementation, this seems consistent with what is described in the issue (i.e. I don't believe there is a bug). The implementation of getRelativeName is referring to the parent level of the topmost name. Likely, this is an issue in the Istanbul reporter implementation and not in jest itself.

@SimenB
Copy link
Member

SimenB commented Feb 15, 2020

@coreyfarrell thoughts on this one?

@coreyfarrell
Copy link
Contributor

Probably would make sense to report relative to the detected/declared project root and also label that as "Project Root" instead of naming the FS directory. That said it's been this way forever and people tend to be pretty sensitive to reporting changes so I'm not sure I'd want to change this in a semver-minor. I'm otherwise occupied currently, it would help me if someone posted a bug to github.com/istanbuljs/istanbuljs/ and link to this issue.

@flovilmart
Copy link
Contributor

In my opinion I don’t see this as a bug. The coverage will always report relative to the folder containing the topmost reported coverage, not related to the current working directory as initially reported.

@SimenB
Copy link
Member

SimenB commented Feb 16, 2020

I agree it's not a bug in that it behaves as intended - I do think the behavior is not ideal, though.

I think we should be able to set a root, and that all paths should be relative to that regardless of what istanbul considers "top of the tree". If I do jest some-deep-test --coverage that only covers a subtree of files, check html coverage, run some other test that makes the tree different, a refresh of the html report should not show me the old file just because istanbul found a file higher up in the FS (or 404 if we had a cleanup between runs).

I can open up an issue tomorrow 👍

@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@bcoe
Copy link
Contributor

bcoe commented Feb 26, 2022

👋 bringing this thread back to life, would happily take a patch to istanbuljs/istanbuljs to make this configurable.

@github-actions github-actions bot removed the Stale label Feb 26, 2022
@SimenB
Copy link
Member

SimenB commented Feb 26, 2022

That'd be awesome! Do you have any pointers to where we would make changes so a root option (or something else, naming is hard) would go?

@bcoe
Copy link
Contributor

bcoe commented Feb 26, 2022

@SimenB I can dig a little bit tomorrow, I'm honestly not too sure.

If you could figure out how to provide a minimal example in a repo of behavior that has the wrong root, it would help me dig.

@SimenB
Copy link
Member

SimenB commented Feb 26, 2022

I would assume the repository in the OP still reproduces? Should update jest to latest to ensure we're using the latest versions of istanbul etc, but beyond that it should be fine?

@G-Rath
Copy link
Contributor

G-Rath commented Apr 29, 2022

@SimenB does the issue in eslint-plugin-jest serve as a reproduction?

@SimenB
Copy link
Member

SimenB commented Apr 29, 2022

probably not, not exactly minimal. Just updating the one in the OP to latest Jest should be enough tho (unless @bcoe wants a version without Jest at all)

@G-Rath
Copy link
Contributor

G-Rath commented Apr 29, 2022

I was able to reproduce this with the original repo after upgrading to jest v28.

I've also been able to create a even smaller reproduction that doesn't use jest at all: https://github.com/G-Rath/jest-istanbul-repo

It has two coverage maps that were generated from the original reproduction repo just with no code in the index.js and file.js files to make the maps as small as possible - coverage-map-one-file.json is generated by running jest file --coverage and coverage-map-two-file.json is generated by running jest --coverage.

If you call index.js coverage-map-one-file.json it'll generate an html coverage report with this structure:

coverage
├── base.css
├── block-navigation.js
├── favicon.png
├── file.js.html
├── index.html
├── prettify.css
├── prettify.js
├── sort-arrow-sprite.png
└── sorter.js

0 directories, 9 files

If you call index.js coverage-map-two-file.json it'll generate an html coverage report with this structure:

coverage
├── base.css
├── block-navigation.js
├── favicon.png
├── index.html
├── index.js.html
├── prettify.css
├── prettify.js
├── sort-arrow-sprite.png
├── sorter.js
└── src
    ├── file.js.html
    └── index.html

1 directory, 11 files

The desired behaviour (which this issue is about) is that both of these coverage reports should have file.js.html located in src.

@SimenB
Copy link
Member

SimenB commented Apr 30, 2022

Awesome! Yeah, or at least if passed some sort of root option. Would you mind filing an issue in the istanbul repo? Then whenever that's fixed, we can use that option (or update the version if it's made the default) and close this issue here 🙂

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

6 participants