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

Delete unseen transitions in state machine test #388

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

henriiik
Copy link
Contributor

@henriiik henriiik commented Oct 19, 2023

This PR adds a new shrinking step to proptest_state_machine::SequentialValueTree that deletes all transitions not seen by the test runner as the first step. This is related to #387.

To do that it changes the value type from a Vec<Transition> to a new wrapper type ObservedVec<T>. This new type contains a counter that is shared with SequentialValueTree and keeps track of how many transitions have been seen by the test runner.

The shared counter is reset on each call to simplify / complicate, therefore it is no longer valid to call current multiple times without calling simplify and/or complicate in between.

Given my (admittedly limited) understanding of how proptest works this is not an issue as the intention is for the value tree to be used in a loop where each iteration starts with a call to current which produces a value that is used in a test run and based on the test result simplify or complicate is called at least one time before the next iteration.

Any feedback would be welcome!

@henriiik henriiik changed the title delete unseen transitions in state machine test Delete unseen transitions in state machine test Oct 19, 2023
@tzemanovic
Copy link
Contributor

thank you @henriiik, will take a look

Copy link
Contributor

@rexmas rexmas left a comment

Choose a reason for hiding this comment

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

left one comment, the rest of the changes make a lot of sense to me. @tzemanovic knows this code best though so we'll wait for his feedback :)

thanks for the contribution 👍

@tzemanovic
Copy link
Contributor

tzemanovic commented Mar 19, 2024

I've rebased the branch on proptest-state-machine-0.2.0 and added couple fix-ups (4c8696d and 0aa8087 - should be squashed before merging)

I've got some more changes that I'd like to add WIP, will update here soon

@henriiik
Copy link
Contributor Author

Thanks for picking this up, let me know if you want me to help in any way :)

@tzemanovic
Copy link
Contributor

Thanks again @henriiik, I'd like to add the following changes:

  • Return the ref to seen_transitions_counter from fn current separately without wrapping the transitions in ObservedVec and increment the counter from StateMachineTest::test_sequential.
  • Because we can assume that the test results are deterministic (otherwise the shrinking process is not useful), I think we can safely remove the unseen transitions without undoing it in fn complicate. This is unlike the other Shrink cases where we don't know if the shrinking will affect the result. Furthermore, we only need to count the seen transitions before shrinking as it shouldn't have any effect anymore when we're removing transitions 1-by-1.
  • Remove the first panicking check in fn current - it prevents multiple refs to seen_transitions_counter which should not be necessary, we can allow multiple refs if needed for customization. I kept the second condition, but slightly changed the message.

The last 2 fixup commits make these changes, if you could pls check them over to make sure that this still works for your use-case.

@tzemanovic
Copy link
Contributor

I've opened #434 with the fixups squashed, I can push it back here if you're happy with the changes

@henriiik
Copy link
Contributor Author

@tzemanovic Looks good to me!

@tzemanovic tzemanovic merged commit 418c912 into proptest-rs:master Mar 21, 2024
5 checks passed
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.

3 participants