feat: add seed via exception context - #113
Conversation
|
I've been playing with removing annotated-exception in the curricula project so I understand a bit more what exception context actually is, now, and I guess it's not quite what I thought it was when I suggested doing that here. There's an option where we instruct users to add something to a top-level SpecHook that would catch graphula's exception information (be that a seed in an exception context attached to an hunit exception, or a custom exception type containing the seed as a field) and re-throw as an hunit exception with information in the failure reason string. There's an argument to be made that making the hook responsible for error formatting is the only reasonable choice hspec gives you. I'm not sure it's worth it though.
Seems right. But is there any harm in also adding it as exception context? It seems like that would allow us to address #44 (make the seed programmatically obtainable from any type of exception) without changing either the exception type or the hunit failure reason string. |
Makes sense. I'll try that. |
This moves the `throwIO` out of the function that is concretely `HUnitFailure`. This is because I'll next be adding something that needs to occur between those two points.
If using `base > 4.20` we add the graphula seed to the exception context of any thrown exceptions. Closes #44 as best we can. The information won't appear anywhere automatically, but can be pulled out (e.g.) via `SpecHook` for those that know (see README). This is a compromise. Give how `hspec` works today, any other method that would make the seed more visible for non-expectation-failure exceptions comes with too many negative trade-offs.
|
OK, I've done it that way. I clobbered what was here and did a "make the change easy / make the easy change" method in two commits: chore: refactor throw vs exception modificationThis moves the feat: add seed to exception context, if supportedIf using Closes #44 as best we can. The information won't appear anywhere |
| , GenerateKey | ||
| , NoConstraint | ||
| #if MIN_VERSION_base(4,20,0) | ||
| , GraphulaSeed(..) |
There was a problem hiding this comment.
The comment above deprecates this section, did you mean to add this here?
| , NodeOptions | ||
| , GenerateKey | ||
| , NoConstraint | ||
| #if MIN_VERSION_base(4,20,0) |
There was a problem hiding this comment.
I wonder if the CPP guard here causes more confusion than it alleviates; GraphulaSeed could be defined and exported regardless of the base version.
cdmren
left a comment
There was a problem hiding this comment.
Might be worth adding a test, I find Typeable stuff very finicky.
When supported (base-4.20), use
addExceptionContextto add the seed tothe failure. This ensures it happens, not just with expectation failures
(
HUnitFailure), but also with other thrown-exception failures, anddoes so without altering the exception type (as
annotated-exceptionwould).
In base lower than 4.20, behavior is unchanged.
Closes #44.
CAVEAT: this doesn't work :(
HUnit-based testing libraries (e.g hspec) have not updated themselves to
use or display exception context in their output. They would need to
incorporate it into
formatFailureMessagefor expectation failures.Even if/when they do, this also doesn't even work for its intended
purpose (non-expectation-failure exceptions) because
hspec(forexample) uses
showor (optionally)displayException, neither ofwhich include context (which is wild, IMHO).
So, while it would be nice if we could use exception context for this
information, users would still need to update their test runners somehow
to ensure its displayed.
I know the
hspecproject is not amenable to displaying moreinformation like this in its output by default, but I think they do
plan to add some sort of
--verboseflag. If/when that existed, Ithink we could get them to include any exception context in such output,
and we could instruct our uses to use it if they want to see seed.
But is that how we want it to work? Or should we keep this behavior of
stuffing it into strings that we know are always displayed (and just
give up on #44)? I'm leaning this way.