pretty-print HUnitFailure exceptions #63

sfindeisen · 2022-08-27T15:05:42Z

Implements pretty printing of HUnitFailure exceptions by removing the dependency on HUnit-approx.

Fixes #60

sfindeisen · 2022-08-31T08:00:01Z

test/common/TestCommonEqEpsilon.hs

+    -- The wildcard branch below prints:
+    -- Hello from exceptionHandler! Exception is: HUnitFailure (Just (SrcLoc {srcLocPackage = "horde-ad-0.1.0.0-inplace-testLibrary", srcLocModule = "TestCommonEqEpsilon", srcLocFile = "test/common/TestCommonEqEpsilon.hs", srcLocStartLine = 118, srcLocStartCol = 5, srcLocEndLine = 118, srcLocEndCol = 16})) (Reason "expected: 0.8991\n but got: 0.7424999999999999\n (maximum margin of error: 1.0e-6)") of type: HUnitFailure
+    -- BUT we cannot match e with HUnitFailure x y because it doesn't compile:
+    -- HUnitFailure x y -> putStrLn $ "We got HUnitFailure!"


The compile error is:

test/common/TestCommonEqEpsilon.hs:49:5: error: • Couldn't match expected type ‘e’ with actual type ‘HUnitFailure’ ‘e’ is a rigid type variable bound by a pattern with constructor: SomeException :: forall e. Exception e => e -> SomeException, in an equation for ‘exceptionHandler’ at test/common/TestCommonEqEpsilon.hs:44:19-33 • In the pattern: HUnitFailure x y In a case alternative: HUnitFailure x y -> putStrLn $ "We got HUnitFailure!" In a stmt of a 'do' block: case e of HUnitFailure x y -> putStrLn $ "We got HUnitFailure!" _ -> putStrLn $ "Hello from exceptionHandler! Exception is: " ++ (show e) ++ " of type: " ++ (show $ typeOf e) • Relevant bindings include e :: e (bound at test/common/TestCommonEqEpsilon.hs:44:33) | 49 | HUnitFailure x y -> putStrLn $ "We got HUnitFailure!" |

and I thought: this is perhaps due to a missing instance, i.e. instance Exception HUnitFailure. So I tried:

import Test.Tasty.HUnit.Orig ()

(where that instance is declared) but then I got:

test/common/TestCommonEqEpsilon.hs:15:1: error: Could not load module ‘Test.Tasty.HUnit.Orig’ it is a hidden module in the package ‘tasty-hunit-0.10.0.3’ it is a hidden module in the package ‘tasty-hunit-0.10.0.3’ Use -v (or `:set -v` in ghci) to see a list of the files searched for. | 15 | import Test.Tasty.HUnit.Orig () |

and if I just declare the instance: instance Exception HUnitFailure then I get:

test/common/TestCommonEqEpsilon.hs:19:10: error: Duplicate instance declarations: instance Exception HUnitFailure -- Defined at test/common/TestCommonEqEpsilon.hs:19:10 instance [safe] Exception HUnitFailure -- Defined in ‘tasty-hunit-0.10.0.3:Test.Tasty.HUnit.Orig’ | 19 | instance Exception HUnitFailure |

Is there a way to pattern match on SomeException (HUnitFailure) then?...

What is [safe]?

BTW I just discovered here:

-- | This is the code copied from the original hunit package (v. 1.2.5.2). -- with minor modifications

but this is probably irrelevant.

Is this now explained by there being two copies of HUnitFailure?

No.

It is a well-known issue: UnkindPartition/tasty#327 (comment)

EDIT: maybe. Let me see...

EDIT: yes. Let me implement a fix...

SomeException fails because, with this quantification in place, you need to choose a type e, which decides at what point of the exception hierarchy you want to be. You choose with https://downloads.haskell.org/ghc/latest/docs/libraries/base-4.17.0.0/Control-Exception.html#v:fromException (or rather, with the type that the context implies in the application of fromException). In response you get an exception if, indeed, at this point of the hierarchy there is any exception encapsulated inside that expression. I think, If you implied another type e, you could get a value of another type out (though internally it would be the same untyped value, but you'd get a different API for it).

it is a hidden module

Right, an internal hidden module.

What is [safe]?

No idea.

Is there a way to pattern match on SomeException (HUnitFailure) then?

I don't think so.

I just managed to fix the whole thing without touching any exceptions at all.

SomeException fails because, with this quantification in place, you need to choose a type e, which decides at what point of the exception hierarchy you want to be. You choose with https://downloads.haskell.org/ghc/latest/docs/libraries/base-4.17.0.0/Control-Exception.html#v:fromException (or rather, with the type that the context implies in the application of fromException). In response you get an exception if, indeed, at this point of the hierarchy there is any exception encapsulated inside that expression. I think, If you implied another type e, you could get a value of another type out (though internally it would be the same untyped value, but you'd get a different API for it).

