Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a QuickCheck-style MoreActions type modifier to make it easier to increase the number of actions on average in tests #84

Merged
14 commits merged into from
Oct 24, 2024

Conversation

MaximilianAlgehed
Copy link
Collaborator

I'm not 100% sure I like this design, but now you can do quickCheck $ prop_BlaBla . getMoreActions to increase the number of actions generated in traces without changing other stuff.

The thing I don't like about this is that it doesn't take into account wanting to add other modifiers at the same time. What if we wanted a modifier PostiveActions for polarity for example? quickCheck $ prop_BlaBla . getMoreActions . getPositiveActions would be a bit messy to implement as it stands now. Something to think about?

Checklist:

  • Check source-code formatting is consistent

@MaximilianAlgehed MaximilianAlgehed requested review from a user and UlfNorell September 19, 2024 18:28
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I like the idea, I have left comment suggesting possible improvements .

quickcheck-dynamic/src/Test/QuickCheck/StateModel.hs Outdated Show resolved Hide resolved
quickcheck-dynamic/test/Spec/DynamicLogic/RegistryModel.hs Outdated Show resolved Hide resolved
quickcheck-dynamic/src/Test/QuickCheck/StateModel.hs Outdated Show resolved Hide resolved
quickcheck-dynamic/src/Test/QuickCheck/StateModel.hs Outdated Show resolved Hide resolved
@MaximilianAlgehed
Copy link
Collaborator Author

MaximilianAlgehed commented Sep 20, 2024

I'm not happy with the design here. The current design makes it possible to do something like:
newtype MoreActions state and newtype PositiveActions state but it doesn't make it possible to combine MoreActions and PositiveActions. I think that's a shame and it would be nice to be able to combine type-level modifiers. One way to get around this would be to do something like:

class ArbitraryWithOptions o a where
  arbitraryWithOptions :: o -> Gen a
  shrinkWithOptions :: o -> a -> [a]
instance ArbitraryWithOptions (GenActionsOptions state) a => Arbitrary a
instance ArbitraryWithOptions (GenActionsOptions state) (Actions state)
instance ArbitraryWithOptions (GenActionsOptions state) a => ArbitraryWithOptions (GenActionsOptions state) (MoreActions a)
instance ArbitraryWithOptions (GenActionsOptions state) a => ArbitraryWithOptions (GenActionsOptions state) (PositiveActions a)

But that would introduce the extra overhead of the ArbitraryWithOptions class, but maybe that's worth-while? It would certainly be more flexible than the current approach.

Thoughts very much welcome from @abailly-iohk and @UlfNorell.

@MaximilianAlgehed
Copy link
Collaborator Author

Another more promising approach might be something like this:

data QCDProperty state
toQCD :: Testable p => (Actions state -> p) -> QCDProperty state
instance StateModel state => Testable (QCDProperty state)
withMoreCommands :: Rational -> QCDProperty state -> QCDProperty state
withPositiveActions :: QCDProperty state -> QCDProperty state

We could add some sugar to this to avoid having to write crap like withMoreCommands . toQCD . prop_whatever and make withMoreCommands slightly more polymorphic.

I think this introduces a nicer interface than having the MoreActions type based interface that is more compositional and it addresses @UlfNorell's issue with fixed modifer in MoreActions. Furthermore, it avoids the orphaned instances for Arbitrary that my proposal above suffers from.

@MaximilianAlgehed
Copy link
Collaborator Author

I've re-designed this to be in line with my last comment. It's much nicer now than it was before. It wouldn't hurt to have @abailly-iohk and @UlfNorell look it over and see if there is anything else we'd like to do here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks much better than initial proposal!

@MaximilianAlgehed
Copy link
Collaborator Author

closes #85

@MaximilianAlgehed
Copy link
Collaborator Author

@abailly-iohk I think we are ready to merge this, but the CI rules are a little bit in the way:
image

@ghost
Copy link

ghost commented Oct 24, 2024

I never remember how to disable those, but will do

@ghost ghost merged commit 071092c into main Oct 24, 2024
3 checks passed
@ghost ghost deleted the PR-more-commands branch October 24, 2024 08:48
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need more control over distribution of lengths of action sequences
2 participants