-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add coverage support #13
base: main
Are you sure you want to change the base?
Conversation
src/index.js
Outdated
@@ -23,6 +33,36 @@ export default class LightRunner { | |||
}); | |||
} | |||
|
|||
#filterCoverage(result, projectConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing it here to have access to this.#config
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 })), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
I'm not near my laptop, will test later. Good job! |
Thanks! I will try reviewing this later today. Hopefully then we'll be able to move Babel from istanbul to v8's coverage tracker 🤞 |
It's probably different (counts differently), but afaik there's no bugs with it. Feel free to ping me if there are |
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 |
This comment was marked as outdated.
This comment was marked as outdated.
wow, what a useless error message 😅 |
@fisker pushed again, works on node 12 on my machine |
All good, got the coverage https://github.com/prettier/prettier/runs/5863566812?check_suite_focus=true#step:6:1139 Thanks again! |
awesome! it looks like expected coming from c8? EDIT: https://github.com/prettier/prettier/runs/5863670359 is promising |
oh, haha |
Right, seems to be missing coverage of the CLI. You probably spawn it instead of executing from within a test? How is that on |
Our next branch not running CLI test, remember this prettier/prettier#12271 ? |
Aha! 😀 And the light runner doesn't solve the issue? |
The CLI test need features from Jest that this runner don't support 😄 |
We are using different runners, only format test use this runner https://github.com/prettier/prettier/blob/001f8fb086eefb73abefa46eba20370d5f0e84f0/jest-format-test.config.mjs#L5 |
This is not important, but just a question, why this |
I'm guessing because c8 only hooks up the inspector once instead of per test |
src/index.js
Outdated
const { collectCoverage, coverageProvider } = this.#config; | ||
|
||
if (collectCoverage && coverageProvider !== "v8") { | ||
throw new Error("Coverage needs v8 coverage provider"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be better to just log a warning instead of throwing, i.e. in the case where multiple projects are used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer either throw an error, or ignore settings, just use v8
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People only care where is my coverage report? Who care where is it from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
It looks like this breaks Babel's coverage collection, making it go down from ~90% to 0%. I just added |
6dfa6f9 is the minimum to "unbreak" Babel's tests. Feel free to revert it if you know what is the root cause / proper fix 😅 |
@nicolo-ribaudo seems fine 🙂 But coverage for the babel repo is broken using v8 coverage? sounds like a bug I need to fix 😅 |
Could it be because we test the compiled files, but want coverage in the original sources? |
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 |
But Prettier use the source directly, the coverage still lower. |
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 |
I have simple tests in my repo. You can check it out: psychobolt/yeoman-generator-boilerplate#20 |
Rebased this FWIW. I haven't had the time to dig into why it's different |
I have a possibly related issue, when I reset jest-light-runner/src/worker-runner.js Line 246 in 83da92a
|
I debugged in babel. I also found a problem. |
That last one sounds like a bug in jest - would you mind opening an issue (in jest)? Reproduction of babel repo is fine |
OK, I will do it. |
Unfortunately cannot reproduce in |
Huh. So it's something in this runner |
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. |
I found that I don't seem to have to do anything because |
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 😅 |
That is, the source code is in Also I seem to find a real bug, when I enable a different number of Update: It doesn't seem to be related to |
It seems to be closer to the real cause. update: It seems like a better approach at the moment is to use |
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 |
Note that this doesn't really work - the commented outI think it's readyfilter
needs to be there in some way.However, copying this over into prettier seems to work fine.
Fixes #6.