-
Notifications
You must be signed in to change notification settings - Fork 704
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 test --enable-coverage for multi-package projects #7250
Conversation
This sounds like something we all will love! |
Rebase to fix CI |
A request: that failure was recently fixed in master |
Currently, if you have multiple packages in a project and try and run the test suite of a single package with --enable-coverage, hpc will fail to run. The problem is that _all_ dependencies of the package are built with `-fhpc`, but when we run `hpc markup`, we are only passing the `.mix` directory of the package being tested. Because we built all dependencies with `-fhpc` and we haven't excluded them from the report, we need to supply their `.mix` directories too. The above suggests one fix - compute the transitive closure of all `.mix` directories. However, there is another solution - change from using an exclude-list to using an include-list. This is the approach used in this commit. Explicitly enumerating all modules to _include_ in the report is simpler to code, but is also more likely to be what the user is interested in. Generally, when one generates a coverage report from a test-suite, they want to understand the coverage of the unit being tested. Having coverage information from dependencies is usually not relevant. This is also the behavior from old-style Cabal builds, where there wasn't even a notion of a Cabal project. Fixes haskell#5433.
@ocharles looks like it failed due to some name shadowing. Do you mind fixing? I'm happy to merge once those changes are fixed. |
This is missing a changelog entry. @ocharles could you add one in a follow-up pr? A regression test would be also good |
What version of |
3.6 also this is still missing a changelog entry |
@ocharles: if you are too busy right now, please let me know and I will add the changelog entry. If you any hints about a possible regression test, please share as well (I'm new to this issue). |
Please go ahead. I don't think I'll be able to provide any more work on this I'm afraid. |
I've added the changelog. I won't attempt adding a regression test at this point. Regarding the tickbox "The documentation has been updated, if necessary", I don't think any documentation is detailed enough about coverage when testing to require a fix. |
Currently, if you have multiple packages in a project and try and run
the test suite of a single package with --enable-coverage, hpc will fail
to run. The problem is that all dependencies of the package are built
with
-fhpc
, but when we runhpc markup
, we are only passing the.mix
directory of the package being tested. Because we built alldependencies with
-fhpc
and we haven't excluded them from the report,we need to supply their
.mix
directories too.The above suggests one fix - compute the transitive closure of all
.mix
directories. However, there is another solution - change fromusing an exclude-list to using an include-list. This is the approach
used in this commit.
Explicitly enumerating all modules to include in the report is simpler
to code, but is also more likely to be what the user is interested in.
Generally, when one generates a coverage report from a test-suite, they
want to understand the coverage of the unit being tested. Having
coverage information from dependencies is usually not relevant. This is
also the behavior from old-style Cabal builds, where there wasn't even
a notion of a Cabal project.
Fixes #5433.
Please include the following checklist in your PR:
changelog.d
directory).Please also shortly describe how you tested your change. Bonus points for added tests!