Add iterative runner #396
Add iterative runner #396
Conversation
There was a problem hiding this comment.
I haven't gone through all the nitty gritty details of the implementation but on first pass my main meta question/comment is:
Why would someone use the SequentialRunner instead of just using the IterativeRunner with no checking of the SutState? Can we just incorporate the SutState in to the sequential runner and remove the need for users to have to decide which to use? We purposely extracted the state machine code in to its own crate versioned below 1.0 so we can make breaking changes as we find the right abstsractions
| @@ -0,0 +1,861 @@ | |||
| use std::{cell::RefCell, marker::PhantomData}; | |||
|
|
|||
There was a problem hiding this comment.
we should add some module docs to the crate root to explain the differences in iterative runner from the regular runner and why you would choose one over the other.
There was a problem hiding this comment.
and in general throughout this file, we should add some doc comments on relevant traits / structs / public methods
| test_runner::{Config, Reason, TestError, TestRunner}, | ||
| }; | ||
|
|
||
| pub fn test_sequential< |
There was a problem hiding this comment.
i wonder if theres some way to unify the sequential and iterative versions a bit more so we don't need separate definitions for things like this. this is nearly the same as the test_sequential function in test_runner.rs
There was a problem hiding this comment.
I'm not sure current types would allow to do that easily. The rational to have this function at all for IterativeRunner is to be able to reproduce the failing test. I will rename it to test_iterative as you suggested.
| S::teardown(sut_state, mod_state) | ||
| } | ||
|
|
||
| pub trait IterativeModelStateMachine { |
There was a problem hiding this comment.
bikeshed: i'd like to think through the names for the types in this crate a bit more so we have a consistent scheme/glossary. this mirrors ReferenceStateMachine in strategy.rs so we're now using Model and Reference as interchangeable it seems.
There was a problem hiding this comment.
The model in the IterativeRunner is an entity that supports generating of the test scenarios rather than the reference model. If the idea is to have both IterativeRunner and the current SequentialRunner in the crate I would say it might be desired to have explicit distinction between the two.
| /// Generate transition based on the state of the model. System under | ||
| /// test can be consulted here as well to reduce number of generated | ||
| /// transitions |
There was a problem hiding this comment.
bikeshed: Generate transition based on the state of the model and system under test.
| fn transition( | ||
| mod_state: &Self::ModelState, | ||
| sut_state: &Self::SutState, | ||
| ) -> BoxedStrategy<Self::Transition>; |
There was a problem hiding this comment.
is there an implicit invariant here that ModelState and SutState are valid together? can they diverge and if so, what is an implementation supposed to do since the generation of the Transition is infallible -- panic?
There was a problem hiding this comment.
ModelState and SutState may diverge, and what is more important and differs this from the intent of SequentialRuner is that ModelState here is a supporting entity, rather than the reference model.
Transition is indeed infallible, and perhaps the better way would be to treat that in the same way as failure to apply a transition to the model or to the system under test.
| fn apply( | ||
| sut_state: Self::SutState, | ||
| mod_state: &Self::ModelState, | ||
| transition: Self::Transition, | ||
| ) -> Self::SutState; | ||
|
|
||
| /// Check some invariant on the SUT state after every transition. | ||
| fn check_invariants(sut_state: &Self::SutState, mod_state: &Self::ModelState) { | ||
| // This is to avoid `unused_variables` warning | ||
| let _ = (sut_state, mod_state); | ||
| } |
There was a problem hiding this comment.
i know you're copying this from the existing trait so not necessarily new but I wonder if we should remove mod_state/ref_state from apply and that is only used for applying a Transition to the System-Under-Test. The fact that apply can check state and check_invariants also exists is a bit redundant and it may be clearer to have it so we have one method for applying a change and one method for validating that change against the reference/model
There was a problem hiding this comment.
To be honest given that Model in IterativeRunner is a less formulated and strict, I would prefer to keep this API as is, as it provides flexibility. For example for actual problem where I needed to use iterative runner the model is implementing both the simplified (not complete) model of the system under test, and also controls the external resources that system under test interacts with. For example the model may not be aware that system under test is issuing the calls towards the external subsystems (because the model is not complete), but system under test would be able to get this data from the model in the apply call. Somewhat similar in the check_invariants API depending on how the model is used, there might be desire to check that state of the model and state of the system under test are in fact in sync here and invariant holds based on the state of the model.
I understand that this explanation is a bit vague, but one of the intents of the IterativeRunner is to provide flexibility that otherwise can't be provided by the existing SequentialRunner.
|
Thank you for spending time to reviewing this.
One of the use-cases for the existing SequentialRunner that I can see is when the But otherwise everything that one may want to implement using SequentialRunner |
This PR adds possibility to perform state machine testing with model that is aware of the system under test, as suggested in the #378 I don't expect this code to be merged as is because while the approach is a fork with some modifications of the existing test_runner, there are some nuances here as well.
Some important changes additionally to iterative way of generating the transitions:
Would love to hear some feedback about the PR