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

Comments are reported as not covered #238

Open
HarelM opened this issue Jan 15, 2024 · 11 comments · May be fixed by #244
Open

Comments are reported as not covered #238

HarelM opened this issue Jan 15, 2024 · 11 comments · May be fixed by #244

Comments

@HarelM
Copy link

HarelM commented Jan 15, 2024

I recently was trying to add coverage to my end to end tests.
I used this tool since my e2e run with puppeteer.
The code can be found here:
https://github.com/maplibre/maplibre-gl-js/blob/f197991152117c6e3964f824ebb6e3cec7d2d218/test/integration/render/run_render_tests.ts#L795C51-L795C51

When looking at the coverage in codecov I noticed that some lines were missing coverage, most of the line that I saw were comments and empty lines:
image

The coverage report here is a merge of the run in e2e and unit test (which uses jest).
When I tried to understand the difference I saw that they don't report the same way.
In jest the report is only on the "code line" and comments and empty lines are not part of the report, while when using v8 I get all the lines and those that are no covered, even if they are empty lines or comments are still reported as missing.

Is there something wrong with my setup?
Is this a known issue?

Here are the two main json coverage report files:
Jest report:
coverage-final.json
v8-to-istanbul
coverage-render.json

To see how things behave I looked at the scroll_zoom.ts part of the report.

Let me know if there's anything I can provide to help investigate this.

@cenfun
Copy link

cenfun commented Jan 18, 2024

try using playwright instead of puppeteer. puppeteer can not provide raw v8 coverage data, or using puppeteer-to-istanbul

@HarelM
Copy link
Author

HarelM commented Jan 18, 2024

Thanks for the tip! Are you sure about puppeteer not providing raw coverage? As the method name that I linked to my code is a new edition in v8 that output raw coverage as far as I understand, but I don't understand much...
https://github.com/maplibre/maplibre-gl-js/blob/f197991152117c6e3964f824ebb6e3cec7d2d218/test/integration/render/run_render_tests.ts#L795C51-L795C51

@cenfun
Copy link

cenfun commented Jan 18, 2024

Yes, puppeteer can provide raw v8 coverage data with option includeRawScriptCoverage
Is it still not working?

@HarelM
Copy link
Author

HarelM commented Jan 18, 2024

It works, but it doesn't work well, it reports comments and empty lines as not covered.

@cenfun
Copy link

cenfun commented Jan 18, 2024

There should be a bug somewhere, it could possibly be a problem with the sourcemap, see this issue, but it seems to be hard to fix. Therefore, we have implemented our own coverage report, see here.
see codecov example: https://app.codecov.io/gh/cenfun/playwright-ct-vue/blob/main/src%2FApp.vue
The comment lines and blank lines will be ignored.

@HarelM
Copy link
Author

HarelM commented Jan 18, 2024

Thanks, I guessed the same regarding sourcemap or conversion issue.
I'll check your links, thanks!

@bcoe
Copy link
Member

bcoe commented Jan 23, 2024

@cenfun if you want to open an issue on this repository, I'm open to moving towards retiring the guts of v8-to-istanbul in favour of your implementation, if we can show:

  • It works with all existing test cases (both for v8-to-istanbul and c8).
  • It works on large codebases like Node.js.
  • It uses a reasonable set of dependencies.

I'm worried the AST might fail in certain edge-cases, but open to this as being a good potential solution.

@cenfun
Copy link

cenfun commented Jan 24, 2024

@bcoe Thanks for your suggestion. My implementation simply provides a custom coverage report for Playwright, and we don't want to use Istanbul to instrument the source code because it's very slow, V8 is faster.
But I never intended to replace tool v8-to-istanbul, because they have different functions. My implementation is more about displaying the native V8 report directly.

  • Bytes coverage
  • CSS Coverage

There is gist here: https://gist.github.com/cenfun/0754c23f03d1aa541b4467920ab4d09f

@AriPerkkio
Copy link
Contributor

Would it work if lines that are not present in source maps would be excluded from the report? There would be no need for AST.

There's related PR in #240 but that one attempts to detect lines using regexp.

@cenfun
Copy link

cenfun commented Feb 14, 2024

@AriPerkkio Just sharing my experience how to exclude blank lines and comment lines.

BTW, the problem can't be solved by using regular expressions. see example if the comment is inside a string. That's why I implemented comment-parser.js It seems to work very well.

image

@AriPerkkio
Copy link
Contributor

These implement comment detection by parsing the content characted-by-character? Handling all weird edge cases that JS syntax allows might be difficult.

Here is another attempt based on the idea from #238 (comment) - ignore all lines that are not present in source maps. Left side is before, right one is after:

image

This is all done in ~13 lines of code using @jridgewell/trace-mapping which is already a dependency of v8-to-istanbul. No need for AST or parsing the source file for whitespace.

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

Successfully merging a pull request may close this issue.

4 participants