-
Notifications
You must be signed in to change notification settings - Fork 459
Use tasty-golden for golden tests and ditch vendored hspec-expectations package. #125
Conversation
Not sure where this is coming from, but we don't want the builders to get stuck.
|
|
||
| source-repository-package | ||
| type: git | ||
| location: https://github.com/rewinfrey/hspec-expectations-pretty-diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Down to only two vendored packages left (the hard ones, unfortunately).
| testsForLanguage language = do | ||
| let dir = "test/fixtures" </> language </> "corpus" | ||
| let items = unsafePerformIO (examples dir) | ||
| localOption (mkTimeout 3000000) $ testGroup language $ fmap testForExample items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests sometimes get held up; I’m not sure why (I think it’s a race condition w/r/t file I/O), but this timeout remedies it so the builders won’t get stuck for 10 minutes before it happens. (Worth filing a bug about, I think.)
| -- tests and find this output hard to read, install the `jd` CLI tool | ||
| -- (https://github.com/josephburnett/jd), which will print a detailed | ||
| -- summary of the differences between these JSON files. | ||
| renderDiff :: String -> String -> [String] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The major compromise associated with giving up hspec-expectations-pretty-diff is that the quality of the diffs went down. However, tasty-golden lets us provide our own diffing invocation, which means we can retain the excellent output.
| let nativeSpecs = tests session | ||
| asTastySpecs <- Tasty.testSpecs $ legacySpecs session | ||
| let allSpecs = nativeSpecs <> asTastySpecs | ||
| pure . Tasty.localOption Tasty.Success $ testGroup "semantic" allSpecs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tasty has no concept of pending tests, so we mark our pending tests as successes.
| {-(KeyValue | ||
| {-(TextElement)-} | ||
| {-(Float)-})-} | ||
| {-(KeyValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we don’t use verbatim, we care about whitespace here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A feature, IMO.
| @@ -1,36 +1,53 @@ | |||
| {-# LANGUAGE ImplicitParams, LambdaCase, NamedFieldPuns #-} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Countdown to Rob requesting NoImplicitParams in 3… 2… 1… 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I do think implicit params are a better solution than plumbing parameters everywhere or Reader + umpteen calls to pure. But we should arguably do the let ?session in the top-level Spec.hs file.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u wot m8
The purist in me would prefer a Reader effect, but I see the appeal. Tell you what: you maintain it and we’re good 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I kid, I kid. I’m cool with this 👍)
robrix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @patrickt ❤️
| other-modules: Paths_semantic | ||
| build-depends: base >= 4.12 && < 5 | ||
| , ansi-terminal ^>= 0.8.2 | ||
| , ansi-terminal >= 0.8.2 && <1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pourquoi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not technically needed, but ansi-terminal just hit 0.9.0, and the old hspec-expectations-pretty-diff was complaining about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha.
| import: haskell, dependencies, executable-flags | ||
| type: exitcode-stdio-1.0 | ||
| hs-source-dirs: test | ||
| ghc-options: -Wunused-imports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we apply this in a common stanza?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. We should really just turn on -Wall everywhere, but I’ll file a separate issue about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig it; the thing fused-effects does is working pretty nicely.
| @@ -1,36 +1,53 @@ | |||
| {-# LANGUAGE ImplicitParams, LambdaCase, NamedFieldPuns #-} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u wot m8
The purist in me would prefer a Reader effect, but I see the appeal. Tell you what: you maintain it and we’re good 😉
| {-(KeyValue | ||
| {-(TextElement)-} | ||
| {-(Float)-})-} | ||
| {-(KeyValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A feature, IMO.
This patch:
hspec-expectations-pretty-diff, which partially unblocks Publish to Hackage/Stackage. #16.unsafePerformIOin places where it is somewhat unavoidable (I defendunsafePerformIO’s use in tests)tasty-goldenlets us provide our own diff command, and attempts to usejdfor JSON comparisons.hspec-expectationsFixes #120.