Pretty printing for `HUnitFailure`

[andreasabel]

There is a Show instance for HUnitFailure which gives the Haskell representation of this exception. So far so good.

I am looking for a function that pretty-prints the exception, for presentation to the user. It should look nicer than what Show gives me:

      Exception: HUnitFailure (Just (SrcLoc {srcLocPackage = "main", srcLocModule = "Main", srcLocFile = 
"tests/PackageTestMain.hs", srcLocStartLine = 127, srcLocStartCol = 3, srcLocEndLine = 127, srcLocEndCol = 
17})) (Reason "Expected success but got: \"correct-package-0.1.0.0/correct-package.cabal:11:34: \\nunexpected 
Unknown cabal spec version specified: 1.10123456\\nexpecting \\\".\\\", \\\"-\\\", white space, \\\"&&\\\" or \\\"||
\\\"\\n\"")

[VictorCMiraldo]

This affects me too. It's particularly problematic when the Reason contains a lot of information.

FWIW I'm currently hacking my way around that with some unsafePerformIO and using something like:

-- Catches a HUnit test failure, if the test fails.
assertionToMaybe :: HU.Assertion -> IO (Maybe HU.HUnitFailure)
assertionToMaybe = flip E.catches [E.Handler $ return . Just] . (>> return Nothing)

testCounterexample :: String -> HU.Assertion -> HU.Assertion
testCounterexample msg = maybe testSuccess (E.throw . adjustMsg) <=< assertionToMaybe
    where
      joinMsg :: String -> String
      joinMsg rest = msg ++ "; " ++ rest

      adjustMsg :: HU.HUnitFailure -> HU.HUnitFailure
      adjustMsg (HU.HUnitFailure loc (HU.Reason txt)) =
        unsafePerformIO $ do
          putStrLn ('\n' : joinMsg txt)
          return $ HU.HUnitFailure loc (HU.Reason "")
      adjustMsg (HU.HUnitFailure loc (HU.ExpectedButGot pref x y)) =
        HU.HUnitFailure loc (HU.ExpectedButGot (maybe (Just msg) (Just . joinMsg) pref) x y)

It's not perfect, but at least it preserves the newlines of the Reason message, which is way better than a giant raw-string.

[adamgundry]

Yes, it would be nice if HUnitFailure had a displayException implementation, and exceptionResult used displayException instead of show.

[UnkindPartition]

Hey, sorry for the silence. See https://github.com/UnkindPartition/UnkindPartition/blob/main/README.md.

I trust highly all 3 of you, so I'm going to add you as contributors to the repo. Please take care of it while I'm away. And feel free to ask @ocharles or hackage admins to add you as maintainers there.

[UnkindPartition]

Also added @Bodigrim, whom I also trust and who's been helping a lot with tasty.

[VictorCMiraldo]

Thanks @UnkindPartition; I'll set up a PR on this next week :)

[andreasabel]

Hey, sorry for the silence. See https://github.com/UnkindPartition/UnkindPartition/blob/main/README.md.

Roman, I pray that you get through this terrible situation unharmed.

[VictorCMiraldo]

Hey, sorry for the silence. See https://github.com/UnkindPartition/UnkindPartition/blob/main/README.md.

OMG :fearful: ! I just read this. I make Andreas words mine, my thoughts are with you Roman.

[VictorCMiraldo]

So I started working on this today but I couldn't reproduce the problem. Eventually I discovered that @andreasabel and I did something wrong. What we did is some variation on:

import qualified Test.HUnit as Raw
import qualified Test.Tasty.HUnit as Tasty

t1 = Tasty.testCase "Some Test" $ Raw.assertFailure "Omg"

The issue comes from the fact that Raw.HUnitAssertion is a different type altogether from Tasty.HUnitAssertion. Because Tasty.Assertion == Raw.Assertion == IO (), and failures are communicated through exceptions, we're bypassing the try on Tasty.HUnit.run and flagging the test as failed through a random exception, since that method is trying on Tasty.HUnitAssertion.

The solution is to not depend on HUnit, depend exclusively on tasty-hunit and use the functions from there. Your test failures will be printed properly then.

[andreasabel]

@VictorCMiraldo : Thanks for the excellent analysis and the cookbook! Indeed, this fixes my issue, see:

• https://github.com/haskell/hackage-server/pull/1065

[VictorCMiraldo]

I'll close this then, it wasn't really an issue to begin with :)

I'll eventually send a PR to update the documentation of tasty-hunit mentioning the potential pitfall.

[sol]

A better solution would be to depend on upstream HUnit so that tasty-hunit is compatible with other packages that build on top of HUnit. This would also offer a solution to #262, without the need to reimplementing hspec-expectations for Tasty specifically.

Alternatively, it may make sense to rename tasty-hunit to tasty-unit to avoid confusion.

For historic context, I'm not sure for what reason @UnkindPartition decided to ship his own copy of HUnit, but at that time HUnit was not on GitHub and possibly he was concerned about its maintenance.

That's not the case anymore, and hasn't been for the last 7 years. @bergmark and myself are maintaining HUnit (and a lot more people have push access to the repo).

[VictorCMiraldo]

That's a reasonable suggestion @sol, I could not find any info on the commit that dropped the dependency on HUnit so I do lack context to really give an opinion.

Let's give some time for @UnkindPartition to have a chance to chime in. I'm happy to review/shepherd a pr bringing HUnit back as a dependency of tasty if we're all happy with it.

