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

fix: add missing code coverage ranges that span only a single character #8911

Merged
merged 1 commit into from Sep 7, 2022

Conversation

smithc
Copy link
Contributor

@smithc smithc commented Sep 6, 2022

What kind of change does this PR introduce?

Bugfix

Summary

This change fixes an issue in the JavaScript code coverage collection algorithm that, under specific circumstances, can lead to incomplete coverage data.

The existing algorithm filtered out code coverage range results that spanned a single character, perhaps in the assumption that such characters could only represent trivia tokens. This is safe when such tokens represented whitespace, or newline trivia, but unfortunately it also had the effect of filtering out valid semantic tokens as well, such as the ternary operators ?/:.

Furthermore, this particular implementation meant that the resulting output of an otherwise repeatable code-coverage run would differ between manual collection via Chrome Dev Tools, and automated collection via Puppeteer, since the former would correctly include such single-character tokens, and the latter would not.

Links/Issues

While this fix was not introduced explicitly to solve for the following issue, a quick search yielded this existing report, and I believe that this may solve for the reported issue (or perhaps at least inform anybody following the issue for a thread to pull on):

Does this PR introduce a breaking change?

This fix introduces a change in behavior to how we collect coverage ranges. Previously, coverage ranges of a single character length were elided entirely from the coverage output. With this change, those ranges are not ignored, and are included in the resulting coverage output.

From the perspective of consumers, anybody relying on consistent output of coverage ranges from this algorithm (ex. unit tests that rely upon cached or saved copies of coverage data) will need to update their expectations to match these changes accordingly - in essence, consumers should now expect that trivia tokens (like newlines), or more importantly single-token operators (like the ternary false operator :) will be included as part of the coverage output. This aligns with the expected output of Chrome's code coverage tooling in Dev Tools.

Other information

Please feel free to reach out if there are any questions, and I'll be happy to provide further help or input as needed; I really appreciate all the hard work this team does in providing such an invaluable product!

@smithc smithc force-pushed the fix/missing-code-coverage-ranges branch from aff2dec to 2a554f8 Compare September 6, 2022 21:41
@OrKoN OrKoN self-requested a review September 7, 2022 05:16
@OrKoN OrKoN force-pushed the fix/missing-code-coverage-ranges branch from 2a554f8 to 35b20a4 Compare September 7, 2022 05:39
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Looks reasonable to me.

@OrKoN OrKoN enabled auto-merge (squash) September 7, 2022 05:40
@OrKoN OrKoN merged commit 0c577b9 into puppeteer:main Sep 7, 2022
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 this pull request may close these issues.

None yet

2 participants