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

Coverage doesn't work if the test suite doesn't depend on the library #3682

Open
mightybyte opened this issue Aug 8, 2016 · 15 comments
Open

Comments

@mightybyte
Copy link
Collaborator

mightybyte commented Aug 8, 2016

To reproduce this, first clone this repo:

git clone https://github.com/mightybyte/map-syntax.git

Then the following inside the map-syntax repo generates nice HPC test coverage reports:

git checkout hpc-works
cabal install --enable-tests
cabal test
hpc markup --destdir=hpc testsuite

This appears to generate HPC test coverage reports, but they are actually empty:

git checkout hpc-broken
cabal install --enable-tests --enable-coverage
cabal test

In this case, cabal test is supposed to generate the coverage reports for you, so the last hpc line from the other example should not be needed. The only difference between the two branches is a ghc-options: -fhpc line in the cabal file.

The biggest problem here is that I actually can't use the hpc-works approach because cabal sdist says that I should not use the -fhpc option inside my .cabal file and that hackage will reject my package.

One other thing that would be good to keep in mind here is that hpc allows you to tweak things with a variety of command line flags like --exclude, --include, etc. Whatever solution cabal gives us should still make it possible to specify your own flags. Maybe this is as simple as putting the coverage data in a predictable place so you can run hpc manually. But it is still probably something that we need to think about if we're going to continue outlawing -fhpc in .cabal files.

This is with cabal-install-1.25.0.0. I think built from commit a92bfc8

@23Skidoo
Copy link
Member

23Skidoo commented Aug 8, 2016

/cc @ttuegel

@ezyang
Copy link
Contributor

ezyang commented Aug 8, 2016

The -fhpc check was added by @dcoutts in 39c0386

The broken HPC appears to be broken in 1.24 as well.

@ezyang
Copy link
Contributor

ezyang commented Aug 8, 2016

Oddly enough, -fhpc does seem to be passed to GHC for all invocations. So I am not sure why the workaround works:

ezyang@sabre:~/Dev/labs/map-syntax$ /opt/cabal/1.24/bin/cabal test -v
Component build order: test suite 'testsuite'
creating dist/build
creating dist/build/autogen
/usr/bin/ghc-pkg init dist/package.conf.inplace
Preprocessing test suite 'testsuite' for map-syntax-0.2.0.1...
Building test suite testsuite...
creating dist/build/testsuite
creating dist/build/testsuite/testsuite-tmp
/usr/bin/ghc --make -no-link -fbuilding-cabal-package -O -fhpc -hpcdir dist/hpc/vanilla/mix/testsuite -j1 -static -outputdir dist/build/testsuite/testsuite-tmp -odir dist/build/testsuite/testsuite-tmp -hidir dist/build/testsuite/testsuite-tmp -stubdir dist/build/testsuite/testsuite-tmp -i -idist/build/testsuite/testsuite-tmp -isrc -itest -idist/build/autogen -Idist/build/autogen -Idist/build/testsuite/testsuite-tmp -optP-include -optPdist/build/autogen/cabal_macros.h -hide-all-packages -package-db dist/package.conf.inplace -package-id HUnit-1.3.0.0-904109a490c85ff643c14c4cfbe058fb -package-id QuickCheck-2.8.1-2cs6jeUUZX9ZLDN6ycKJHM -package-id base-4.8.2.0-0d6d1084fbc041e1cded9228e80e264d -package-id containers-0.5.6.2-59326c33e30ec8f6afd574cbac625bbb -package-id deepseq-1.4.1.1-614b63b36dd6e29d2b35afff57c25311 -package-id hspec-2.2.3-26H6GBDRpLQCyZpmHWBtcW -package-id mtl-2.2.1-378RFoja0lduCvzTAM9NPi -package-id transformers-0.4.2.0-81450cd8f86b36eaa8fa0cbaf6efc3a3 -XHaskell2010 test/TestSuite.hs -Wall -fwarn-tabs
[1 of 4] Compiling Data.Map.Syntax  ( src/Data/Map/Syntax.hs, dist/build/testsuite/testsuite-tmp/Data/Map/Syntax.o )
[2 of 4] Compiling Data.Map.Syntax.Util ( test/Data/Map/Syntax/Util.hs, dist/build/testsuite/testsuite-tmp/Data/Map/Syntax/Util.o )
[3 of 4] Compiling Data.Map.Syntax.Tests ( test/Data/Map/Syntax/Tests.hs, dist/build/testsuite/testsuite-tmp/Data/Map/Syntax/Tests.o )

