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.

Sign up for GitHub

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

Jump to bottom

Fix the issue with coverage inside of a TypeApply #18420

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Fix the issue with coverage inside of a TypeApply #18420

merged 1 commit into from
Aug 25, 2023

Conversation

javax-swing
Copy link

@javax-swing javax-swing commented Aug 18, 2023 •
edited
Loading

Based on looking into the issues that: #17239 was trying to solve.

It was discovered that TypeApply was throwing away parts of the expression tree, which meant that the coverage file contained references to things that could never be invoked.

Note that i'm not too familiar with opening pull requests in github, so my apologies if this is not correct

@javax-swing javax-swing mentioned this pull request Aug 18, 2023
Closed
@javax-swing javax-swing changed the title Fix the issue with coverage that was caused by information being lost… Fix the issue with coverage inside of a TypeApply Aug 19, 2023
mbovel
mbovel requested changes Aug 21, 2023
View reviewed changes
Copy link
Member

@mbovel mbovel 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 your PR, looks good to me.
Could you please 1. add a test in tests/coverage/pos demonstrating the fix, and 2. rebase your commits into a single one?

compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala Outdated Show resolved Hide resolved
tests/pos-with-compiler-cc/dotc/transform/InstrumentCoverage.scala Outdated Show resolved Hide resolved
@mbovel mbovel requested a review from KacperFKorban August 21, 2023 13:24
@javax-swing
Copy link
Author

@mbovel I've made the changes you suggested.

It seems that it was a bit more involved than adding a test into tests/coverage/pos as my changes didn't result in a change in the scoverage.coverage file, but rather in a runtime change.

So i've added the ability for any of the tests/coverage/run tests to include a $filename.measurement.check file. When present the CoverageTests suite will verify that the generated measurement file matches the check file (similar to the scoverage.check file)

This is optional behaviour for now.

javax-swing
javax-swing commented Aug 21, 2023
View reviewed changes
compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala
@@ -61,6 +61,26 @@ class CoverageTests:
val instructions = FileDiff.diffMessage(expectFile.toString, targetFile.toString)
fail(s"Coverage report differs from expected data.\n$instructions")

// measurement files only exist in the "run" category
Copy link
Author

Choose a reason for hiding this comment

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

This verifies the measurement file against the check file (if a measurement check file exists).

Mainly so that none of the other tests need tweaking

KacperFKorban
KacperFKorban reviewed Aug 22, 2023
View reviewed changes
Copy link
Member

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

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

The change looks good to me.
Added one small nitpick.

tests/pos-with-compiler-cc/dotc/transform/InstrumentCoverage.scala Outdated Show resolved Hide resolved
@mrooneytrimble @javax-swing
Fix the issue with coverage that was caused by information being lost…
e5b3d4d 
… with a TypeApply

Add the ability to verify measured coverage invocations in CoverageTests
KacperFKorban
KacperFKorban approved these changes Aug 22, 2023
View reviewed changes
Copy link
Member

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

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

LGTM

@KacperFKorban KacperFKorban requested a review from mbovel August 22, 2023 19:53
@javax-swing
Copy link
Author

Thanks for your PR, looks good to me. Could you please 1. add a test in tests/coverage/pos demonstrating the fix, and 2. rebase your commits into a single one?

done

mbovel
mbovel approved these changes Aug 25, 2023
View reviewed changes
Copy link
Member

@mbovel mbovel left a comment

Choose a reason for hiding this comment

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

LGTM!

@sjrd sjrd merged commit 2616c8b into scala:main Aug 25, 2023
@javax-swing javax-swing deleted the adjust-coverage branch August 25, 2023 12:37
@TheElectronWill
Copy link
Contributor

thanks for the fix!

@KacperFKorban KacperFKorban mentioned this pull request Sep 4, 2023
Closed
@rolandtritsch rolandtritsch mentioned this pull request Sep 30, 2023
Closed
@Kordyjan Kordyjan mentioned this pull request Dec 11, 2023
Merged
Kordyjan added a commit that referenced this pull request Dec 13, 2023
@Kordyjan
Backport "Fix the issue with coverage inside of a TypeApply" to LTS (#…
234d0f2 
…19233)

Backports #18420 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan added this to the 3.3.2 milestone Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KacperFKorban KacperFKorban KacperFKorban approved these changes

@mbovel mbovel mbovel approved these changes

Assignees

@mbovel mbovel

Labels
None yet
Projects
None yet
Milestone
3.3.2
Development

Successfully merging this pull request may close these issues.
7 participants
@javax-swing @TheElectronWill @mbovel @KacperFKorban @sjrd @Kordyjan @mrooneytrimble