feat: add coverage support #13

SimenB · 2022-04-06T13:40:08Z

~~Note that this doesn't really work - the commented out filter needs to be there in some way.~~ I think it's ready

However, copying this over into prettier seems to work fine.

Fixes #6.

SimenB · 2022-04-06T14:04:16Z

src/index.js

@@ -23,6 +33,36 @@ export default class LightRunner {
    });
  }

+  #filterCoverage(result, projectConfig) {


doing it here to have access to this.#config

SimenB · 2022-04-06T14:05:01Z

src/index.js

+      v8Coverage: result.v8Coverage
+        .filter(res => res.url.startsWith("file://"))
+        .map(res => ({ ...res, url: fileURLToPath(res.url) }))
+        .filter(
+          ({ url }) =>
+            // TODO: will this work on windows? It might be better if `shouldInstrument` deals with it anyways
+            url.startsWith(projectConfig.rootDir) &&
+            shouldInstrument(url, coverageOptions, projectConfig)
+        )
+        .map(result => ({ result })),
+    };


https://github.com/facebook/jest/blob/777e32687775b9287650c85ade4f9fdda1405fe1/packages/jest-runtime/src/index.ts#L1192-L1209

SimenB · 2022-04-06T14:14:37Z

@nicolo-ribaudo I have zero attachment to this particular approach, feel free to slice and dice it as needed 🙂 it seems to work fine, at least

fisker · 2022-04-06T14:49:40Z

I'm not near my laptop, will test later. Good job!

nicolo-ribaudo · 2022-04-06T15:36:18Z

Thanks! I will try reviewing this later today. Hopefully then we'll be able to move Babel from istanbul to v8's coverage tracker 🤞

SimenB · 2022-04-06T15:53:41Z

It's probably different (counts differently), but afaik there's no bugs with it. Feel free to ping me if there are

fisker · 2022-04-07T01:59:04Z

I got an error, I don't know what is it... https://github.com/prettier/prettier/runs/5861202752?check_suite_focus=true#step:8:14

SimenB · 2022-04-07T06:19:02Z

wow, what a useless error message 😅

SimenB · 2022-04-07T06:21:21Z

EDIT: 14.6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Working_With_Private_Class_Features#browser_compatibility

SimenB · 2022-04-07T06:25:52Z

@fisker pushed again, works on node 12 on my machine

fisker · 2022-04-07T06:43:43Z

All good, got the coverage https://github.com/prettier/prettier/runs/5863566812?check_suite_focus=true#step:6:1139

Thanks again!

SimenB · 2022-04-07T06:49:57Z

awesome! it looks like expected coming from c8?

EDIT: https://github.com/prettier/prettier/runs/5863670359 is promising

fisker · 2022-04-07T07:06:41Z

c8 says 96.5% https://github.com/prettier/prettier/runs/5850917095?check_suite_focus=true#step:6:1767
this one says 72.4% https://github.com/prettier/prettier/runs/5863566812?check_suite_focus=true#step:6:1142

SimenB · 2022-04-07T07:16:03Z

oh, haha

SimenB · 2022-04-07T07:17:33Z

Right, seems to be missing coverage of the CLI. You probably spawn it instead of executing from within a test? How is that on main?

fisker · 2022-04-07T07:20:25Z

Our next branch not running CLI test, remember this prettier/prettier#12271 ?
The 96.5% from c8 also not including CLI test.

SimenB · 2022-04-07T07:21:30Z

Aha! 😀 And the light runner doesn't solve the issue?

fisker · 2022-04-07T07:24:11Z

The CLI test need features from Jest that this runner don't support 😄

fisker · 2022-04-07T07:25:34Z

We are using different runners, only format test use this runner https://github.com/prettier/prettier/blob/001f8fb086eefb73abefa46eba20370d5f0e84f0/jest-format-test.config.mjs#L5

fisker · 2022-04-07T07:30:29Z

This is not important, but just a question, why this collectCoverage makes Jest much slower than running with c8?

SimenB · 2022-04-07T07:50:45Z

I'm guessing because c8 only hooks up the inspector once instead of per test

SimenB · 2022-04-07T08:56:24Z

src/index.js

