-
Notifications
You must be signed in to change notification settings - Fork 26
Test Genesis State Machine with quickcheck-dynamic
#1413
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
base: main
Are you sure you want to change the base?
Conversation
ab71f71
to
ae48bb8
Compare
ad743f9
to
7d704d0
Compare
@@ -1,4 +1,4 @@ | |||
module Test.QuickCheck.Extras ( | |||
module Test.Ouroboros.Consensus.QuickCheck.Extras ( |
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.
This module needs to be moved, as quickcheck-dynamic
also has Test.QuickCheck.Extras
.
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/GSM/QD.hs
Outdated
Show resolved
Hide resolved
521c87e
to
3261feb
Compare
bf22c03
to
ba4c4f4
Compare
- bump the index state in `cabal.project` - update hackage.nix, use the legacy `for-stackage` branch, as we still support GHC 8.10. See [this note](https://github.com/input-output-hk/hackage.nix/blob/58d6ddbb7cb41518a2f61a6b1f123308f6eece76/default.nix#L2) for more details. - bump tool index state This PR is needed to remove an s-r-p on `quickcheck-dynamic` in #1413
1ade547
to
2de5a06
Compare
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.
This looks great!
Thanks a lot for the thorough attention to labelling! One further observation: we set the minimum command length to 20 for QSM; for QD, I get this distribution:
# of actions (1000 in total):
41.5% "0 - 9"
19.4% "10 - 19"
12.2% "20 - 29"
7.6% "30 - 39"
6.0% "40 - 49"
3.1% "50 - 59"
3.0% "100 - 199"
2.9% "60 - 69"
2.2% "70 - 79"
1.1% "80 - 89"
0.8% "90 - 99"
0.2% "200 - 299"
This still seems completely fine, especially as the chosen value of 20 for QSM was ad-hoc either way.
The PR description can be updated in two aspects:
- The way of using
reflection
changed as of Test Genesis State Machine withquickcheck-dynamic
#1413 (comment). - The new release of
quickcheck-dynamic
seems to have happened already.
Apart from that, I think we can now proceed as described in the PR description and remove the vendored QSM code and the QSM GSM test.
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/GSM/QD.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/GSM/QD.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/GSM/QD.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/GSM/QD.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/GSM/QD.hs
Outdated
Show resolved
Hide resolved
60ecd1f
to
7d887a9
Compare
db77deb
to
65cf44b
Compare
- add tests based on quickcheck-dynamic - remove old tests based on quickcheck-state-machine, substituted by the above - Test.QuickCheck.Extras -> Test.Ouroboros.Consensus.QuickCheck.Extras
65cf44b
to
d3a2163
Compare
I think the failing reference must now point to https://developers.cardano.org/docs/get-started/cardano-node/installing-cardano-node |
I've fixed the link, thanks @jasagredo! |
Blocked on update of |
Fixes #1149
"no changelog", as the changes only affect the
ouroboros-consensus-diffusion:consensus-test
package.In this PR, we refactor the
Test.Consensus.GSM
modules, which contain the tests for the Genesis State Machine component, to use a different state machine testing framework. Specifically, we are switching the tests fromquickcheck-state-machine
toquickcheck-dynamic
. The main motivation for the switch is being able to run the Genesis State Machine inIOSim
in order to benefit fromIOSim
s ability to manipulate the passage of time and this speed-up the tests.While the current GSM tests do run in
IOSim
, this is only possible due to vendoring a copy ofquickcheck-state-machine
that allows usingIOSim
, which we would like to avoid long-term.Specifically, this PR:
Below, I describe some aspects of the switch that I consider worth highlighting, hopefully making the review more manageable.
Differences from
quickcheck-state-machine
testsWhile I've tried to replicate the existing
quickcheck-state-machine
-based tests as closely as possible, there are some unavoidable differences between the test suites stemming from how thequickcheck-dynamic
library is structured and implemented.Different distributions of command sequences
While the user can specify the logic to generate individual commands, both
quickcheck-dynamic
andquickcheck-state-machine
have internal hard-coded logic to generate sequences of commands:quickcheck-dynamic
quickcheck-state-machine
Here are the partial tests reports, which show that QSM produces more uniform distribution of commands:
Similarly, for 5 peers:
QD also generates significantly more commands than QSM, probably due to longer defaults length for command sequences.
The glue code of
quickcheck-dymamic
is essentially pure and thus allows to useIOSim
The type of
quickcheck-state-machine
's runCommands function constraints the underlying monad to IO:This is needed, because
quickcheck-state-machine
usesTChan
s internally to orchestrate the interaction between the model and the system-under-test. This in particular makes it impossible to run the system-under-test inIOSim
and motivated us to vendor a copy of QSM with the types adapted to permitIOSim
.In
quickcheck-dymamic
, commands are called actions, and run using therunActions function, whose type gives the user much more freedom:
I.e. the user needs to instantiate the
RunModel
class, where the library only requiresm
to be aMonad
. The implementation ofrunActions
itself is essentially pure and only uses the facilities of Test.Quickcheck.Monadic to invoke the underlying monadic code of the system-under-test.Slightly different test setup routine
While
quickcheck-state-machine
represents state machines as records of functions, and thus gives a lot of flexibility to the user in terms pasteurisation,quickcheck-dynamic
is not as flexible, as it uses type classes.Specifically, I was having a hard time to emulate the pattern of parameterising the state machine with a
Context
that exists in the current QSM test (see here, for example). TheContext
data type contains parameters that are generated withquickcheck
and are fixed over the execution of one property test, i.e. they are static from the state machine's point of view.quickcheck-dynamic
does not provide a way to supply static information to the state machine out of the box, thus not allowing to configure the initial state according to some external static parameter and presenting us with an instance of the classic configuration problem. I've tried several ways to solve this particular instance, but the turned out to be using reflection as suggested by @nfrisby. Specifically, I've taken inspiration from this blogpost by John Wiegley, where he uses reflection to provide configuration toquickcheck
generators --- this is almost exactly what we want here!The essence of the solution is to constrain the
StateModel
instance with an constraint provided byreflection
. This way, we get to use have the reflected parameters in theinitialState
method:We would then create a value of
StaticParams
in the property definition:The only tricky thing here is to explicitly invoke the generator for actions, so that the reified value of StaticParams is in scope.
Next steps
Once the reviewers are happy with the changes, we should:
quickcheck-state-machine
quickcheck-state-machien
that allowsIOSim
GSMTests
Independently:
quickcheck-dynamic
, as this PR relies on the version frommaster
which is not yet released