test/Data/Map/Syntax/Tests.hs:10:1: Warning:
    The import of ‘Data.Monoid’ is redundant
      except perhaps to import instances from ‘Data.Monoid’
    To import instances alone, use: import Data.Monoid()
[4 of 4] Compiling Main             ( test/TestSuite.hs, dist/build/testsuite/testsuite-tmp/Main.o )
Linking...
/usr/bin/ghc --make -fbuilding-cabal-package -O -fhpc -hpcdir dist/hpc/vanilla/mix/testsuite -static -outputdir dist/build/testsuite/testsuite-tmp -odir dist/build/testsuite/testsuite-tmp -hidir dist/build/testsuite/testsuite-tmp -stubdir dist/build/testsuite/testsuite-tmp -i -idist/build/testsuite/testsuite-tmp -isrc -itest -idist/build/autogen -Idist/build/autogen -Idist/build/testsuite/testsuite-tmp -optP-include -optPdist/build/autogen/cabal_macros.h -hide-all-packages -package-db dist/package.conf.inplace -package-id HUnit-1.3.0.0-904109a490c85ff643c14c4cfbe058fb -package-id QuickCheck-2.8.1-2cs6jeUUZX9ZLDN6ycKJHM -package-id base-4.8.2.0-0d6d1084fbc041e1cded9228e80e264d -package-id containers-0.5.6.2-59326c33e30ec8f6afd574cbac625bbb -package-id deepseq-1.4.1.1-614b63b36dd6e29d2b35afff57c25311 -package-id hspec-2.2.3-26H6GBDRpLQCyZpmHWBtcW -package-id mtl-2.2.1-378RFoja0lduCvzTAM9NPi -package-id transformers-0.4.2.0-81450cd8f86b36eaa8fa0cbaf6efc3a3 -XHaskell2010 test/TestSuite.hs -o dist/build/testsuite/testsuite -Wall -fwarn-tabs
Linking dist/build/testsuite/testsuite ...
Running 1 test suites...
Test suite testsuite: RUNNING...
dist/build/testsuite/testsuite
Test suite testsuite: PASS
Test suite logged to: dist/test/map-syntax-0.2.0.1-testsuite.log
/srv/prod/bin/hpc markup dist/hpc/vanilla/tix/testsuite/testsuite.tix '--destdir=dist/hpc/vanilla/html/testsuite' '--hpcdir=dist/hpc/vanilla/mix/testsuite' '--hpcdir=dist/hpc/vanilla/mix/map-syntax-0.2.0.1' '--exclude=Data.Map.Syntax' '--exclude=Data.Map.Syntax.Util' '--exclude=Data.Map.Syntax.Tests' '--exclude=Main'
hpc: can not find 5Dy4335KtNp12MnuUhJwf2/Test.Hspec.Timer in ./.hpc, ./dist/hpc/vanilla/mix/map-syntax-0.2.0.1, ./dist/hpc/vanilla/mix/testsuite
CallStack (from ImplicitParams):
  error, called at libraries/hpc/Trace/Hpc/Mix.hs:119:15 in hpc-0.6.0.2:Trace.Hpc.Mix

EDIT: The difference would be that we're not passing hpcdir. Hmm...

@ezyang
Copy link
Contributor

ezyang commented Aug 8, 2016

Got it.

The immediate cause for failure is the tix file is including ticks for Test.Hspec.Timer and other modules, even though it shouldn't (the code assumes that only tix for the current component under test get generated.)

The actual cause for failure is hspec and co are being compiled with coverage enabled. Why? Because you did cabal install --enable-coverage. So, the workaround is to say cabal install --dependencies-only --enable-tests, and then cabal configure --enable-tests --enable-coverage and then it should work. Or use new-build which shouldn't commit this mistake. I think it's just wrong for cabal install to pass the flag on? Will have to think about it more.

There's probably something very wonky going on with the implicit assumptions Setup makes which should be fixed but I haven't thought about it carefully enough.

@mightybyte
Copy link
Collaborator Author

Unfortunately that does not solve the problem. I did exactly the steps you said (including rm -fr ~/.ghc at the beginning) and got the same empty coverage reports. I also tried it with new-build and that doesn't seem to work at all. I suspected that might be the case because I originally noticed this issue yesterday when I was working on the Snap 1.0 release. In that case we were using new-build. Here are the two build logs:

https://travis-ci.org/snapframework/snap-core/jobs/150434976
https://travis-ci.org/snapframework/snap-core/jobs/150470212

And here is the diff between those commits

snapframework/snap-core@ba5e8fb...1d65f0e

@ezyang
Copy link
Contributor

ezyang commented Aug 8, 2016

