-
Notifications
You must be signed in to change notification settings - Fork 6
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
The PhET-iO template crashes #49
Comments
@ariel-phet This is the second PhET-iO problem found in the recent Molarity 1.4 release. (https://github.com/phetsims/phet-io-wrappers/issues/135 was the first.) So it feels like we have a quality control problem wrt PhET-iO. @samreid @zepumph Any thoughts on what the problem is? Are developers not spending enough time testing? Are QA test instruction insufficient? |
I suspect the main problem is that it has been a long time since we have had a PhET-iO RC test, and PhET-iO has many new features. We have been augmenting the instructions in phetsims/qa#19 and should continue to do so. I have notes in https://github.com/phetsims/phet-io-wrappers/issues/147 and phetsims/qa#19 (comment) to update the PhET-iO testing documentation. |
This is particularly hard to test because of https://github.com/phetsims/phet-io-wrappers/issues/145, I will try to get that to work first, then come back here. |
This was a bit of a confusing chain to crack, let me document the steps I took to reproduce/diagnose and the problems along the way. First I noticed that the assertion that triggers in the debug version is less than helpful, since all the Type names are garbled. I think it would be good to add the Type name to that assertion error if we able (not sure how to manage that). Next I found that when trying to test locally, recent changes moving the index wrapper caused a bug that I fixed in https://github.com/phetsims/phet-io-wrappers/issues/145. Next, I was successfully able to checkout 1.4 shas and test locally, finding that the same error occurs in requirejs mode (i.e. this assertion fails when just launching the standalone version of the sim in requirejs). The error was probably from taking a sun sha in the middle of the implementation of phetsims/sun#340. This means that this issue is not systemic, and will just need a simply patch in sun (this commit will not need to be on master, just for molarity 1.4). On top of this, we should add https://github.com/phetsims/phet-io-wrappers/commit/f94aa36747ee94058ca7a29ae98c5470f2524017 so that we can easily test that the patch works as expects. Honestly I'm very surprised that this made it through testing, and perhaps we need a bit more strict testing (i.e. using the debug flag in our wrapper suite). That said, it is also pretty cool that such an assertion failed, yet all of our phet-io infrastructure was fault tolerant to it. @pixelzoom since all of these changes are outside of molarity, I will plan to make the commit, and then pass it over to you for the maintenance release, unless you would like me to do the release. |
Thanks @zepumph. I can do the maintenance release. Let me know when you're done. |
I have fixed this problem for the molarity 1.4 branch. I did so by:
Still to do:
Testing steps to make sure that the patch fixed things:
@pixelzoom over to you to create the maintenance release. Please let me know how/if the steps above were sub-optimal and if so please provide feedback to how I can do this better next time. I know that you prefer doing maintenance releases for your sims, which is very understandable and reasonable. For this particular situation I feel like I'm leaving you in a half baked state. Let me know if I can help any more. |
@zepumph I'm in the dark about this issue, and it looks like things are partially patched. So how about if you complete and test all patches that are required for the molarity 1.4 branch and its dependencies. When it's ready to go, assign back to me, and I'll publish the RC, create the GitHub issue, etc. |
Marking high so it doesn't fall off my radar. It shouldn't take too long. |
Here are the steps I just did:
@pixelzoom over to you for final maintenance release, checkout-shas should now point you in the correct location, but just to make sure, it would be good to confirm that you are on the |
(note) I apologize for the potentially superfluous lists of steps, but I'm not yet confident enough in the build process to proceed in a simpler manner. |
@zepumph said:
FYI, according to @jonathanolson, we should be using |
Thank you! good to know. I can't seem to keep up with all the good code. |
I did a local build:
The brand=phet version tested OK. For the brand=phet-io version, here's @zepumph's test instructions with clarification:
Navigate to molarity/build/phet-io, click the "Simulation" link, replace the filename in the URL with "molarity_all_phet-io_debug.html". So my URL is
Navigate to molarity/build/phet-io, copy the HTML that is shown in the black box in the API subsection, paste it into a new html file, run that html file. The above tests passed. @zepumph confirmed that I did this correctly. Ready to begin RC testing. |
This is to be verified in 1.4.2.-rc.1 testing, see phetsims/qa#178. @KatieWoe you can use the test procedure that I've described in #49 (comment). |
Just tested on Win 10 chrome, and it seems to work. Will test on other platforms before verifying in phetsims/qa#178 |
Confirmed 1.4.2-rc.1 |
Status? This has been high-priority since 8/17. |
According to phetsims/qa#178 (comment) we deployed 1.4.2. @pixelzoom can this issue be closed? |
Navigate to https://phet-io.colorado.edu/sims/molarity/1.4.1/ and download the "basic example of a functional wrapper" to a local file, then launch it in the browser. I obtain this error and the sim doesn't launch:
When I changed
debug: true,
todebug: false,
, then the simulation launched. Discovered during phetsims/qa#19@zepumph are you available to investigate this? I don't yet know if the problem is specific to Molarity or general. I'll mark as high priority and meeting in case it is general.
The text was updated successfully, but these errors were encountered: