Opt out of modal features during fuzz testing

[zepumph]

I have a couple of assertion errors in BAN that are incredibly hard to get to, and when I look at fuzzing, much of its time is in modal features (phet menu, keyboard/preferences dialog, and sim specific dialogs). It would be great to be able to turn this off for some testing.

[zepumph]

Added above. I will see if anyone wants to review it, but for me in BAN it is working really nicely!

Questions for the reviewer:

• Should we add this to CT at all? (I don't think we need to)
• Are there any items that aren't Popupable that need this behavior too
• I know that probably an obvious question would be, "what about if I DO want to test my sim-specific modal dialogs). I decided purposefully that those are also distractions for this case, which is centered around the primary use cases of the sim.

Let me know what you think!

UPDATE: Perhaps the home button on the nav bar could be disabled too, as that will increase the time on sim specific content.

[samreid]

In review @zepumph and I discussed that we want to change the query parameter to a probabilistic one so that we can tune the amount of time spent in dialogs. Right now fuzzing spends way too much time in modals. But we don't want to always have to avoid them completely. Probably something like ?showModalProbability=1, but you could set it to 0 if you don't want any modals.

Second, fuzzing does not do a very good job of visiting the state space, and we would like to reconsider https://github.com/phetsims/axon/issues/383

We also heard an unexpected behavior in Center and Variability that the first time clicking on a disabled modal it plays one sound and the second time it plays another sound.

@zepumph also comments that we don't really need to always fuzz the common code components, such as the home screen, the navigation bar, the phet menu, etc. So we could just fuzz that once (per CT column) and not across all our sims.

We also would like to move the guard upstream into show.

[zepumph]

I totally understand the above comment, but I don't actually think it would be very useful for me. I stopped using ?disableModals almost immediately after writing it in August. I just reverted it, and I think that this issue is best closed. We could work on better features for fuzz testing in another issue if we feel it is worth while.

[zepumph]

Today @marlitas mentioned that she was missing ?disableModals I'm sorry! I should have asked before removing it. Reopening.

[marlitas]

@zepumph asked me to add my goals and usages for?disableModals:

• My main usage is when trying to quickly replicate fuzz errors that may come up on CT, that I know do not involve any modals in the sim. This is quite frequent.
• I will also use it to quickly test changes that affect main interactions in a play area during fuzzing as well to quickly confirm and ensure that the fuzzing is actually fully interacting with the main play area rather than landing in a modal such as preferences all the time.
• I loved how straight forward the flag was to use, and had no additional desires to the functionality that was already there.

[jbphet]

I second @marlita's comments above. As a data point, she and I were doing some fuzz testing of phet-io state in the mean-share-and-balance sim, and we noticed that the sim was spending a significant percentage of its time on the preferences dialog. I suppose this isn't surprising since the dialog is huge, so it soaks up a lot of the fuzz events, but it seems like a problem for testing other aspects of the sim. Being able to disable it (and other modal dialogs) saves us time in tracking down bugs with other aspects of the sims.

[zepumph]

Thanks! That is really helpful! I reverted the removal above, and added it to phetmarks. I'll spent another 15 minutes thinking about the other pieces of this issue before closing.

[zepumph]

After reverting, I see a better way to do it is to prevent show(), not just the isShowingProperty listener. Fixed above.

@samreid, can you please review, and then also apply this patch and see if you like it better? We discussed probabilities, and so it may be a more long-term solution.

To be clear, there are 3 potential paths forward:

1. What is on main right now.
2. The patch below (I believe it to be commit-ready).
3. A new idea that you have to report.

```diff Subject: [PATCH] better shutoff for popupable, https://github.com/phetsims/sun/issues/855 --- Index: phetmarks/js/phetmarks.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phetmarks/js/phetmarks.ts b/phetmarks/js/phetmarks.ts --- a/phetmarks/js/phetmarks.ts (revision 89bcee53c6e6851699f462d049c1a3602087fdc0) +++ b/phetmarks/js/phetmarks.ts (date 1705620058064) @@ -121,7 +121,7 @@ { value: 'interactiveHighlightsInitiallyEnabled', text: 'Interactive Highlights on by default' }, { value: 'preferencesStorage', text: 'Load Preferences from localStorage.' }, { value: 'webgl', text: 'WebGL', type: 'boolean' }, - { value: 'disableModals', text: 'Disable Modals' }, + { value: 'disableModalsProbability=1', text: 'Disable Modals' }, { value: 'listenerOrder', text: 'Alter listener order', Index: sun/js/Popupable.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/Popupable.ts b/sun/js/Popupable.ts --- a/sun/js/Popupable.ts (revision 67101f5164dfff66a9fb8d651e5f50afe8a37715) +++ b/sun/js/Popupable.ts (date 1705622154321) @@ -19,6 +19,7 @@ import PickOptional from '../../phet-core/js/types/PickOptional.js'; import { FocusManager, Node, NodeOptions } from '../../scenery/js/imports.js'; import sun from './sun.js'; +import dotRandom from '../../dot/js/dotRandom.js'; type SelfOptions = { @@ -41,7 +42,7 @@ // When true, no modal show/hide feature will be supported. This is a way of opting out of the Popupable feature // altogether for this runtime. - disableModals?: boolean; + disableModalsProbability?: number; }; type ParentOptions = PickOptional; export type PopupableOptions = SelfOptions & ParentOptions; @@ -63,7 +64,7 @@ // The node provided to showPopup, with the transform applied public readonly popupParent: Node; - private readonly disableModals: boolean; + private readonly disableModalsProbability: number; private readonly isModal: boolean; // Whether the popup is being shown @@ -92,7 +93,7 @@ layoutBounds: null, focusOnShowNode: null, focusOnHideNode: null, - disableModals: _.get( window, 'phet.chipper.queryParameters.disableModals' ) || false + disableModalsProbability: _.get( window, 'phet.chipper.queryParameters.disableModalsProbability' ) || 0 }, providedOptions ); // see https://github.com/phetsims/joist/issues/293 @@ -100,7 +101,7 @@ this.layoutBounds = options.layoutBounds; this._focusOnShowNode = options.focusOnShowNode; - this.disableModals = options.disableModals; + this.disableModalsProbability = options.disableModalsProbability; this.isModal = options.isModal; this._focusOnHideNode = options.focusOnHideNode; this._nodeToFocusOnHide = null; @@ -134,7 +135,7 @@ // Provide a chance of not showing, see disableModals protected shouldShowPopup(): boolean { - const optOut = this.isModal && this.disableModals; + const optOut = this.isModal && dotRandom.nextDouble() < this.disableModalsProbability; return !optOut; } Index: chipper/js/initialize-globals.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/initialize-globals.js b/chipper/js/initialize-globals.js --- a/chipper/js/initialize-globals.js (revision 64e6d1499be7dd22585497987558204a0f168ca6) +++ b/chipper/js/initialize-globals.js (date 1705622154321) @@ -152,11 +152,18 @@ /** - * sets all modal features of the sim as disabled. This is a development-only parameter that can be useful in + * Probability of preventing a given modal trying to show. This is a development-only parameter that can be useful in * combination with fuzz testing. This was created to limit the amount of time fuzz testing spends on unimportant * features of the sim like the PhET Menu, Keyboard Help, and Preferences popups. + * 0 - open modals every time. + * 0.5 - open modals 50% of the time. + * 1 - open modals never. */ - disableModals: { type: 'flag' }, + disableModalsProbability: { + type: 'number', + defaultValue: 0, + isValidValue: value => value >= 0 && value <= 1 + }, /** * enables assertions

[samreid]

The commits and the runtime behavior look good. The patch is a good implementation of the probability idea, but after reflection, maybe it is best to keep this simpler as a boolean. Sorry for the extra effort on the probability. Sound OK?

[zepumph]

Sorry for the extra effort on the probability

nonsense. It was just 5 minutes and a fun exploration.

Seems very reasonable! And I think we are ready to close. Thanks.