Oh ho ho, what a confusing ticket. It turns out that the error I pasted above was NOT the error you were trying to report (rereading your initial report, it looks like you did NOT get any error or non-zero exit code, just nothing in the coverage. This is why it's good to always post logs when you have them.)

I looked at my now succeeding hpc output and it is indeed empty. And I also know why this is the case:

ezyang@sabre:~/Dev/labs/map-syntax$ HOME=/home/ezyang/Dev/labs/map-syntax cabal test -v
Using internal setup method with build-type Simple and args:
["build","--verbose=2","--builddir=dist","--jobs=2","testsuite"]
Component build order: test suite 'testsuite'
/usr/bin/ghc-pkg init dist/package.conf.inplace
creating dist/build/testsuite
creating dist/build/testsuite/autogen
Preprocessing test suite 'testsuite' for map-syntax-0.2.0.1...
Building test suite testsuite...
creating dist/build/testsuite
creating dist/build/testsuite/testsuite-tmp
/usr/bin/ghc --make -no-link -fbuilding-cabal-package -O -fhpc -hpcdir dist/hpc/vanilla/mix/testsuite -j2 -static -outputdir dist/build/testsuite/testsuite-tmp -odir dist/build/testsuite/testsuite-tmp -hidir dist/build/testsuite/testsuite-tmp -stubdir dist/build/testsuite/testsuite-tmp -i -idist/build/testsuite/testsuite-tmp -isrc -itest -idist/build/testsuite/autogen -idist/build/global-autogen -Idist/build/testsuite/autogen -Idist/build/global-autogen -Idist/build/testsuite/testsuite-tmp -optP-include -optPdist/build/testsuite/autogen/cabal_macros.h -hide-all-packages -package-db dist/package.conf.inplace -package-id HUnit-1.3.0.0-904109a490c85ff643c14c4cfbe058fb -package-id QuickCheck-2.8.1-3b4e59fb7b4229f7e8519c5347611ffa -package-id base-4.8.2.0-0d6d1084fbc041e1cded9228e80e264d -package-id containers-0.5.6.2-59326c33e30ec8f6afd574cbac625bbb -package-id deepseq-1.4.1.1-614b63b36dd6e29d2b35afff57c25311 -package-id hspec-2.2.3-IP0Fj6TUGZrCHjKQR73tKM -package-id mtl-2.2.1-3af90341e75ee52dfc4e3143b4e5d219 -package-id transformers-0.4.2.0-81450cd8f86b36eaa8fa0cbaf6efc3a3 -XHaskell2010 test/TestSuite.hs -Wall -fwarn-tabs
Linking...
/usr/bin/ghc --make -fbuilding-cabal-package -O -fhpc -hpcdir dist/hpc/vanilla/mix/testsuite -static -outputdir dist/build/testsuite/testsuite-tmp -odir dist/build/testsuite/testsuite-tmp -hidir dist/build/testsuite/testsuite-tmp -stubdir dist/build/testsuite/testsuite-tmp -i -idist/build/testsuite/testsuite-tmp -isrc -itest -idist/build/testsuite/autogen -idist/build/global-autogen -Idist/build/testsuite/autogen -Idist/build/global-autogen -Idist/build/testsuite/testsuite-tmp -optP-include -optPdist/build/testsuite/autogen/cabal_macros.h -hide-all-packages -package-db dist/package.conf.inplace -package-id HUnit-1.3.0.0-904109a490c85ff643c14c4cfbe058fb -package-id QuickCheck-2.8.1-3b4e59fb7b4229f7e8519c5347611ffa -package-id base-4.8.2.0-0d6d1084fbc041e1cded9228e80e264d -package-id containers-0.5.6.2-59326c33e30ec8f6afd574cbac625bbb -package-id deepseq-1.4.1.1-614b63b36dd6e29d2b35afff57c25311 -package-id hspec-2.2.3-IP0Fj6TUGZrCHjKQR73tKM -package-id mtl-2.2.1-3af90341e75ee52dfc4e3143b4e5d219 -package-id transformers-0.4.2.0-81450cd8f86b36eaa8fa0cbaf6efc3a3 -XHaskell2010 test/TestSuite.hs -o dist/build/testsuite/testsuite -Wall -fwarn-tabs
Using internal setup method with build-type Simple and args:
["test","--verbose=2","--builddir=dist","--log=$pkgid-$test-suite.log","--machine-log=$pkgid.log","--show-details=failures","testsuite"]
Running 1 test suites...
Test suite testsuite: RUNNING...
dist/build/testsuite/testsuite
Test suite testsuite: PASS
Test suite logged to: dist/test/map-syntax-0.2.0.1-testsuite.log
/usr/bin/hpc markup dist/hpc/vanilla/tix/testsuite/testsuite.tix '--destdir=dist/hpc/vanilla/html/testsuite' '--hpcdir=dist/hpc/vanilla/mix/testsuite' '--hpcdir=dist/hpc/vanilla/mix/map-syntax-0.2.0.1' '--exclude=Data.Map.Syntax' '--exclude=Data.Map.Syntax.Util' '--exclude=Data.Map.Syntax.Tests' '--exclude=Main'
Writing: hpc_index.html
Writing: hpc_index_fun.html
Writing: hpc_index_alt.html
Writing: hpc_index_exp.html
Test coverage report written to dist/hpc/vanilla/html/testsuite/hpc_index.html
1 of 1 test suites (1 of 1 test cases) passed.
/usr/bin/hpc sum --union dist/hpc/vanilla/tix/testsuite/testsuite.tix '--output=dist/hpc/vanilla/tix/map-syntax-0.2.0.1/map-syntax-0.2.0.1.tix' '--exclude=Data.Map.Syntax' '--exclude=Data.Map.Syntax.Util' '--exclude=Data.Map.Syntax.Tests' '--exclude=Main'
/usr/bin/hpc markup dist/hpc/vanilla/tix/map-syntax-0.2.0.1/map-syntax-0.2.0.1.tix '--destdir=dist/hpc/vanilla/html/map-syntax-0.2.0.1' '--hpcdir=dist/hpc/vanilla/mix/map-syntax-0.2.0.1' '--hpcdir=dist/hpc/vanilla/mix/testsuite' '--exclude=Data.Map.Syntax' '--exclude=Data.Map.Syntax.Util' '--exclude=Data.Map.Syntax.Tests' '--exclude=Main'
Writing: hpc_index.html
Writing: hpc_index_fun.html
Writing: hpc_index_alt.html
Writing: hpc_index_exp.html
Package coverage report written to
dist/hpc/vanilla/html/map-syntax-0.2.0.1/hpc_index.html

Look at the invocation of hpc: we are passing --exclude which excludes all of the library files that you might be interested in. Why is it doing that? I think the idea is that , in general, you don't care about the coverage of code in your test suite; it should all be trivially covered.

So why is it excluding your library files? Well, look at your Cabal file, you didn't declare a dependency on the library; you're using the hack (fwiw, cabal-install uses this hack too, so not such a hack) where you put src in hs-source-dirs so that you recompile the library when you run the test suite. So hpc thinks that the library code is part of the test suite, and excludes it.

Workarounds: call hpc yourself without the exclude flags. Or make the test suite depend on the library and remove the src from hs-source-dirs.

My recommendation on the Cabal side is we should just remove the --exclude logic. It feels like overengineering for not much benefit. @ttuegel who wrote the code should be able to say more.

@ttuegel
Copy link
Member

ttuegel commented Aug 8, 2016

My recommendation on the Cabal side is we should just remove the --exclude logic. It feels like overengineering for not much benefit. @ttuegel who wrote the code should be able to say more.

This was requested by someone at some point. (I don't recall exactly; but I'm exceptionally lazy and I never would have done it out-of-the-blue 😉) I agree that it seems overengineered. Also, doesn't cabal.config have a field for HPC options? That would seem to address the original use-case.

@ezyang
Copy link
Contributor

ezyang commented Aug 8, 2016

@ttuegel are you planning on fixing this?

@mightybyte
Copy link
Collaborator Author

Ok, I think I understand what is going on now. I have a couple more comments. First, when I make the test suite depend on the library it works almost as expected. However, there is one little frustrating detail...a random hash gets prepended to the module names:

screen shot 2016-08-08 at 8 11 13 pm

This is fine for my own consumption, but it's a little annoying for this one which I am auto-generating from my Snap build and publishing on the Snap website. This is fairly minor and I might not even mention except that we already have to consider the issue that the approach of having the test suite depend on the library does not work in all situations. As you mention, you use the hs-source-dirs hack in Cabal. But it's more than just a hack. In Snap we have internal modules that we do not want to export but need to be testable by the test suite. In that situation the hs-source-dirs approach is the only thing you can do.

I also spent a lot of time yesterday fighting with this stuff for the snap release. That case was a little different because we are using new-build. In this issue where I'm not using new-build it was already pretty hard to discover the right hpc incantation. In the new-build situation it's even worse because cabal doesn't give you any examples of how to invoke hpc.

I definitely appreciate the spirit of what cabal is trying to do about automatically making sure the test suite files don't show in the coverage reports. Here's the solution I ended up with for the map-syntax package that I was using in this ticket: https://github.com/mightybyte/map-syntax/blob/master/runCoverage.sh It would certainly be nice to not have to manually construct the list of excludes, but it's no good if it doesn't work in all situations.

My main concern with all this is making information more discoverable so it's easier for me (and other people in the future) to get something analogous to that runCoverage script into its final form. The biggest problem there was figuring out what the hpc command line should look like. One way cabal-install might make this easy might be to provide a command "cabal hpc-command" (similar in spirit to stack's path command) that would simply return the right command (with no --exclude or --include options). Having it holistically built into something like cabal test makes me much more likely to open an issue here when it doesn't handle the particular special case I need at the moment.