[Bodigrim]

I'd rather refrain from architectural changes without @UnkindPartition's explicit consent. It was a conscious choice, and I believe the intention was to evolve API separately (see #262 and comments about deprecated functions).

Thanks to modular architecture, one can use tasty-hunit-compat instead of tasty-hunit. No need to break an existing package.

Also remember that PVP essentially discourages re-exports from other packages, because otherwise it's difficult to maintain a stable interface.

[UnkindPartition]

There were two reasons afair (my bad for not mentioning them in the commit message):

1. Like @sol says, there were maintenance issues with hunit at the time
2. To reduce the number of dependencies.

(1) is no longer relevant, white (2) still is. However, I realize that this decision has caused some confusion and pain over the years, so if the consensus here (esp. among the new maintainers) is to switch back, so be it.

Yet another option would be to make use of the extensible exceptions system such that the packages wouldn't need to depend on each other. E.g. insert Proxy "TestFailure" as a "superclass" of both versions of HUnitFailure and change tasty-hunit to catch that instead. This won't resolve the confusion (like the one in this issue) but will solve the interoperability with hspec-expectations (as in #262) as well as other potential packages that may have a need for their own test failure exceptions.

[UnkindPartition]

A good example of the latter would be quickcheck btw. There are people who use hunit and/or hspec operators inside monadic QC properties, not realizing that it's not quite what they want. If quickcheck also started to treat that Proxy exception in a special way, that usage pattern could start working as expected without introducing extra dependencies for quickcheck.

[adamgundry]

First of all, @UnkindPartition, like the others I want to wish you all the best in what must be an incredibly difficult time.

On more technical matters I'm reopening this issue because while https://github.com/UnkindPartition/tasty/issues/327#issuecomment-1108667339 is correct, I still think tasty could be improved slightly here. I believe the crux of the issue is that in code like https://github.com/UnkindPartition/tasty/blob/efe0ad287032b17bfd78c646db62e833518eeb19/hunit/Test/Tasty/HUnit/Steps.hs#L29-L30 and https://github.com/UnkindPartition/tasty/blob/efe0ad287032b17bfd78c646db62e833518eeb19/core/Test/Tasty/Core.hs#L116 there are calls to show on exceptions (modulo the special handling for (tasty's version of) HUnitFailure). Using displayException instead would give the more user-friendly output regardless of whether the specific exception being thrown was a HUnitFailure from tasty, from HUnit, or a completely different exception.

[re-xyr]

Since @UnkindPartition has agreed that it is possible to switch back to HUnit, what is the opinion of other maintainers? @andreasabel @Bodigrim @ocharles @VictorCMiraldo @adamgundry

If we reach a consensus, I'm happy to implement this.

[andreasabel]

I do not have an opinion on this, so +1 by default.

[ocharles]

I think the dependency footprint of HUnit is minimal, and the cost of having a single exception type/code to maintain outweighs this cost. I'm +1 on this too, especially as the HSpec community are now maintaining it.

[Bodigrim]

I have concerns about re-exports from other packages, but AFK, will be back next week.

[Bodigrim]

My concern is that a specific version of a package should provide a stable API, whatever build plan. This means that for re-exported functions each version of the proposed tasty-hunit must bound HUnit >= X.Y && < X.(Y+1) and for re-exported datatypes it should be HUnit >= X.Y.Z && < X.Y.(Z+1) (because new instances can leak). Maintaining this is cumbersome and I'm not convinced there are resources to do it in the long term.

Yes, tasty-quickcheck and tasty-smallcheck are already guilty w. r. t. re-exports, but tasty-hunit currently is not and I'm reluctant to worsen the situation.

[re-xyr]

and for re-exported datatypes it should be HUnit >= X.Y.Z && < X.Y.(Z+1) (because new instances can leak)

I thought it's only the case when the instances are orphans, and in that case it would be a major bump anyway?

---

If we depend on HUnit, then we have an extra bound to maintain, while if we borrow the code from HUnit directly, we have an extra copy of code to maintain. Therefore I think it could be an improvement in terms of maintenance if we depend on HUnit. Or I failed to consider some other factors?

Moreover, if we were to run out of maintenance resources, we get a restrictive upper bound, which I don't think is worse (nor necessarily better) than an outdated API, which would be the case if we don't depend on HUnit.

[Bodigrim]

I thought it's only the case when the instances are orphans, and in that case it would be a major bump anyway?

No. Imagine re-exporting HUnitFailure from HUnit in tasty-hunit-0.11.0.0 with build-depends: HUnit >= 1.6 && < 1.7. If HUnit-1.6.3.0 adds any instance for HUnitFailure, it will leak from tasty-hunit-0.11.0.0 as well. This means that tasty-hunit-0.11.0.0 fails to provide a stable interface and clients with build-depends: tasty-hunit == 0.11.0.0 have no control of it.

[Bodigrim]

Getting back to this discussion, I must admit that I phased out tasty-hunit from my projects (QuickCheck with ioProperty and once does the trick for me). Thus there is little point for me to interfere in decision making about packages, which I do not use. And definitely using vanilla HUnit means less work in the long term for me and fellow maintainers here.

It seems that everyone else were supportive of switching to vanilla HUnit. @re-xyr are you still up to preparing a PR?

[Bodigrim]

Closing, the future of tasty-hunit is discussed at #388.