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] Lines are marked as covered in lcovonly, but they are not executed #37

Closed
mrazauskas opened this issue Jun 12, 2024 · 16 comments
Closed

Comments

@mrazauskas
Copy link
Contributor

Describe the bug

lcovonly and v8 reports are generated. All is marked correctly in v8 (great UI, by the way):

Screenshot 2024-06-12 at 16 05 40

The lcov report is uploaded to Codacy and in their UI one can see that line which was not executed (test are running on Ubuntu, so the else branch is not reachable) got marked as covered:

Screenshot 2024-06-12 at 15 58 04

Here is data for this file from lcov.info:

SF:source/path/Path.ts
FN:8,(anonymous_0)
FN:10,(anonymous_1)
FN:14,(anonymous_2)
FN:18,(anonymous_3)
FN:22,(anonymous_4)
FN:32,(anonymous_5)
FNF:6
FNH:5
FNDA:4358,(anonymous_0)
FNDA:0,(anonymous_1)
FNDA:890,(anonymous_2)
FNDA:1303,(anonymous_3)
FNDA:849,(anonymous_4)
FNDA:612,(anonymous_5)
DA:3,293
DA:7,293
DA:8,293
DA:10,293
DA:15,890
DA:19,1303
DA:23,849
DA:25,849
DA:26,849
DA:29,849
DA:33,612
LF:11
LH:11
BRDA:7,0,0,293
BRDA:7,0,1,0
BRDA:25,1,0,849
BRDA:25,1,1,0
BRF:4
BRH:2
end_of_record
TN:

I do not understand this data unfortunately. Hopefully it helps to understand, if Codacy’s report is correct or not?

To Reproduce

  • Version: latest
  • Node Version: LTS
  • Have you tried the latest version (including node), and does the issue still exist? yes

Expected behavior

Same output in both reports.

Additional context

Codacy coverage report can be seen here: https://app.codacy.com/gh/tstyche/tstyche/coverage/files/117115485336?bid=42144053&fileBranchId=42144053

@mrazauskas
Copy link
Contributor Author

I drop the lcov.info into https://lcov-viewer.netlify.app. Here is the result:

Screenshot 2024-06-12 at 16 12 39

Looks odd that lines can have full coverage while branches and functions are not fully covered. But, as it was mentioned, I do not understand lcov.

@mrazauskas
Copy link
Contributor Author

If that is helpful, here is console-details output:

Screenshot 2024-06-12 at 16 37 28

The last line is the important one. Here lines are not reported as 100% covered and that is correct. Or?

@cenfun
Copy link
Owner

cenfun commented Jun 12, 2024

Seems there is a bug in lcov lines coverage, I will check it.

v8 report should be the best one, as it directly uses native V8 coverage data, while lcov needs to convert V8 to Istanbul, which may lose accuracy.

By the way, do you have the opportunity to use codecov instead of codacy, because the report codecov is directly converted from V8, it seems it is right in codecov.json
image

@cenfun
Copy link
Owner

cenfun commented Jun 12, 2024

Yes, console-details is directly uses native V8 coverage data too.

@mrazauskas
Copy link
Contributor Author

Indeed, it is tempting to switch to Codecov. I saw links in your readme and was comparing Codecov's and Codacy’s reports. Generating codecov.json directly sounds like the best solution. I was using Codecov some time ago. Hard to decide to come back, but I will think about that.

Anyway, I though it might help to report a bug as well.

@mrazauskas
Copy link
Contributor Author

Interesting. I was looking through Codacy's documentation, apparently internally they have their own protocol similar to Codecov. See here: https://api.codacy.com/swagger#tocscoveragereport

It is possible to upload coverage in that format via API endpoint: https://api.codacy.com/swagger#savecoverage

I will drop them a line asking, if they could make it possible uploading codacy.json via CLI as well. Or I could write custom reporter (looks rather trivial task, did I find the right lines?) and simply use curl. That sounds fun ;D

@cenfun
Copy link
Owner

cenfun commented Jun 12, 2024

Good found, I didn't notice this before. Maybe I could provide build-in codacy.json report for MCR next time.
Please feel free to write and use custom report. It should be easy, see example

{
    reports: [
        [path.resolve('./test/custom-v8-reporter.js'), {
            type: 'v8',
            outputFile: 'custom-v8-coverage.json'
        }]
    ]
}

