improve exceptions rethrown by unwrapAnnotatedHUnitFailure

[chris-martin]

This replicates a bunch of display logic from displayException in annotated-exception that ideally would be exported by that package (cc @parsonsmatt).

Closes #201

[pbrisbin]

I do wonder if the pain we self-inflict by such pervasive use of annotated exceptions will ever end. I do really like them though. 😅

[pbrisbin]

Wait, can we back up a bit as to why we need this hook at all?

The reason we need to be careful in other scenarios is because we've done an AnnotatedException.handle + re-throw (which I think checkpointCallstack qualifies as). When we do that, we risk handling someone else's exception (like HandlerContents) and re-throw-ing it as AnnotatedException HandlerContents, which breaks things, such as Yesod looking for that exception type to render a 400 or 404.

I can't see how our hspec usages would ever run afoul of this with HUnitFailure. As long as we are using hspec assertions (shouldBe) and not AnnotatedException.throw HUnitFailure, provided we don't have the handle-rethrow anywhere, and provided we don't manually checkpoint callstacks around any assertions, there should never be an AnnotatedException HUnitFailure seen. I know those assumptions aren't guarantees, but they seem pretty stable to me in typical usage.

You originally got here because the throwString in getJsonBody was printing out the AnnotatedException instead of looking like an HUnitFailure -- and you thought that was due to excessive wrapping, right? But it wasn't. It was just throwing an annotated exception (by using our custom throwString) that was coming out as it was. Making that expectationFailure (or using Control.Exception.throw HUnitFailure, which is all that is) is the fix -- with or without a hook like this.

Am I misunderstanding something?

[chris-martin]

I originally got on this track because shouldBe failures should print like this:

ghci> :m Test.Hspec Freckle.App.Prelude Freckle.App.Exception

ghci> hspec $ it "" $ 'a' `shouldBe` 'b'

Ghci2[7:9] [✘]

Failures:

  <interactive>:7:21: 
  1) Ghci2[7:9]
       expected: 'b'
        but got: 'a'

  To rerun use: --match "/Ghci2[7:9]/"

Randomized with seed 603656257

Finished in 0.0020 seconds
1 example, 1 failure
*** Exception: ExitFailure 1

But when they're annotated, they print like this:

ghci> hspec $ it "" $ checkpointCallStack $ 'a' `shouldBe` 'b'

Ghci2[12:9] [✘]

Failures:

  <interactive>:12:9: 
  1) Ghci2[12:9]
       uncaught exception: AnnotatedException
       AnnotatedException {annotations = [Annotation @CallStack [("checkpointCallStack",SrcLoc {srcLocPackage = "interactive", srcLocModule = "Ghci2", srcLocFile = "<interactive>", srcLocStartLine = 12, srcLocStartCol = 17, srcLocEndLine = 12, srcLocEndCol = 36})]], exception = HUnitFailure (Just (SrcLoc {srcLocPackage = "interactive", srcLocModule = "Ghci2", srcLocFile = "<interactive>", srcLocStartLine = 12, srcLocStartCol = 43, srcLocEndLine = 12, srcLocEndCol = 53})) (ExpectedButGot Nothing "'b'" "'a'")}

  To rerun use: --match "/Ghci2[12:9]/"

Randomized with seed 1536058360

Finished in 0.0002 seconds
1 example, 1 failure
*** Exception: ExitFailure 1

there should never be an AnnotatedException HUnitFailure seen

I don't remember why, but there was, it's a real problem that I hit in curricula. I don't think it's unusual.

• Hspec expectations can be anything with an Example instance, e.g. some custom App monad, and I don't think it's unreasonable to include a checkpointCallStack somewhere in the evaluation the application monad.
• Hspec assertions may happen within callbacks to some application-level thing that includes a checkpointCallStack.
• It is good for HUnitFailures to get annotated with a call stack. In more complex tests, like when the assertion comes from some shared testing utility rather than from somewhere directly within the lexical scope of the spec that failed, a call stack is super helpful in figuring out what happened. (You can argue that we just shouldn't write tests this way, but we do.)

The only way to get a stack is to checkpointCallStack, and the only way to get a pretty diff printing is to convert AnnotatedException back to HUnitFailure right before it's handed off to the framework for formatting, hence this hook that combines the annotations with the HUnit failure to produce all the information we want in the format we want.

[parsonsmatt]

Note that the new version of annotated-exception has a much improved Show instance - that alone may help solve some of the pain here

I'd be happy to accept a PR exposing anything you needed to copy out

[pbrisbin]

Assertions happening within your application monad (AppExample) or within an application callback that may be checkpointed is what I was missing.