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 #5713 Don't assume unified coverage report redundant if only one *.tix file #5731

Merged
merged 1 commit into from
May 4, 2022

Conversation

mpilgrem
Copy link
Member

@mpilgrem mpilgrem commented May 2, 2022

Currently, Stack.Coverage.generateHpcUnifiedReport assumes that a unified coverage report will be redundant if there is only one *.tix file. However, that is not necessarily the case. For example, one package may test the library of another package that does not test its own library.

As an interim solution, this proposed pull request suggests that a unified report should always be produced if there is one or more *.tix files, even if it may be redundant in some circumstances.

A more complex solution would be to have a more complex test that determines whether a unified report would be truely redundant if there is only one *.tix file.

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests! Tested by building, and using, stack.

…nly one *.tix file

Currently, `Stack.Coverage.generateHpcUnifiedReport` assumes that a unified coverage report will be redundant if there is only one `*.tix` file. However, that is not necessarily the case. For example, one package may test the library of another package that does not test its own library.

As an interim solution, this proposed pull request suggests that a unified report should always be produced if there is one or more `*.tix` files, even if it may be redundant in some circumstances.

A more complex solution would be to have a more complex test that determines whether a unified report would be truely redundant if there is only one `*.tix` file.
@NorfairKing
Copy link
Contributor

I just checked and this does indeed solve my problem in #5713 . 🎉

Copy link
Contributor

@NorfairKing NorfairKing left a comment

Choose a reason for hiding this comment

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

This fixes a bug and simplifies code. Double-win.

@NorfairKing
Copy link
Contributor

@mpilgrem Do we need anything for this to be merged?
A test would be nice, but I'm not much worried about it.

@mpilgrem
Copy link
Member Author

mpilgrem commented May 3, 2022

As I have identified in other recent pull requests, I am new to being an outside collaborator, in general and on stack in particular, and I don't know the convention/etiquette as regards 'merging your own pull requests'. It seems to me akin to 'marking your own homework', when the change has effects that affect users. However, if nobody objects or comments after a reasonable period of exposure, I will merge the pull request.

@NorfairKing
Copy link
Contributor

I have commit rights, but I'm not sure it'd be proper for me to merge this either...
Let's wait to see if anyone objects.

@NorfairKing
Copy link
Contributor

I don't see any complaints, this seems very sensible, and I've tried it out, so I'll go ahead and merge.

@NorfairKing NorfairKing merged commit e9fc45b into commercialhaskell:master May 4, 2022
@mpilgrem mpilgrem deleted the fix5713 branch May 4, 2022 22:12
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.

2 participants