https://github.com/cenfun/monocart-coverage-reports/blob/main/test/custom-v8-reporter.js

codecov example: handleCodecovReport
https://github.com/cenfun/monocart-coverage-reports/blob/main/lib/generate.js#L263

@cenfun
Copy link
Owner

cenfun commented Jun 13, 2024

Good news is I found out the root cause which is replated to static initialization blocks
There are different range between v8 and AST, so we haven't got the right line coverage in lcov.
image
I need to figure out how to fix it and may require more time.

@mrazauskas
Copy link
Contributor Author

Thanks! Meanwhile I have send a feature request to Codacy: codacy/codacy-coverage-reporter#506

@mrazauskas
Copy link
Contributor Author

@cenfun I have just spotted another issue related with static initialization blocks. Might be it already got fixed in the commit you have push.

Here is v8 coverage. Look at calls count in the static block. 4358 initializations does not sound realistic, because there are only 331 test.

Below you can find lcov generated by c8. Funny result for line 5, but otherwise 293 initializations is the number I would expect:

Screenshot 2024-06-13 at 14 58 03
Screenshot 2024-06-13 at 14 58 26

@mrazauskas
Copy link
Contributor Author

Ah.. Here raw data.lines:

{
  "1": 1,
  "3": 1,
  "4": 293,  // correct
             // skipped line 5, so much correct!
  "6": 293,  // correct
  "7": 293,  // correct
  "8": 4358,  // hm.. and what is this?
  "9": "1/2",
  "10": 0,
  "11": 0,
  "12": 293,
  "14": 890,
  "15": 890,
  "16": 890,
  "18": 1303,
  "19": 1303,
  "20": 1303,
  "22": 849,
  "23": 849,
  "25": 849,
  "26": 849,
  "27": 849,
  "29": 849,
  "30": 849,
  "32": 612,
  "33": 612,
  "34": 612,
  "35": 1,
}

@cenfun
Copy link
Owner

cenfun commented Jun 14, 2024

Thanks! Meanwhile I have send a feature request to Codacy: codacy/codacy-coverage-reporter#506

Great. I've also tried uploading a Codacy-formatted JSON but wasn't successful. Otherwise, we could provide built-in Codacy report, just like Codecov.

I have just spotted another issue related with static initialization blocks. Might be it already got fixed in the commit you have push.

Yes. It should be fixed, see commit. A patch will be released, and I still have several other issues to solve together.

Here is v8 coverage. Look at calls count in the static block. 4358 initializations does not sound realistic, because there are only 331 test. Below you can find lcov generated by c8. Funny result for line 5, but otherwise 293 initializations is the number I would expect

c8 used v8-to-istanbul to convert the format to istanbul from V8, It's an excellent initiative to implement native V8 coverage reports. At the beginning, I also used v8-to-istanbul to implement the conversion. However, there are some issues that it seems unable to resolve. Because it just provide a simple conversion, not an accurate one. So I try to implement a better conversion which is analyzing the AST of the source code to achieve more accurate conversion. for example, excluding comments lines, blank lines and so on. see here for more history.

BTW, I have contributed MCR to c8 as an experimental feature, you can use it like below in the latest version.

c8 --experimental-monocart --reporter=v8 --reporter=console-details node foo.js

@cenfun
Copy link
Owner

cenfun commented Jun 15, 2024

@mrazauskas It should be fixed. Please try [email protected]

@mrazauskas
Copy link
Contributor Author

Thanks. Yes, it works now. Closing this issue.

I hope to find some time to play with Codacy’s API today or tomorrow. Let’s see.

And thanks once again for creating this library!

@cenfun
Copy link
Owner

cenfun commented Jun 15, 2024

@mrazauskas Sorry, seems that I missed a question.

{
  "1": 1,
  "3": 1,
  "4": 293,  // correct
             // skipped line 5, so much correct!
  "6": 293,  // correct
  "7": 293,  // correct
  "8": 4358,  // hm.. and what is this?
  "9": "1/2",

image

For line 8, there are two parts:

  • Path.normalizeSlashes = 293 hits maybe
  • (filePath) => filePath; 4358 hits (function executions)

Currently, it take the maximum hits as the hits of the line. Does this make sense?

@mrazauskas
Copy link
Contributor Author

Ah.. That makes sense now. I thought function executions will be marked on line 4. If V8 reports these on line 8, that is perfectly fine.

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

2 participants