Listener order should be scrambled on fuzz #215
Assignees
Labels
Comments
|
Discussed in today's dev meeting. It was mentioned that though it is a bit of a code smell to have listener order matter. For example if you create an emitter and immediately add a listener to it such that that listener would HAVE to be before any others added. That said @jonathanolson thought that this didn't necessarily warrant a large investigation into an automatic fix for this. |
|
It seems inappropriate to combine listener order scrambling with input fuzzing. Let's introduce a separate query parameter |
This was referenced Oct 15, 2019
|
I shuffled listener order in TinyEmitter, which powers Emitter and Property (though not yet ObservableArray, see #270 ) via this patch: Index: js/TinyEmitter.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/TinyEmitter.js (revision 89d19e65ac30c9d39bc56f1d92b89ca5afe35c86)
+++ js/TinyEmitter.js (date 1571116506547)
@@ -49,6 +49,8 @@
emit() {
assert && assert( !this.isDisposed, 'should not be called if disposed' );
+ this.listeners = _.shuffle( this.listeners );
+
// Notify wired-up listeners, if any
if ( this.listeners.length > 0 ) {
this.activeListenersStack.push( this.listeners );I ran local aqua fuzz on phet brand and saw errors for the following sims:
Note: some of these errors were pre-existing and unrelated to the listener order. |
samreid
added a commit
to phetsims/phet-info
that referenced
this issue
Oct 15, 2019
samreid
added a commit
to phetsims/chipper
that referenced
this issue
Oct 15, 2019
samreid
added a commit
that referenced
this issue
Oct 15, 2019
|
The above commit adds The behavior is also buried behind I also added a note to the CRC for checking runtime behavior with this flag enabled. @zepumph would you like to take a look before we run this past @pixelzoom or a dev team meeting? |
|
Also, is there a QA checklist where this would be documented as well? |
|
My guess is that the above is only a piece of the errors that this may have caused. Though it is a code smell, I could foresee a case in which creating a Property and immediately linking to it feels so linked, that assertions aren't created making sure that that listener's logic happened before other logic, but it would still change something in the sim. |
|
I agree, we would need to manually test simulation behavior to see how the listener order impacts it. I would expect some but not all of it to be caught in automated testing. |
|
I really like this query parameter, and I think it is a step in the right direction. When creating this issue, it was clear that it is definitely PREFERRED to have all of these errors fixed. That said, I'm not sure what the priority of it would be. For example, in gravity force lab (having just worked on it and encountered this listener dependency order before), it is not an obvious fix, and may need serious model refactoring to resolve. |
|
I also really like its role in the CRC. That will help this issue not proliferate. |
|
Perhaps it would be good to try to fix one or two before passing this off to another for further discussion. That would inform our next steps here. For example, if all of these issues were <10 mintues each, we would likely try to fix them up front. Perhaps a dev meeting chip away issue is needed. That said this really feels like an order of magnitude more costly than we will want to investigate. We will know more after doing a few conversions. |
samreid
added a commit
to phetsims/wave-interference
that referenced
this issue
Oct 15, 2019
|
I fixed the waves-intro fuzz problem in the preceding commit. It took around 10 minutes. This seemed to be a good use of time, because it wasn't evident why that bug wasn't being triggered in the first place. So this fix seems to have made the code more tolerant and refactor-proof. I made a cursory look through the rest of the sim and didn't see other errors. I don't expect all problems will be this easy to solve, this is probably a best case scenario. Assigning back to @zepumph but also flagging for dev meeting because it will likely be ready for discussion by then. |
|
Also I can use this with snapshot comparison, which is already good at determining whether sims are purely deterministic based on the seed. We can also verify that changing listener order results in no visual differences in sims. |
|
That sounds excellent. |
For CT, yes, it's better than what we're doing now. Just keep in mind that this will verify that the sim works with 2 specific listener orders, not that there are no dependencies on listener order. So I think there's still a need for
This is still not guaranteed to identify all order dependencies, but it's lightyears better than only testing that the listener order works in reverse. |
marlitas
added a commit
that referenced
this issue
Apr 3, 2023
marlitas
added a commit
to phetsims/chipper
that referenced
this issue
Apr 3, 2023
samreid
added a commit
to phetsims/perennial
that referenced
this issue
Apr 3, 2023
samreid
added a commit
to phetsims/phetmarks
that referenced
this issue
Apr 3, 2023
samreid
added a commit
that referenced
this issue
Apr 3, 2023
samreid
added a commit
to phetsims/phet-info
that referenced
this issue
Apr 3, 2023
|
@marlitas and I made good progress, summarizing changes and next steps:
@samreid will:
|
zepumph
added a commit
that referenced
this issue
Apr 4, 2023
|
Null checking above for failing CT when we don't have |
samreid
added a commit
that referenced
this issue
Apr 5, 2023
|
OK I added a test harness to manually check the behavior with @pixelzoom would you like to review this issue? Particularly the changes described in #215 (comment) |
|
Looks good. But the info you put in #215 (comment) (description of the values) belongs in the documentation for |
samreid
added a commit
to phetsims/chipper
that referenced
this issue
Apr 20, 2023
|
I updated the JSDoc, ready for review. |
|
👍🏻 JSdoc looks good. |
|
Thanks, it seems this issue is ready to close. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Today in phet-io/dev meeting we decided that axon listener order should not be guaranteed. One nice way that someone thought up (sorry I forgot who) to test this would be to shuffle the order of listeners while fuzzing.
The text was updated successfully, but these errors were encountered: