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

Add Property fuzzing #383

Closed
samreid opened this issue Mar 31, 2022 · 5 comments
Closed

Add Property fuzzing #383

samreid opened this issue Mar 31, 2022 · 5 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Mar 31, 2022

We recently discussed that many options aren't exercised during fuzz testing, see phetsims/aqua#136. We can improve fuzzing coverage by fuzzing the Property values directly, without requiring mouse/touch input to trigger them. For instance, we could automatically switch between discrete valid values like so:

    // Near the end of the Property constructor 
    if ( providedOptions && providedOptions.validValues && this.isSettable() ) {
      stepTimer.setInterval( () => {
        this.value = _.sample( providedOptions.validValues )!;
      }, 200 );
    }

A similar approach could be applied for isSettable NumberProperties that specify a Range, and for BooleanProperty values.

Note the guard on isSettable(), this prevents fuzzing DerivedProperties. It's not clear that isSettable() means the Property can be set to any valid value at any time in the sim without creating an inconsistent state, but many Properties (such as combo boxes, or dialog options) can be set at any time. Would we need additional metadata to Property to indicate whether it is safe to fuzz values at any time? Do we want to proceed with this style of testing?

@pixelzoom can you please review and comment?

@pixelzoom
Copy link
Contributor

It's a nice thought, but @samreid already identified the main problem:

It's not clear that isSettable() means the Property can be set to any valid value at any time in the sim without creating an inconsistent state ...

Game state definitely does not take any value at any time; there's a sequence that makes sense of the game's "state machine".

Even if individual Properties don't object, by fuzzing all Properties that have validValues, you'll likely be putting the sim in states that it would never reach in practice, and which it may not be designed for.

Would we need additional metadata to Property to indicate whether it is safe to fuzz values at any time?

Since I don't think this approach is really generally viable, it would be preferable to opt in than to opt out. For example, for the Property associated with the combo box in Geometric Optics:

  this.opticalObjectChoiceProperty = new EnumerationProperty( options.opticalObjectChoices[ 0 ], {
      fuzzTest: true,
      validValues: options.opticalObjectChoices,
      tandem: options.tandem.createTandem( 'opticalObjectChoiceProperty' )
    } );

One interval (200 ms was used in @samreid's example code above) is unlikely to be a good fit for all Properties. For my example in Geometric Optics, something like 3-5 seconds would probably be a better fit -- we want to significantly test each scene before moving to another scene.

Do we want to proceed with this style of testing?

My gut feeling is no. Besides the problems noted above, this also doesn't address exercising the UI.

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Mar 31, 2022
@samreid
Copy link
Member Author

samreid commented Aug 3, 2022

I tried adding

    // Near the end of the Property constructor 
    if ( providedOptions && providedOptions.validValues && this.isSettable() ) {
      stepTimer.setInterval( () => {
        this.value = _.sample( providedOptions.validValues )!;
      }, 200 );
    }

and testing Geometric Optics, and it seemed like it was testing reasonable things. But it didn't seem to add a lot of value beyond input fuzzing, and had incomplete coverage. For example, booleans weren't toggled on and off. Also, having to visit sites and opt-in to this level of testing seems like additional burden. Perhaps we should close this issue until a stronger need arises.

@samreid
Copy link
Member Author

samreid commented Aug 25, 2023

If we implement this, we could follow a strategy like KeyboardFuzzer, and get control over the amount of Properties changed in one frame.

@samreid
Copy link
Member Author

samreid commented Aug 30, 2023

@zepumph and I discussed that adding Property-based fuzzing on an opt-in basis could help us get much better coverage in our automated testing.

@samreid
Copy link
Member Author

samreid commented Sep 1, 2023

I'm having second thoughts about this issue:

  • In Center and Variability, one of the main controls that should be tested in automated testing is the "maximum number of kicks" combo box which is in the preferences dialog. However, changing that value clears all the data points, so it shouldn't be exercised too often.
  • Defining a language for "how often should this Property" be changed seems like it would add a lot of complexity.
  • Would we want to auto-trigger Emitters too?
  • A developer can already tap into ?fuzz in a sim-specific way like so:
if (assert && queryParameters.fuzz){
  setInterval(myProperty.value = sampleRandom(myProperty.validValues);
}

That is, we do not need a framework or axon options to make sim-specific fuzzing work in this way. However, I do not think anyone would want to see that additional bloat as a long-term addition to their code. Nor would we want to see and maintain property options like fuzzMe: FuzzStrategy.changeValueEveryNSeconds(10, validValues).

So I recommend this issue be closed again. @zepumph please reopen for any reason.

@samreid samreid closed this as completed Sep 1, 2023
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

No branches or pull requests

2 participants