search

phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

Add phetioDesigned:true #294

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

For phetsims/qa#662

In https://github.com/phetsims/gravity-and-orbits/issues/386#issuecomment-870994068, I asked:

If version matches package.json for GAO, why is that not the case for Natural Selection?

@samreid replied:

Gravity and Orbits specifies phetioDesigned: true for the entire simulation (in main), but Natural Selection does not. Perhaps this would be a good time to enable phetioDesigned tracking for natural selection?

pixelzoom commented 3 years ago

@samreid can you refresh my memory on what phetioDesigned is, and how to set it?

@amanda-phet is this something that we want to do for Natural Selection?

And do we want to do this in master, the 1.4 branch, both? @samreid what do you recommend?

samreid commented 3 years ago

I updated the documentation in https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md#post-publication-steps to clarify some of the questions you raised. Please review that, and refer to gravity-and-orbits-main.js as an example if necessary. Please let me know if there are more questions or problems.

pixelzoom commented 3 years ago

From https://github.com/phetsims/natural-selection/issues/294#issuecomment-872597103:

And do we want to do this in master, the 1.4 branch, both? @samreid what do you recommend?

samreid commented 3 years ago

I recommend changing this in master.

pixelzoom commented 3 years ago

@samreid can you refresh my memory on what phetioDesigned is, and how to set it?

@samreid Thanks for the updated "post-publication" documentation, that will help as a reminder to set it up. But @amanda-phet and I are still unclear on what phetioDesigned is. Can you please summarize?

pixelzoom commented 3 years ago

For Natural Selection, I added phetioDesigned:true to Sim options, verified that package.json contains "compareDesignedAPIChanges": true, and ran grunt generate-phet-io-api.

In natural-selection/package.json:

  "version": "1.5.0-dev.0",

In phet-io/api/natural-selection.json:

  "version": {
    "major": 1,
    "minor": 0
  }

I see nothing about having to manually update version in https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md#post-publication-steps. But I seem to recall that this version mismatch caused a problem (in GAO?) so I have not commited and pushed my changes. @sam if I push with this mismatch, is this going to be a problem?

pixelzoom commented 3 years ago

@samreid I believe that the order is wrong for the steps that you added to https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md#post-publication-steps. phetioDesigned: true needs to be done BEFORE generating the API json file. Please confirm that steps look correct after the above commits.

pixelzoom commented 3 years ago

Looking at the API files in phet-io/api/, none of them appear to have "version" that matches package.json. There was discussion about this in https://github.com/phetsims/gravity-and-orbits/issues/386#issuecomment-872321694, but I don't understand what's going on here.

pixelzoom commented 3 years ago

I went ahead and pushed my changes for the 2 commits above. I have no idea how to test this, or whether the version mismatch will be a problem, so I'll keep an eye on CT.

@samreid please review, and:

pixelzoom commented 3 years ago

My understanding is that this does not impact the 1.4 release, because it's only related to CT testing of master. If that's not correct, then someone please add a "block-sim-publication" label asap.

samreid commented 3 years ago

Looking at the API files in phet-io/api/, none of them appear to have "version" that matches package.json. There was discussion about this in https://github.com/phetsims/gravity-and-orbits/issues/386#issuecomment-872321694, but I don't understand what's going on here.

The data type of simVersion is now NullableIO(StringIO) It is output as null in the API file, and string when running a live sim. We nulled it out because we didn't want the version number to be part of the API and require API file regeneration when changed.

Verify that I did this correctly.

Looks good, thanks!

Describe phetioDesigned for designers, so that they know what it does, and when/where it's appropriate to add it.

Marking a subtree as phetioDesigned: true means its design has been fully designed and implemented, and it triggers errors on CT if the implementation deviates.

My understanding is that this does not impact the 1.4 release, because it's only related to CT testing of master. If that's not correct, then someone please add a "block-sim-publication" label asap.

This does not block publication.

pixelzoom commented 3 years ago

The data type of simVersion is now NullableIO(StringIO) ....

What is simVersion? We're talking about version, which I understood from https://github.com/phetsims/gravity-and-orbits/issues/386 to be the same as what's in package.json.

samreid commented 3 years ago

I thought this was clarified in https://github.com/phetsims/gravity-and-orbits/issues/386#issuecomment-870994068. To recap, using master and natural selection as an example:

// this is the version of the data stream, in case we change data stream semantics/structure
"dataStreamVersion": "1.0.0", 
// placeholder for the simulation version.  Not checked in with the API files.  But is exposed during
// sim runtime so the client can get the version that way.
"simVersion": null, 
// Top level version of the API file structure
"version": { 
    "major": 1,
    "minor": 0
  }

But I seem to recall that this version mismatch caused a problem (in GAO?) so I have not commited and pushed my changes.

We resolved the mismatch by nulling out the simVersion. There was never a mismatch problem with the 1.0 "version".

pixelzoom commented 3 years ago

@samreid said:

Marking a subtree as phetioDesigned: true means its design has been fully designed and implemented, and it triggers errors on CT if the implementation deviates.

... and after publication, the general recommendation is to put phetioDesigned: true on the top of the Studio tree, so that the complete tree will be checked for deviations.

@amanda-phet if this answers your question, feel free to close. If you need more explanation, please ask @samreid directly.

pixelzoom commented 3 years ago

https://github.com/phetsims/natural-selection/issues/294#issuecomment-875132933:

... Please confirm that steps look correct after the above commits.

I don't see any indication that @samreid reviewed this, so assigning to him.

samreid commented 3 years ago

I reviewed the changes to the phet-io-instrumentation-guide and they look good. Anything else for this issue?

pixelzoom commented 3 years ago

@samreid asked:

... Anything else for this issue?

That's all of the questions I had for you, thanks. @amanda-phet will contact you if she needs more clarification about phetioDesigned.

pixelzoom commented 3 years ago

Oh, I see that @amanda-phet gave a 👍🏻 .

Closing, since there's nothing to be done or tested for the 1.4 release branch.

pixelzoom commented 3 years ago

Reopening.

I've followed all of the steps in https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md#post-publication-steps for setting up phetioDesigned: true. Everything is commited and pushed. When I run NS in Studio with phetioCompareAPI, it's failing with:

Assertion failed: Designed API changes detected, please roll them back or revise the reference API: .... lots of json .... at window.assertions.assertFunction (assert.js:25) at XMLHttpRequest. (phetioEngine.js:312)

The json output part of the console output is way too long to be examined manually.

I've tried re-running grunt generate-phet-io-api, and no changes are generated, so there's nothing for me to diff.

@samreid @zepumph suggestions on how to proceed?

pixelzoom commented 3 years ago

Moving the phetioCompareAPI to a new issue, https://github.com/phetsims/natural-selection/issues/301. Re-closing.