+    const { collectCoverage, coverageProvider } = this.#config;
+
+    if (collectCoverage && coverageProvider !== "v8") {
+      throw new Error("Coverage needs v8 coverage provider");


it might be better to just log a warning instead of throwing, i.e. in the case where multiple projects are used

I'd prefer either throw an error, or ignore settings, just use v8.

People only care where is my coverage report? Who care where is it from.

It's ok with the error. If someone uses a different provider and decides to use jest-light-runner, it's better to notify them loudly.

nicolo-ribaudo · 2022-04-07T22:54:25Z

It looks like this breaks Babel's coverage collection, making it go down from ~90% to 0%. I just added coverageProvider: "v8" to https://github.com/babel/babel/blob/main/jest.config.js and run make test-ci-coverage.

nicolo-ribaudo · 2022-04-07T23:03:45Z

6dfa6f9 is the minimum to "unbreak" Babel's tests. Feel free to revert it if you know what is the root cause / proper fix 😅

SimenB · 2022-04-08T09:09:16Z

@nicolo-ribaudo seems fine 🙂 But coverage for the babel repo is broken using v8 coverage? sounds like a bug I need to fix 😅

nicolo-ribaudo · 2022-04-08T09:42:48Z

Could it be because we test the compiled files, but want coverage in the original sources?

SimenB · 2022-04-08T09:50:23Z

as long as there's a sourcemap that shouldn't be an issue. Jest has some custom mapping, maybe something like that is needed? https://github.com/facebook/jest/blob/a5f1ef43981b7426d61bcd7cbc59a79f548c075a/scripts/mapCoverage.mjs

fisker · 2022-04-08T09:52:21Z

But Prettier use the source directly, the coverage still lower.

SimenB · 2022-04-08T10:08:28Z

Some smaller reproduction than prettier or babel would be nice to investigate 🙂 I think this PR can still land in the meantime.

One interesting check would be to check coverage using v8 provider with the default jest runner and if it differs from the one from the light runner

psychobolt · 2022-04-08T14:29:56Z

I have simple tests in my repo. You can check it out: psychobolt/yeoman-generator-boilerplate#20

SimenB · 2022-09-10T15:11:50Z

Rebased this FWIW. I haven't had the time to dig into why it's different

liuxingbaoyu · 2022-09-11T00:40:32Z

c8 says 96.5% prettier/prettier/runs/5850917095?check_suite_focus=true#step:6:1767 this one says 72.4% prettier/prettier/runs/5863566812?check_suite_focus=true#step:6:1142

I have a possibly related issue, when I reset globalThis.__coverage__ before each test run, babel's coverage drops by 30%.
globalThis.__coverage__ = undefined
It seems that globalThis.__coverage__ is not being merged properly.

jest-light-runner/src/worker-runner.js

Line 246 in 83da92a

coverage: globalThis.__coverage__,

liuxingbaoyu · 2022-09-20T18:27:26Z

I debugged in babel.
Just removing shouldInstrument() produces coverage output (although coverage is currently around 10% less), since babel is precompiled and does not use jest-transform.
But this also makes coveragePathIgnorePatterns completely invalid, maybe we can move shouldInstrument() to after restoring the source file path using sourcemap?

I also found a problem.
babel-helper-transform-fixture-test-runner will run the dummy file on the vm, which will cause the jest _getCoverageResult method to throw an exception because the file cannot be read.
https://github.com/babel/babel/blob/53e9057d3022cdf41c2593261b8f5c4a4e26265d/packages/babel-helper-transform-fixture-test-runner/src/index.ts#L99-L107

SimenB · 2022-09-20T18:29:11Z

That last one sounds like a bug in jest - would you mind opening an issue (in jest)? Reproduction of babel repo is fine

liuxingbaoyu · 2022-09-20T18:31:13Z

OK, I will do it.

liuxingbaoyu · 2022-09-20T19:01:11Z

Unfortunately cannot reproduce in jest-runtime because dummy files are not in this._v8CoverageSources.😰
https://github.com/facebook/jest/blob/561907ce5d1e9065311cf585f75b9c5864036193/packages/jest-runtime/src/index.ts#L1181-L1190

SimenB · 2022-09-20T19:06:30Z

Huh. So it's something in this runner

liuxingbaoyu · 2022-09-20T19:13:16Z

Yes, it seems to be a compatibility issue.

I just applied some skips manually and now v8 and babel have the same result on my local (windows)!

I'll try to parse the source map next to accomplish this filtering.

liuxingbaoyu · 2022-09-20T19:27:33Z

I found that I don't seem to have to do anything because jest also doesn't support collectCoverageFrom=="packages/*/src/**/*.{js,cjs,mjs,ts}", I just need to use collectCoverageFrom== "packages/*/lib/**/*.{js,cjs,mjs,ts}" is enough.

SimenB · 2022-09-20T21:20:59Z

What do you mean with "doesn't support"? Is the pattern not supported? Not respected? Sounds like a bug still, but somewhat hard to nail down 😅

liuxingbaoyu · 2022-09-20T21:27:40Z

That is, the source code is in ./src, and the compiled file is in ./lib.
collectCoverageFrom must be set to ./lib to work.
I think this is acceptable behavior whether it's a bug or not.

Also I seem to find a real bug, when I enable a different number of coverageReporters, the strange thing is that the coverage shown changes, it seems that some reporters are changing the original coverage data.

Update: It doesn't seem to be related to coverageReporters, but the v8 cause, even if I run it twice, the results will be different, very strange.

liuxingbaoyu · 2022-09-21T07:21:32Z

It seems to be closer to the real cause.
When I use maxWorkers=2, the result is 80%. When I use maxWorkers=16, the result is 90%.
This might also explain #13 (comment) and #13 (comment)

update: It seems like a better approach at the moment is to use c8, which also supports subprocesses.

Alx101 · 2022-12-23T09:55:02Z

Lifted over these changes into my own fork of jest-light-runner (along with some experiments for global setups before all test-suites, still not working). Since I'm running this on a TS project, i've noticed that the coverage report line numbers seem incorrect, probably it's not converting with the source-map?

I'm using "setupFiles": ["source-map-support/register"] in the jest config

SimenB mentioned this pull request Apr 6, 2022

Investigate --coverage support #6

Open

SimenB commented Apr 6, 2022

View reviewed changes

SimenB mentioned this pull request Apr 6, 2022

Try add coverageProvider: 'v8' prettier/prettier#12606

Draft

4 tasks

This comment was marked as outdated.

Sign in to view

SimenB commented Apr 7, 2022

View reviewed changes

SimenB mentioned this pull request Apr 26, 2022

Show details when error occurred during loading test files #27

Open

fisker mentioned this pull request Aug 2, 2022

Support ESM plugins prettier/prettier#13201

Merged

4 tasks

SimenB force-pushed the coverage branch from 5ec64f9 to d8e6d1b Compare September 10, 2022 15:10

almost working coverage

26447d7

SimenB force-pushed the coverage branch from d8e6d1b to 26447d7 Compare September 10, 2022 15:11

fisker mentioned this pull request Oct 15, 2022

Markdown[next branch]: Do not insert spaces between Chinese/Japanese & latin letters prettier/prettier#11597

Merged

4 tasks

feat: add coverage support #13

Are you sure you want to change the base?

feat: add coverage support #13

Conversation

SimenB commented Apr 6, 2022 • edited

SimenB Apr 6, 2022

Choose a reason for hiding this comment

SimenB Apr 6, 2022

Choose a reason for hiding this comment

SimenB commented Apr 6, 2022

fisker commented Apr 6, 2022

nicolo-ribaudo commented Apr 6, 2022

SimenB commented Apr 6, 2022 • edited

fisker commented Apr 7, 2022

This comment was marked as outdated.

SimenB commented Apr 7, 2022

SimenB commented Apr 7, 2022 • edited

SimenB commented Apr 7, 2022

fisker commented Apr 7, 2022

SimenB commented Apr 7, 2022 • edited

fisker commented Apr 7, 2022

SimenB commented Apr 7, 2022

SimenB commented Apr 7, 2022 • edited

fisker commented Apr 7, 2022 • edited

SimenB commented Apr 7, 2022

fisker commented Apr 7, 2022

fisker commented Apr 7, 2022

fisker commented Apr 7, 2022

SimenB commented Apr 7, 2022

SimenB Apr 7, 2022 • edited

Choose a reason for hiding this comment

fisker Apr 7, 2022

Choose a reason for hiding this comment

fisker Apr 7, 2022

Choose a reason for hiding this comment

nicolo-ribaudo Apr 7, 2022

Choose a reason for hiding this comment

nicolo-ribaudo commented Apr 7, 2022

nicolo-ribaudo commented Apr 7, 2022

SimenB commented Apr 8, 2022

nicolo-ribaudo commented Apr 8, 2022

SimenB commented Apr 8, 2022

fisker commented Apr 8, 2022

SimenB commented Apr 8, 2022

psychobolt commented Apr 8, 2022

SimenB commented Sep 10, 2022

liuxingbaoyu commented Sep 11, 2022

liuxingbaoyu commented Sep 20, 2022

SimenB commented Sep 20, 2022

liuxingbaoyu commented Sep 20, 2022

liuxingbaoyu commented Sep 20, 2022

SimenB commented Sep 20, 2022

liuxingbaoyu commented Sep 20, 2022

liuxingbaoyu commented Sep 20, 2022 • edited

SimenB commented Sep 20, 2022

liuxingbaoyu commented Sep 20, 2022 • edited

liuxingbaoyu commented Sep 21, 2022 • edited

Alx101 commented Dec 23, 2022 • edited

SimenB commented Apr 6, 2022 •

edited

SimenB commented Apr 6, 2022 •

edited

SimenB commented Apr 7, 2022 •

edited

SimenB commented Apr 7, 2022 •

edited

SimenB commented Apr 7, 2022 •

edited

fisker commented Apr 7, 2022 •

edited

SimenB Apr 7, 2022 •

edited

liuxingbaoyu commented Sep 20, 2022 •

edited

liuxingbaoyu commented Sep 20, 2022 •

edited

liuxingbaoyu commented Sep 21, 2022 •

edited

Alx101 commented Dec 23, 2022 •

edited