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 code coverage in specific path mode #44625

Merged
merged 5 commits into from
Mar 21, 2022

Conversation

IanButterworth
Copy link
Member

Fixes #44593

@IanButterworth IanButterworth added the backport 1.8 Change should be backported to release-1.8 label Mar 15, 2022
@IanButterworth IanButterworth requested a review from vtjnash March 15, 2022 17:23
Comment on lines 370 to 371
m = moduleroot(m)
mpath = pathof(m)
Copy link
Member

Choose a reason for hiding this comment

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

Is this how we define the path for coverage? I thought it was managed per-line in the table by the caller

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think it was possible to be more specific in this function? Perhaps something other than this function is needed?

Copy link
Member Author

@IanButterworth IanButterworth Mar 16, 2022

Choose a reason for hiding this comment

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

I've tried to make coverage info emission file-based but I'm hitting a BoundsError, so it appears the index handling needs more thought

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out doing it on a line by line basis, so now it's enabled for all lines in a CodeInfo if any of the lines fall in tracked files. Tests pass locally.

@IanButterworth IanButterworth force-pushed the ib/fix_code_cov branch 2 times, most recently from 36bbb59 to 82a316e Compare March 16, 2022 05:51
@IanButterworth
Copy link
Member Author

This fixes the codecov tests, and running the Octavian tests locally with CI enabled take the same time as master, so seems to not have a noticeable performance hit.

base/options.jl Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Member Author

@vtjnash I'm planning to merge this tomorrow, just in case you have any review/objection before then

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Lgtm. Some small comments

base/options.jl Outdated Show resolved Hide resolved
base/compiler/optimize.jl Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Member Author

Linux32 and win32 failures appear unrelated.

The buildkite hang seems like a glitch? @DilumAluthge

@IanButterworth IanButterworth changed the title Fix code cov for specific path Fix code coverage in specific path mode Mar 21, 2022
@IanButterworth IanButterworth merged commit feb7b77 into JuliaLang:master Mar 21, 2022
@IanButterworth IanButterworth deleted the ib/fix_code_cov branch March 21, 2022 01:28
KristofferC pushed a commit that referenced this pull request Mar 21, 2022
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Mar 21, 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.

Requesting code coverage for a specific path misses lines
3 participants