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 test --enable-coverage for multi-package projects #7250

Merged
merged 2 commits into from
Apr 14, 2021

Conversation

ocharles
Copy link
Contributor

@ocharles ocharles commented Jan 16, 2021

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 #5433.


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog (add file to changelog.d directory).
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

@cartazio
Copy link
Contributor

This sounds like something we all will love!

@fgaz
Copy link
Member

fgaz commented Mar 19, 2021

Rebase to fix CI

@cartazio
Copy link
Contributor

@fgaz is that a statement of intent or a request for @ocharles or that anyone do that ?

@fgaz
Copy link
Member

fgaz commented Mar 19, 2021

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
Copy link
Contributor Author

@fgaz @emilypi ok, I have rebased this.

@emilypi
Copy link
Member

emilypi commented Apr 13, 2021

@ocharles looks like it failed due to some name shadowing. Do you mind fixing? I'm happy to merge once those changes are fixed.

@emilypi emilypi merged commit b92b63e into haskell:master Apr 14, 2021
@fgaz
Copy link
Member

fgaz commented Apr 14, 2021

This is missing a changelog entry. @ocharles could you add one in a follow-up pr? A regression test would be also good

@j-mie6
Copy link

j-mie6 commented Jun 29, 2021

What version of cabal will this make it into?

@fgaz
Copy link
Member

fgaz commented Jun 29, 2021

3.6

also this is still missing a changelog entry

@Mikolaj
Copy link
Member

Mikolaj commented Jun 29, 2021

@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).

@ocharles
Copy link
Contributor Author

Please go ahead. I don't think I'll be able to provide any more work on this I'm afraid.

Mikolaj added a commit to Mikolaj/cabal that referenced this pull request Jun 29, 2021
@Mikolaj
Copy link
Member

Mikolaj commented Jun 29, 2021

@ocharles: that's fine; thank you for your contributions and all the best!

@fgaz: Starting PR #7467. I hope I'm doing it right.

@Mikolaj
Copy link
Member

Mikolaj commented Jun 30, 2021

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.

Mikolaj added a commit to Mikolaj/cabal that referenced this pull request Jun 30, 2021
Mikolaj added a commit that referenced this pull request Jun 30, 2021
fendor pushed a commit to fendor/cabal that referenced this pull request Jul 12, 2021
fendor pushed a commit to fendor/cabal that referenced this pull request Jul 12, 2021
fendor pushed a commit to fendor/cabal that referenced this pull request Jul 12, 2021
fendor pushed a commit to fendor/cabal that referenced this pull request Jul 12, 2021
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.

cabal new-test --enable-coverage fails
6 participants