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

[Question] Odd coverage of catch #41

Closed
mrazauskas opened this issue Jun 15, 2024 · 5 comments
Closed

[Question] Odd coverage of catch #41

mrazauskas opened this issue Jun 15, 2024 · 5 comments

Comments

@mrazauskas
Copy link
Contributor

Minor problem. I noticed one catch marked as partially covered:

Screenshot 2024-06-15 at 14 29 54

Context. fs.watch is asynchronous api, hence signal is passed to it. Aborting the signal is the only way to close the watcher. That’s why AbortError is ignored in the catch block.

As you see code within the catch block is executed, but why it is marked as partially covered?

Hm.. or this is because of that for await loop? I mean, the try block is never executed till the end, because the loop is infinite. The execution can only jump to catch straight from for await. That could explain why there is partial mark.

@mrazauskas
Copy link
Contributor Author

In the other news, I just managed to generate and upload Codacy native coverage report. Now I use monocart-coverage-reports to transform native V8 coverage instead lcov generated by c8. There is a lot of nice improvements.

Would you be interested to land a PR implementing experimental Codacy native coverage report? Uploading via API endpoint is somewhat tricky, that’s why I call it experimental. But the code itself is trivial: https://github.com/tstyche/tstyche/blob/271f1632e724503d8affbc425707de9d0ca6f59d/scripts/merge-coverage-reports.js#L10

@cenfun
Copy link
Owner

cenfun commented Jun 15, 2024

Minor problem. I noticed one catch marked as partially covered:

It could be an incorrect position conversion with sourcemap.

  • original code: it equal to source code like the code in your screenshots.
  • generated code: it compiled with some build tool, such as Webpack, esbuild, ts-node, etc.

The position in the V8 coverage data should be based on generated code but not original code.
We need to convert the position of generated code to the position of original code with sourcemap. In other words, it seems that it is a meaningless catch partially covered, but the generated code could be something else.
It requires debugging to find out the root cause. Or could you provide a minimal reproduction or demo repo so I can help to debug it.

Would you be interested to land a PR implementing experimental Codacy native coverage report? Uploading via API endpoint is somewhat tricky, that’s why I call it experimental.

I'm interested in is that Codacy can support this feature: codacy/codacy-coverage-reporter#506
It is similar to codecov.json report, then we could provide build-in codacy.json report too.

@mrazauskas
Copy link
Contributor Author

Right. This can be caused by mapping. I check at the build file and it looks like this:

}
catch (error) {
    if (error instanceof Error && error.name === "AbortError") ;
}

I formatted the source code in the same fashion and got this:

Screenshot 2024-06-15 at 16 28 30

The end of the try block is never reached, but catch (error) is executed. And that is why } catch (error) { gets marked as partial hit in the OP example. That’s correct. Thanks for a hint.


Let’s wait for response from Codacy.

@cenfun
Copy link
Owner

cenfun commented Jun 15, 2024

😂 sounds reasonable, but I don't think it's possible to cover part of the word.
I just debugged this issue in your project tstyche and found it is a mapping issue.
open https://evanw.github.io/source-map-visualization/ and load tstyche.js and tstyche.js.map, the mapping shows
image
That's why letter c is uncovered.

@mrazauskas
Copy link
Contributor Author

Ah.. I missed those pink marks in the report. Indeed it is a mapping issue.

To be honest, I don’t like this catch block. It works correctly, but semantics are broken. I mean, ignoring some error, because it is not error feels wrong. So now I have one more reason to rewrite this code.

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