@ezyang
Copy link
Contributor

ezyang commented Aug 9, 2016

However, there is one little frustrating detail...a random hash gets prepended to the module names:

Is this related to https://ghc.haskell.org/trac/ghc/ticket/10952 ? I guess this is a different aesthetic issue. The source of the issue might be hpc or GHC or Cabal, hard to say. CC @mgsloan who has also looked at this.

In Snap we have internal modules that we do not want to export but need to be testable by the test suite

This is exactly what convenience/internal libraries are for, which are coming next Cabal release! I don't know what your Cabal support window looks like but eventually you will be able to put your internal code in an internal, non-published library component, and then have your public library as well as your test suites depend on it. So yes it's a hack, but a justifiable one today, and perhaps less justifiable three years from now. (If you have a three GHC release window...)

I also spent a lot of time yesterday fighting with this stuff for the snap release. That case was a little different because we are using new-build. In this issue where I'm not using new-build it was already pretty hard to discover the right hpc incantation. In the new-build situation it's even worse because cabal doesn't give you any examples of how to invoke hpc.

Well, it is not surprising you had difficulty, since it simply not supported at all right now (just like how new-run, new-test, etc are not supported). I'll file another ticket about getting hpc to work with new-build. There are a lot of bugs to fix and features to implement (just look at the nix-local-build tag on the issues tracker); we could definitely use more hands. (Submit a PR and you'll get commit bits #3567 ;)