What do you say? That you cannot pattern match on SomeException (except the wildcard _ pattern), but you have to explicitly call fromException instead? Why?

Why?

Because it's 21st century and we have OOP or at least hierarchies or at least subtyping or at least the type tricks above that emulate subtyping. But seriously, is the way hierarchy is expressed surprising or the purpose unclear or what else?

Perhaps the crucial bit is extensible hierarchies. You have a virtual datatype of all possible exceptions you can get in your application and any new dependency you link against can add a few cases to that "datatype". That can't be done with normal datatypes. OCaml extensible variant types might suffice, but Haskell's anonymous sums are probably not there yet.

sfindeisen · 2022-08-31T09:39:55Z

test/common/TestCommonEqEpsilon.hs

+-- because we do not want to depend on Test.HUnit.Approx . Instead, we're using an outdated copy of HUnit hardcoded into
+-- Tasty (Test.Tasty.HUnit).
+--
+-- Copyright (c) 2014, Richard Eisenberg


Hi @goldfirere , I have stolen some of your code. Hope you're OK with that?... This is to get rid of dependency on HUnit-approx (Tasty contains its own copy of HUnit and if we have both, it leads to problems).

Mikolaj

Works for me. Please feel free to merge now in whichever of the 3 ways.

It's a pity we have to copy-paste code, but that's the price for the design decision in Tasty that makes them safe from PVP violations from re-exporting Hunit modules, IIUC (UnkindPartition/tasty#327).

Mikolaj · 2022-08-31T17:45:03Z

Quick! Before @goldfirere tells you this doesn't make any sense.

Mikolaj · 2022-09-02T20:50:17Z

Please feel free to merge now in whichever of the 3 ways.

I've pushed to master, so the 'Merge PR' method now requires a local rebase.

sfindeisen · 2022-09-03T07:56:33Z

I've pushed to master, so the 'Merge PR' method now requires a local rebase.

Rebase does not exactly work: https://stackoverflow.com/questions/19016698/git-branch-diverged-after-rebase , ok to merge master in here instead?

Mikolaj · 2022-09-03T08:04:52Z

Rebase does not exactly work

This SO thread says that it works fine with push -f. And since you are not collaborating with anyone on this branch, push -f should be fine, shouldn't it? Or am I missing something? I've been rebasing for decades. Weren't you?

ok to merge master in here instead?

I'd prefer not to, because it complicates git history and it becomes harder to read and harder to bisect. Unless you want to squash afterwards, but if so, you don't need to merge at all.

… the latter This is because we do not want to depend on Test.HUnit.Approx . Instead, we're using an outdated copy of HUnit hardcoded into Tasty (Test.Tasty.HUnit). If we mix the two, we get two copies of HUnitFailure exception which leads to trouble.

sfindeisen · 2022-09-03T09:22:52Z

I can't see why is push -f better than a merge commit, but here you have it.

Mikolaj · 2022-09-03T09:33:18Z

I can't see why is push -f better than a merge commit, but here you have it.

Thank you.

It's a matter of taste. Some people prefer merges to rebases. With many long-lived concurrent features branches, rebases become a nightmare, but with merges the history becomes a nightmare. It's a bit similar to "should programs be optimized for writing or for reading".

Do we agree, however, that the effect of push -f on a git history is different than of a merge commit and that, at scale, bisecting, reverting, etc., is harder with merges than with linear history?

sfindeisen · 2022-09-03T09:53:22Z

Yes, sure. I would have less reservation towards push -f if there was a mechanism to verify that "you are not collaborating with anyone on this branch". Perhaps @goldfirere started hacking on it already?... Regarding bisecting, reverting etc. I've never encountered any problems with that in practice. Did you?

Mikolaj · 2022-09-03T10:17:23Z

Yes, sure. I would have less reservation towards push -f if there was a mechanism to verify that "you are not collaborating with anyone on this branch". Perhaps @goldfirere started hacking on it already?...

Fair enough.

Regarding bisecting, reverting etc. I've never encountered any problems with that in practice. Did you?

I did. With both, but most of all with reading and understanding the intention of PRs, because they are no longer self contained entities, but amalgams with whatever else they merge.

sfindeisen · 2022-09-05T07:37:33Z

I got rid of @goldfirere 's code, making this PR even smaller. OK to merge, @Mikolaj ?

Mikolaj · 2022-09-05T07:40:08Z

I got rid of @goldfirere 's code, making this PR even smaller. OK to merge, @Mikolaj ?

Well done. Please do.

Mikolaj · 2022-09-05T10:35:58Z

@sfindeisen: Thanks a lot for the PR. This now complements #57 to become a comprehensive solution to UnkindPartition/tasty#337 that can be added on top of Tasty and that does not use HUnit nor any other non-base and non-tasty packages.

sfindeisen · 2022-09-05T16:23:46Z

@Mikolaj thanks.

sfindeisen mentioned this pull request Aug 27, 2022

In approx tests, HUnitFailure is not caught and not pretty-printed #60

Closed

sfindeisen requested a review from Mikolaj August 27, 2022 15:26

sfindeisen commented Aug 31, 2022

View reviewed changes

sfindeisen marked this pull request as ready for review August 31, 2022 09:07

sfindeisen commented Aug 31, 2022

View reviewed changes

sfindeisen requested a review from goldfirere August 31, 2022 09:42

Mikolaj approved these changes Aug 31, 2022

View reviewed changes

sfindeisen and others added 10 commits September 3, 2022 11:19

implement provisional HUnitFailure exception handler

ced16af

catch SomeException instead of HUnitFailure; rethrow

052b1d4

rewrite the exception handler

68399e7

improve debug msg

37ef959

remove unused code

6cae9e6

remove unused import

1d480e1

remove unused dependency (cabal)

80e1d72

add TODO

82afa16

add copyright notice (Richard Eisenberg)

0e31549

sfindeisen force-pushed the sfindeisen/60-HUnitFailure-catch branch from 5598815 to 0e31549 Compare September 3, 2022 09:19

get rid of assertApproxEqual

4c895e9

sfindeisen removed the request for review from goldfirere September 5, 2022 07:34

sfindeisen merged commit 53e1281 into master Sep 5, 2022

sfindeisen deleted the sfindeisen/60-HUnitFailure-catch branch October 28, 2022 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pretty-print HUnitFailure exceptions #63

pretty-print HUnitFailure exceptions #63

sfindeisen commented Aug 27, 2022 •

edited

Loading

sfindeisen Aug 31, 2022

Mikolaj Aug 31, 2022

sfindeisen Aug 31, 2022 •

edited

Loading

Mikolaj Aug 31, 2022

sfindeisen Aug 31, 2022

sfindeisen Aug 31, 2022

Mikolaj Aug 31, 2022

Mikolaj Aug 31, 2022 •

edited

Loading

sfindeisen Aug 31, 2022

Mikolaj left a comment •

edited

Loading

Mikolaj commented Aug 31, 2022

Mikolaj commented Sep 2, 2022

sfindeisen commented Sep 3, 2022

Mikolaj commented Sep 3, 2022

sfindeisen commented Sep 3, 2022

Mikolaj commented Sep 3, 2022

sfindeisen commented Sep 3, 2022

Mikolaj commented Sep 3, 2022

sfindeisen commented Sep 5, 2022

Mikolaj commented Sep 5, 2022

Mikolaj commented Sep 5, 2022

sfindeisen commented Sep 5, 2022

pretty-print HUnitFailure exceptions #63

pretty-print HUnitFailure exceptions #63

Conversation

sfindeisen commented Aug 27, 2022 • edited Loading

sfindeisen Aug 31, 2022

Choose a reason for hiding this comment

Mikolaj Aug 31, 2022

Choose a reason for hiding this comment

sfindeisen Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Mikolaj Aug 31, 2022

Choose a reason for hiding this comment

sfindeisen Aug 31, 2022

Choose a reason for hiding this comment

sfindeisen Aug 31, 2022

Choose a reason for hiding this comment

Mikolaj Aug 31, 2022

Choose a reason for hiding this comment

Mikolaj Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

sfindeisen Aug 31, 2022

Choose a reason for hiding this comment

Mikolaj left a comment • edited Loading

Choose a reason for hiding this comment

Mikolaj commented Aug 31, 2022

Mikolaj commented Sep 2, 2022

sfindeisen commented Sep 3, 2022

Mikolaj commented Sep 3, 2022

sfindeisen commented Sep 3, 2022

Mikolaj commented Sep 3, 2022

sfindeisen commented Sep 3, 2022

Mikolaj commented Sep 3, 2022

sfindeisen commented Sep 5, 2022

Mikolaj commented Sep 5, 2022

Mikolaj commented Sep 5, 2022

sfindeisen commented Sep 5, 2022

sfindeisen commented Aug 27, 2022 •

edited

Loading

sfindeisen Aug 31, 2022 •

edited

Loading

Mikolaj Aug 31, 2022 •

edited

Loading

Mikolaj left a comment •

edited

Loading