-
Notifications
You must be signed in to change notification settings - Fork 47
Enhance fuzzing for arrays #738 #755
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
Conversation
16ea1b7
to
7ab9dc6
Compare
} | ||
private val limit: Int = | ||
when(recursion) { | ||
1 -> Int.MAX_VALUE |
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.
The logic looks different: before when recursion is 1, 2, 3 and so on then limit is 1. Now it is harder to understand what's happening. The idea behind this was that the limit is articulated as: when we find constructors that accepts another objects then we construct them, but use no more than one available constructor. If recursion is 0, than we pass null to every object parameter in constructor. But usually, we look for constructors with primitive values.
protected val recursion: Int | ||
): ModelProvider { | ||
// TODO: currently it is var due to tests, maybe we can make it private val? | ||
var modelProviderForRecursiveCalls: ModelProvider = |
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.
Looks like we have lost an ability to change model providers for ObjectModelProvider, because before the changed model provider is passed into recursive call. Now, it uses only default providers.
}) | ||
} | ||
} | ||
|
||
private fun generateArrayLengths(description: FuzzedMethodDescription): Set<Int> { |
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.
Looks like this can be simplified. It is too much work to generate some values in interval from 0 up to 10
|
||
var modelProvider: ModelProvider = objectModelProviders(idGenerator) | ||
// TODO: can we make it private val (maybe depending on recursion)? |
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.
It is manual option and a part of public API
protected val idGenerator: IdentityPreservingIdGenerator<Int>, | ||
protected val recursion: Int | ||
): ModelProvider { | ||
// TODO: currently it is var due to tests, maybe we can make it private val? |
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.
It was a part of public API to change a set of model providers. Currently, it looks broken
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.
Please, look at comments
7ab9dc6
to
f218d6b
Compare
|
||
data class ModelConstructor( | ||
val neededTypes: List<ClassId>, | ||
val createModel: (List<FuzzedValue>) -> FuzzedValue |
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.
You can use notation like this:
val createModel: (inputs: List<FuzzedValue>) -> FuzzedValue
to clarify name for parameter. Also, doc will be very useful here
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.
Added name for parameters. Also, added docs here and in other places where needed
modelProviderForRecursiveCalls.map { | ||
if (it is RecursiveModelProvider) | ||
it.copy(idGenerator, recursionDepthLeft - 1).apply { | ||
modelProviderForRecursiveCalls = this@RecursiveModelProvider.modelProviderForRecursiveCalls |
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.
Why don't we copy all these values in the copy
method? It looks better as we want to get a copy of this provider. Here we can leave values that should be changed like totalLimit (but in copy it should be copied, I think)
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.
Renamed to createNewInstance
for more clarity
@@ -412,6 +413,7 @@ class ModelProviderTest { | |||
} | |||
} | |||
|
|||
@Disabled |
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.
Test checks that Outer class is created using only one constructor from the inner and it is the simplest one. I think we should keep this behavior for ObjectModelProvider
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.
Returned this behavior back
val createModel: (List<FuzzedValue>) -> FuzzedValue | ||
) | ||
|
||
abstract class RecursiveModelProvider( |
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.
Please, add docs
*/ | ||
fun defaultModelProviders(idGenerator: IdentityPreservingIdGenerator<Int>, recursionDepth: Int = 1): ModelProvider = | ||
if (recursionDepth >= 0) | ||
nonRecursiveProviders(idGenerator).with(recursiveModelProviders(idGenerator, recursionDepth)) |
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.
Note, that the other method called 'defaultModelProviders' without parameter with recursion has another set of model providers. Therefore. I'd recommend to change name for this method to avoid ambiguous calls.
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 method should have replaced the method without recursionDepth
parameter with no changes in behavior when this parameter is not specified. Seems like I accidently got duplication after rebase. Deleted method without parameter now. Could you please check that it didn't change the behavior?
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.
It changes because PrimitivesModelProvider
isn't used anymore. By default I want to keep an ability to set different providers by default, because I usually want to use shallower value set for it. Right now this change is only about PrimitivesModelProvider, but in the future I'd want to add some providers that are used for generating flat parameters and aren't used for recursive.
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.
Please, see comments
f4405fa
to
36c749e
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.
Looks good but let's discuss changes about defaultModelProviders
* Add flattening to Combined * Change default value for RecursiveModelProvider.branchingLimit
Description
Now,
ArrayModelProvider
generates more interesting tests:Fixes #738
Type of Change
How Has This Been Tested?
Manual Scenario
Generate tests for methods from
ArrayExample
class (using only fuzzer):For each of them, meaningful tests are generated that cover both true/false outputs.
Checklist (remove irrelevant options):
This is the author self-check list