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

[BUG] Incorrect code coverage when converting from V8 to istanbul #50

Closed
edumserrano opened this issue Jun 17, 2023 · 7 comments
Closed

Comments

@edumserrano
Copy link

edumserrano commented Jun 17, 2023

Hi @cenfun,

Bug description

I mainly use the V8 code coverage when working in my dev machine and all works great. However, I have a requirement to upload code coverage to SonarQube and it requires an lcov file. When I enable the monocart-reporter option to get the lcov file as such:

coverage: {
    toIstanbul: true,
    lcov: true,
    ...
},

The code coverage that I get is incorrect. Please see screenshots below for more information. The code coverage screenshots were produced by running with:

  • babel + istanbul: this shows correct code coverage.
  • v8 + monocart reporter without the toIstanbul property set which defaults to false: this shows correct code coverage.
  • v8 + monocart reporter with the toIstanbul property set to true: this shows INCORRECT code coverage.
babel+istanbul

Note
The code coverage below is correct

app/core/ui-services/ui-helper-functions.ts
image

app/features/find-institution/institutions-list/institutions-list.component.ts
image

v8 + monocart reporter without the toIstanbul property set which defaults to false

Note
The code coverage below is correct

src/app/core/ui-services/ui-helper-functions.ts
image

src/app/features/find-institution/institutions-list/institutions-list.component.ts
image

v8 + monocart reporter with the toIstanbul property set to true

Warning
The code coverage below is INCORRECT. Some lines are showed as covered when they should not be.

app/core/ui-services/ui-helper-functions.ts
image

app/features/find-institution/institutions-list/institutions-list.component.ts
image

How to reproduce

  1. Checkout the temp branch at edumserrano/monocart-code-coverage-demo/tree/temp.
  2. run npm install to install all the required packages

To get the babel+istanbul code coverage:

  1. run npm run test-istanbul. The first time tests will fail because of missing snapshots, you can ignore this.
  2. run npm run nyc-report.
  3. run npm run open-istanbul-coverage.

To get the v8 + monocart reporter without the toIstanbul property set which defaults to false code coverage:

  1. run npm run test-monocart. The first time tests will fail because of missing snapshots, you can ignore this.
  2. run npm run open-monocart-coverage.

To get the v8 + monocart reporter with the toIstanbul property set to true code coverage:

  1. go to the playwright.confi.ts for the monocart setup and uncomment the lines:
// toIstanbul: true,
// lcov: true,
  1. run npm run test-monocart. The first time tests will fail because of missing snapshots, you can ignore this.
  2. run npm run open-monocart-coverage.

More notes

To clarify, the problem is that when using toIstanbul set to true the code coverage conversion marks some lines as covered when they should not be.

Let me know if you need more information.

@cenfun
Copy link
Owner

cenfun commented Jun 17, 2023

if toIstanbul = true, the v8-to-istanbul will take the conversion work.
seems it has bug, i'm not very familiar with it, could you please go here to find out if this bug already exists
https://github.com/istanbuljs/v8-to-istanbul/issues

If they can't fix the bug, maybe there is another way is convert v8 format to lcov format directly, but I don't know anything about lcov now.

@edumserrano
Copy link
Author

Got it, I've reported this issue at istanbuljs/v8-to-istanbul#216.
Let's see what they say.

@edumserrano
Copy link
Author

edumserrano commented Jun 18, 2023

For anyone else reading this thread, I'd like to leave here feedback about a workaround I've used until the issue the the v8-to-istanbul conversion is solved.

What I did was have 2 code coverage related fixtures as part of my playwright tests:

  • one of the fixtures instruments the code with v8 and I also use monocart-reporter configured with global code coverage which makes use of the v8 instrumentation. This is my preferred code coverage reporting experience and the one I use on the day to day dev.
  • one of the fixture instruments the code with istanbul. To do this I use the babel-plugin-istanbul to get istanbul instrumentation added to my Angular app and then the fixture saves that code coverage into a folder which is then used by the nyc tool to produce a cobertura and lcov files. These files are what I need to upload to my Azure DevOps pipeline and SonarQ.

To understand more about how you could implement the above please read this comment on microsoft/playwright#7030.

For a code example read the README and see the code on the edumserrano/monocart-code-coverage-demo repo. My workaround is essentially the merging of both configurations and fixtures on /tests/babel-istanbul and [/tests/monocart(https://github.com/edumserrano/monocart-code-coverage-demo/tree/main/tests/monocart).

A potential downside of this workaround is that I'm instrumenting the code twice. I haven't noticed any added slowness when running the tests but your mileage may vary. You could argue that instrumenting the code via istanbul only is enough since it gives me a code coverage report which I could attach to the monocart-reporter but I really like the user experience given by the monocart-reporter when a v8 report is available.

@azamat-sharapov
Copy link

@edumserrano I'm guessing you are using the workaround for normal playwright tests, not Component Testing right?

@edumserrano
Copy link
Author

@azamat-sharapov yes, not doing component testing as far as I'm aware. Doing "normal" playwright tests. The sample repo I created shows the kind of tests I'm doing.

Also I'm not sure what you mean by "the workaround for normal playwright tests"...

@cenfun
Copy link
Owner

cenfun commented Jun 27, 2023

@edumserrano It's been a week, but there is no update on the issue.
I did some research and found that the conversion is really complicated, it seems that the v8-to-istanbul library can't resolve this issue, so I rewrote a new own converter, which should fix this issue. (monocart doesn't use v8-to-istanbul anymore, feel free to close that issue istanbuljs/v8-to-istanbul#216 )
please try monocart-reporter@1.7.2

@edumserrano
Copy link
Author

@cenfun you're a genius! 👏

I can confirm this is now all working properly! Well done, thank you very much 🎉

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

No branches or pull requests

3 participants