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

Generator for creating command sequences of scaled length #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mfherbst
Copy link

@mfherbst mfherbst commented Mar 6, 2016

Added a generator based on rc::state::gen::commands for generating
command sequences with a shorter or longer length than default. The
size parameter of the commands themselves is left unaltered.

Implements the mechanism discussed in #110. I am happy to implement tests or document the feature as well, just let me know if you agree with the interface.

Added a generator based on ``rc::state::gen::commands`` for generating
command sequences with a shorter or longer length than default. The
size paramter of the commands themselves is left unaltered.
@emil-e
Copy link
Owner

emil-e commented Mar 6, 2016

Really happy when people contribute!

Some thoughts:

  • In your case, the problem was the absolute number of commands that were generated. If the way the sequence length is determined were to change completely, would this still solve your problem? There are a number of possible versions I could think of:
    • The version you have here. It's nice in that it's simple to implement but it really says nothing about the number of commands if that is important. That's the "problem" with size in general, it's intentionally not defined what it means and the reason why you would reduce it is for practical reasons (i.e. performance), not because you want to constrain the generated values.
    • A version that takes a fixed number of commands to generate. The problem with this is that you probably don't want to shrink the number of commands in the sequence on a failing test since that would violate the user intent, i.e. "Hey, I said 54 commands, not 3". This is the way gen::container(int, Gen<Element>) works.
    • A version that takes a maximum sequence length. As long as it's made clear in the documentation that it is a maximum and not and exact, it is fine to shrink it on failure.
    • Have the number of commands be a part of the model and have it as a precondition that the number of commands applied is less than the maximum. This actually makes a lot of sense to me since in your case, this property actually does seem to be a part of the system and so it would make sense to have it reflected in the model. The problem right now is that RapidCheck will fail if it can't generate enough commands in order to protect against cases where all tests succeed simply because the command sequences are so short.
  • The convention previously has been to not have versions of other generators have different names but instead to use overloads if the argument sets are different, such as in this case. I don't know if that's a good thing, though, so I could be convinced otherwise.

I'd love to solve this, I just want to make sure we do it the right way so comments are welcome .)

@mfherbst
Copy link
Author

Quick update: Actually you raised a couple of good points there, which I did not manage to think about properly so far, since I had a pretty busy week. I agree that it's better to tackle the issue properly and once and for all. I'll get back when I got any further ideas.

@emil-e
Copy link
Owner

emil-e commented Mar 11, 2016

It's not always easy to come with the right solution, you hsve to strike the right balance between usability and flexibility.

One thing I've been thinking about is to have versions where the length of a sequence (also applies to gen::container) can be configured to be in a given range, min/max. That should give most people all the flexibility they could want. Fixed length would only be a special case of this then.

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.

2 participants