My main concern with all this is making information more discoverable so it's easier for me (and other people in the future) to get something analogous to that runCoverage script into its final form. The biggest problem there was figuring out what the hpc command line should look like.

Having never used hpc before, I have no idea what this should look like. Suggestions welcome :)

@ezyang ezyang changed the title The --enable-coverage flag doesn't work Coverage doesn't work if the test suite doesn't depend on the library Aug 9, 2016
@mightybyte
Copy link
Collaborator Author

This is exactly what convenience/internal libraries are for, which are coming next Cabal release!

Awesome, that sounds great! I'll have to think about the compatibility thing, but it's nice to know this is in the works.

Having never used hpc before, I have no idea what this should look like.

Well, you know what hpc command is generated by the existing cabal test. I'd say just start with that (possibly minus any --include or --exclude options). Then the user can see the command and is much more empowered to tweak it as they see fit. That's how I figured out what to put in my runCoverage.sh script. Before opening this ticket even though I did look at the -v output, but failed to notice the hpc command lurking in the wall of text.

@lapplislazuli
Copy link

Hi there,

It seems I have the very same problem just with a little tweak:
I'd like to get a coverage of an executable.

So i have something alike:

[...]
executable Hasculator
  main-is:            Program.hs 
  other-modules:      ModuleA,
                      ModuleB
  hs-source-dirs:      Src 
[...]

Test-Suite HUnitTestSuite
  type:                exitcode-stdio-1.0
  main-is:             UnitTests.hs
  hs-source-dirs:      Test
                       Src
  other-modules:       ModuleA,
                       ModuleB, 
                       TestHelperModule

[...]

running cabal new-test . produces empty coverage reports (as above) - however I cannot depend on my executable?

The full project can be found here

Is there any way I can help with this?

@Mikolaj
Copy link
Member

Mikolaj commented Jul 24, 2021

This is related to #5213, but I doubt the PR that fixes #5213 will fix that (if it's still buggy --- I haven't tested).

@lapplislazuli: I guess you can place all the modules from Hasculator into an internal library and only have a tiny wrapper as a test.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 24, 2021

@lapplislazuli: Actually, you'd probably get "cabal: Error: test coverage is only supported for packages with a library" so that's not a valid workaround. I tried with newest development cabal and GHC 9.0.1.

However, you may also turn on HPC, build exe, run it and then manually create HPC report and HPC makrup. At least that's what I was doing some years ago, though now my scripts are bit-rotten.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 24, 2021

BTW, in #7250 the --exclude have been changed to --include. Not sure if this fixes the original problems that mention --exclude.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants