Skip to content

Conversation

@brprice
Copy link
Contributor

@brprice brprice commented Mar 29, 2022

Upstream has deprecated testProperty in favour of testPropertyNamed. To adapt to this change we additionally

  • upgrade tasty-discover so we can use the new custom tasty_... tests instead of hprop_..., since tasty-discover generates code referencing testProperty for those.
  • upgrade hlint to avoid warnings that tasty_... should be CamelCased

@brprice brprice requested a review from dhess March 29, 2022 11:32
@brprice
Copy link
Contributor Author

brprice commented Mar 29, 2022

Note that this changes the test runner output to look like (just for the laws test of STE and STV)

  STE                                                                          
    Semigroup                                                                  
      Associativity:  OK (0.23s)                                                                                                                               
          ✓ <internal-property> passed 100 tests. 

(Note the <internal-property> rather than the current Associativity)
For a failing test we get the hedgehog instructions

This failure can be reproduced by running:                                                                                                         
> recheck (Size 6) (Seed 9578155960577194227 4715574170518147751) <internal-property>

rather than the current recheck ... Associativity.

The other option is to keep exactly the status quo, by following GaloisInc/what4#195 (comment)

@brprice
Copy link
Contributor Author

brprice commented Mar 29, 2022

@dhess: this is based on your dhess/ghc9 branch as tasty-hedgehog 1.2.0 does not seem to be available on main (I guess haskell.nix needs to be bumped). It may be worth rebasing this commit to happen before the ghc9 change?

@dhess
Copy link
Member

dhess commented Mar 29, 2022

Thanks. You're probably correct that main's Hackage index doesn't include 1.2.0. I believe 1.2.0 isn't necessary for GHC 9, so what I'll do now is un-bump tasty-hedgehog in #244, get that merged, then rebase this PR.

@dhess dhess requested a review from georgefst March 29, 2022 11:46
@dhess
Copy link
Member

dhess commented Mar 29, 2022

Looping in @georgefst and asking for his review to get his thoughts on this test output change.

Copy link
Contributor

@georgefst georgefst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not convinced that upstream have taken the best approach in qfpl/tasty-hedgehog#42. Wouldn't it have been better to e.g. patch Hedgehog to allow the whole recheck message to be hidden, for when it's embedded within some test runner like Tasty? As @brprice noted:

the instructions to rerun via tasty are correct, and are the supported way to run the tests

But this seems like the best thing we can do from down here.

@dhess
Copy link
Member

dhess commented Mar 29, 2022

OK then, I'll merge this after #244 is merged.

@dhess dhess force-pushed the dhess/ghc9 branch 7 times, most recently from df7e1ee to 89a6cfc Compare May 2, 2022 17:05
@dhess dhess force-pushed the dhess/ghc9 branch 11 times, most recently from b1f2cfe to 14a5730 Compare May 7, 2022 11:35
dhess
dhess previously approved these changes Jun 22, 2022
Copy link
Member

@dhess dhess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks fine, once the history is cleaned up. Thanks for tackling this un-fun work.

@brprice brprice force-pushed the brprice/tasty-hedgehog-1.2 branch 2 times, most recently from 98b915e to ce11a19 Compare June 22, 2022 21:00
@brprice
Copy link
Contributor Author

brprice commented Jun 22, 2022

I have rebased onto main

@dhess
Copy link
Member

dhess commented Jun 23, 2022

I apologize for waffling, but in light of all the hlint and HLS hacking that's in this PR in its current state, I've changed my mind (again) about merging it. See #349 (comment) for details.

@dhess dhess dismissed their stale review June 23, 2022 13:36

Changed my mind.

@brprice brprice mentioned this pull request Jun 23, 2022
@dhess
Copy link
Member

dhess commented Jun 23, 2022

Great, this is now ready to go modulo rationalizing the commit history. (Let's start over; I think it can be just one commit now.)

(edit: Note for posterity — it was removing all the hlint and HLS workarounds and adding the one new directive in our hlint config that changed my mind about this w.r.t. my previous comment.)

brprice added 4 commits June 27, 2022 12:34
We would like to upgrade tasty-hedgehog to 1.2.0.0, but this introduces
a change (deprecating `testProperty`) which forces large (mechanical)
changes to our codebase, and an upgrade of tasty-discover to stay -Wall
clean. It also needs configuration for hlint, to avoid lint errors. We
separate these three upgrades into their own commits.
This is required to not complain about `tasty_...` custom name tests
being not CamelCase. We currently do not have any of these in the
codebase, but in the next commit we will migrate our tests to use this
new feature of tasty-discover. That will enable us to upgrade
tasty-hedgehog and avoid deprecation warnings.

This change should only be necessary temporarily until hlint makes a new
release (the development version of hlint no longer warns about
`tasty_...` names, similarly to how hlint normally does not warn about
`hprop_...` names) and haskell-language-server makes a new release
incorporating this change (hls bundles its own version of hlint).
This doesn't change any behaviour. This is simply setup so we can
upgrade tasty-hedgehog in the next commit and avoid deprecation warnings
about `testProperty` (which tasty-discover emits for hprop_ tests).
Note that 'testProperty' has been deprecated in favour of
'testPropertyNamed'.

This upstream change was to allow hedgehog and tasty to have different
notions of the test's name: tasty wants a display name and hedgehog
wants a top-level value that can be used with 'recheck' in ghci.

Since (for Tests.Question.test_laws) we don't have such a value, we just
use the static string "<internal-property>". Hopefully this means that
the incorrect hedgehog instructions to 'recheck' any failing properties
under 'test_laws' will be obviously wrong.  Note that the instructions
to rerun via tasty are correct, and are the supported way to run the
tests. This is an improvement on the status quo where the 'recheck'
instructions were less obviously incorrect.
@brprice brprice force-pushed the brprice/tasty-hedgehog-1.2 branch from 3432f44 to fdd533a Compare June 27, 2022 11:38
@brprice
Copy link
Contributor Author

brprice commented Jun 27, 2022

Great, this is now ready to go modulo rationalizing the commit history. (Let's start over; I think it can be just one commit now.)

(edit: Note for posterity — it was removing all the hlint and HLS workarounds and adding the one new directive in our hlint config that changed my mind about this w.r.t. my previous comment.)

I have rebased to clean history. I kept it in 4 commits, to separate out the large mechanical change from the small interesting change, and also to separate out the hlint change so it is easy to revert later (see #544 )

Copy link
Member

@dhess dhess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the rework.

@georgefst you may want to review this, as well. (edit: Oh, I see he already did, and presumably the PR has not changed materially since his review, so maybe no need, then.)

@georgefst georgefst linked an issue Jun 27, 2022 that may be closed by this pull request
@brprice brprice added this pull request to the merge queue Jun 27, 2022
@dhess dhess removed the Do not merge Do not merge label Jun 27, 2022
@brprice brprice merged commit 5d2cd96 into main Jun 27, 2022
@brprice brprice deleted the brprice/tasty-hedgehog-1.2 branch June 27, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Something that needs cleaning up priority: medium This issue has medium priority testing Related to tests/testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bump tasty-hedgehog

4 participants