Add Test.Hspec.JUnit.Formatter

[pbrisbin]

This PR adds an implementation of our formatter via the newer "hooks" feature of Hspec, as documented here and here.

There are other general cleanups in this PR, but the import parts are:

• Test.Hspec.JUnit.Format: this is a copy/pasted version of junitFormat but targeting the Format.V1 types, not Core.Format. I chose to copy (instead of refactor) because I've deprecated, and plan to remove soon, junitFormat
• Test.Hspec.JUnit.Formatter: this packages things up into hooks; all these functions take an explicit JUnitConfig argument
• Test.Hspec.JUnit.Formatter.Env: same as above, but reading JUNIT_* environment variables instead of accepting that argument

[pbrisbin]

This isn't totally done, but here is a little bit of an experience report so far:

The good

• I love hooks

  And with the additional functions from hspec-api, making a SpecWith a -> SpecWith a to do whatever you want configuration-wise is a breeze. This is all much better than the previous options for doing this sort of stuff. I love that test/SpecHook.hs works with hspec-discover, although that was a bit hard to, ahem, discover. For example, I still don't know if it loads all test/**/*Hook.hs like it does *Spec.hs, or if I only get one.

The bad

• There are two competing abstractions for a "formatter"

  As far as I can tell, everything wants a format function (FormatConfig -> IO Format), and that's what I should be packaging in my hooks, but then hspec's own checks, specdoc, etc are Formatters and converted with formatterToFormat. Should I be mimicking that instead? Or should I be providing both?

• The versioning muddies that further

  There are 2 versions of Format and 3 versions of Formatters. Should I try to target latest version or oldest versions of things? As long as users are interacting with a hook, it seems like the choice is internal, but I may be missing something. I chose to go latest, but it did require a touch of CPP.

• Implementing add was harder than it should be

  I'm curious why configFormat is a Maybe and defaulted to checks at the end, rather than being non-Maybe and defaulted to checks at initial construction. This would've made the idea of "adding" a formatter from the outside more obvious and harder to get wrong. I'm not entirely sure what I have here will work correctly in all cases.

[sol]

This isn't totally done, but here is a little bit of an experience report so far:

The good

• I love hooks

🎉

I still don't know if it loads all test/**/*Hook.hs like it does *Spec.hs, or if I only get one.

Did you read https://hspec.github.io/hspec-discover.html#spec-hooks?

The bad

• There are two competing abstractions for a "formatter"

This is for historic reason. Originally there was only Formatter, which is suitable for text formatters in line with hspec's default formatters. Format is more general and less opinionated, and suitable for what you're doing. I understand that this is confusing. My attempt is to tackle this with documentation, by only documenting the "modern" approach: https://hspec.github.io/extending-hspec-formatter.html

As far as I can tell, everything wants a format function (FormatConfig -> IO Format), and that's what I should be packaging in my hooks

Yes.

but then hspec's own checks, specdoc, etc are Formatters and converted with formatterToFormat. Should I be mimicking that instead?

No.

Or should I be providing both?

No.

• The versioning muddies that further
  There are 2 versions of Format and 3 versions of Formatters. Should I try to target latest version or oldest versions of things? As long as users are interacting with a hook, it seems like the choice is internal, but I may be missing something. I chose to go latest, but it did require a touch of CPP.

You shouldn't use CPP. The whole point of versioning the APIs is to not require users of the API to use CPP.

• default to using the latest version of an API
• if for some reason you want to support older versions of Hspec, then use the latest version of an API that is supported by that version of Hspec

• Implementing add was harder than it should be
  I'm curious why configFormat is a Maybe and defaulted to checks at the end, rather than being non-Maybe and defaulted to checks at initial construction. This would've made the idea of "adding" a formatter from the outside more obvious and harder to get wrong.

I agree that for this specific situation, non-Maybe would be nicer. In general it can also be useful if, for a config value, you can determine whether the user changed the value, or whether it's still the default value. Maybe allows for that.

If we would start from a clean slate, we could go with non-Maybe, but changing it now would be a breaking change, so that's not going to fly.

I'm not entirely sure what I have here will work correctly in all cases.

It will work, except for the limitation you pointed out at hspec/hspec#895 (comment) (if a user selects a different format with --format, they will lose the JUnit output)

[pbrisbin]

Thanks for the details. All makes sense.

Did you read https://hspec.github.io/hspec-discover.html#spec-hooks?

Read it now and I think I understand. It might be useful to document what order loaded hooks are run in when there are multiple, since that can be important.

by only documenting the "modern" approach

That makes sense. I probably got confused because I started this whole journey with a PR in hspec itself, and so was exposed first to Formatter.

If we would start from a clean slate, we could go with non-Maybe, but changing it now would be a breaking change

Agreed and agreed.

if for some reason you want to support older versions of Hspec, then use the latest version of an API that is supported by that version of Hspec

I think I do, but I'm not sure if the reason is sound. We try to support older GHCs for as far back as is reasonable without too much fuss. And, pragmatically, we test this on CI by using (and supporting) older Stackage LTSs. So, while I don't have an explicit reason to target older hspecs, it can be necessary in order to run in an older LTS overall, as sometimes pulling in one latest extra-dep on top of a very old resolver can get you into a spiral of transitive issues.

I guess my question would be, if I remove my CPP and just target V1 (meaning, I support hspec-api-0.10), is there any downside?

[pbrisbin]

if a user selects a different format with --format, they will lose the JUnit output

Oh, that's actually worse than the limitation I was describing there. I was hoping that add would work such that passing --format would change the default, but JUnit would still be added to it. I guess this means CLI arguments are processed after any modifyConfig rather than before?

[sol]

I guess my question would be, if I remove my CPP and just target V1 (meaning, I support hspec-api-0.10), is there any downside?

Not really, Test.Hspec.Api.Format.V1 is simply translated to a Test.Hspec.Core.Format.Format function. As long as you're happy with the interface that V1 provides, you are good.

[sol]

I guess this means CLI arguments are processed after any modifyConfig rather than before?

Yes, otherwise formatters added with registerFormatter would not be available to be selected with --format.