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

CT designed API changes detected #386

Closed
KatieWoe opened this issue Jun 10, 2021 · 10 comments
Closed

CT designed API changes detected #386

KatieWoe opened this issue Jun 10, 2021 · 10 comments

Comments

@KatieWoe
Copy link
Contributor

gravity-and-orbits : phet-io-api-compatibility : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623335973957/gravity-and-orbits/gravity-and-orbits_en.html?continuousTest=%7B%22test%22%3A%5B%22gravity-and-orbits%22%2C%22phet-io-api-compatibility%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1623335973957%22%2C%22timestamp%22%3A1623340144390%7D&ea&brand=phet-io&phetioStandalone&phetioCompareAPI
Query: ea&brand=phet-io&phetioStandalone&phetioCompareAPI
Uncaught Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API:

gravityAndOrbits.general.model.simInfo._data.initialState differs. Expected: {"dataStreamVersion":"1.0.0","phetioCommandProcessorProtocol":"1.0.0","repoName":"gravity-and-orbits","screenPropertyValue":"gravityAndOrbits.homeScreen","screens":[{"name":"Home","phetioID":"gravityAndOrbits.homeScreen"},{"name":"Model","phetioID":"gravityAndOrbits.modelScreen"},{"name":"To Scale","phetioID":"gravityAndOrbits.toScaleScreen"}],"simName":"Gravity and Orbits","simVersion":"1.5.0-dev.4","wrapperMetadata":null}, actual: {"simName":"Gravity and Orbits","screens":[{"name":"Home","phetioID":"gravityAndOrbits.homeScreen"},{"name":"Model","phetioID":"gravityAndOrbits.modelScreen"},{"name":"To Scale","phetioID":"gravityAndOrbits.toScaleScreen"}],"simVersion":"1.5.0-dev.6","repoName":"gravity-and-orbits","screenPropertyValue":"gravityAndOrbits.homeScreen","wrapperMetadata":null,"dataStreamVersion":"1.0.0","phetioCommandProcessorProtocol":"1.0.0"}
Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API:

gravityAndOrbits.general.model.simInfo._data.initialState differs. Expected: {"dataStreamVersion":"1.0.0","phetioCommandProcessorProtocol":"1.0.0","repoName":"gravity-and-orbits","screenPropertyValue":"gravityAndOrbits.homeScreen","screens":[{"name":"Home","phetioID":"gravityAndOrbits.homeScreen"},{"name":"Model","phetioID":"gravityAndOrbits.modelScreen"},{"name":"To Scale","phetioID":"gravityAndOrbits.toScaleScreen"}],"simName":"Gravity and Orbits","simVersion":"1.5.0-dev.4","wrapperMetadata":null}, actual: {"simName":"Gravity and Orbits","screens":[{"name":"Home","phetioID":"gravityAndOrbits.homeScreen"},{"name":"Model","phetioID":"gravityAndOrbits.modelScreen"},{"name":"To Scale","phetioID":"gravityAndOrbits.toScaleScreen"}],"simVersion":"1.5.0-dev.6","repoName":"gravity-and-orbits","screenPropertyValue":"gravityAndOrbits.homeScreen","wrapperMetadata":null,"dataStreamVersion":"1.0.0","phetioCommandProcessorProtocol":"1.0.0"}
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623335973957/assert/js/assert.js:25:13)
at XMLHttpRequest.<anonymous> (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623335973957/phet-io/js/phetioEngine.js:321:23)
id: Bayes Chrome
Snapshot from 6/10/2021, 8:39:33 AM
@samreid
Copy link
Member

samreid commented Jun 11, 2021

The only content difference between the states is expected: "simVersion": "1.5.0-dev.4", actual "simVersion": "1.5.0-dev.6",. I don't recall why we put the simVersion in the API file, but we wouldn't want to have to update that part every time we bump dev versions. I'll see if we can remove it.

@samreid
Copy link
Member

samreid commented Jun 11, 2021

Removed in the commits, and I regenerated the API files. Reassigning to @zepumph for a spot check review when convenient.

@samreid
Copy link
Member

samreid commented Jun 11, 2021

Well, https://github.com/phetsims/studio/blob/1c4ebb8386ad66bd8f78d24eed499bca6a4aa62b/js/customizeWrapperTemplateForAction.js#L272-L276 uses the version from simInfo, so I've added it back in. Not sure the best way to proceed, some options:

  • Just leave the version in the API file, and regenerate it when necessary.
  • Omit the version when generating the API file.
  • Another pipeline for getting the version (not through siminfo).

@zepumph what do you recommend?

@samreid
Copy link
Member

samreid commented Jun 29, 2021

In #390 (comment) @pixelzoom raised some questions which are pertinent to this issue. @pixelzoom said:

Reopening.

@samreid said:

The reason the version appears in the API file is discussed in #386.

That issue doesn't tell me much. Is "version" the API version or the sim version? It was originally named simVersion. But in phet-io/api/natural-selection.json, I see:

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

... which does not match the sim version at all, so leads me to believe that it's an API version. But GAO had to be updated to match the sim version.

So to help me understand "version" and this problem, I have many questions:

  • What are the semantics of "version" in the API file?
  • When does "version" need to be updated in the API file?
  • How is "version" updated in the API? Manually? Build tools?...
  • Why did this problem occur for GAO?
  • Why did it occur around the time of publishing the RC?
  • Why did you need to manually update version to 1.6.0-dev.0? Should that have been done by build tools?
  • If version matches package.json for GAO, why is that not the case for Natural Selection?

@samreid
Copy link
Member

samreid commented Jun 29, 2021

Is "version" the API version or the sim version?

The version under discussion in this issue is the sim version. For instance, master has this line for natural selection: "simVersion": "1.4.0-dev.1",

But in phet-io/api/natural-selection.json, I see:

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

That refers to the version of the entire API file, and is not under discussion in this issue except for clarification.

What are the semantics of "version" in the API file?

It is the sim version at the time of the API file generation, and is determined by SimInfo.js

When does "version" need to be updated in the API file?

As described in #386 (comment), we are trying to see if we can omit this from the API file. For now, I believe we should update it eagerly when the sim version changes in package.json.

How is "version" updated in the API? Manually? Build tools?...

For now it is manual. Let's determine whether we can eliminate it before we add tooling around it.

Why did this problem occur for GAO? Why did it occur around the time of publishing the RC?

Creating the release branch bumped the version number in master from 1.5.0-dev.8 to 1.6.0-dev.0. Since this is in the initial state, which is tracked, CT identified it as a state change that wasn't in the API file.

Why did you need to manually update version to 1.6.0-dev.0? Should that have been done by build tools?

We don't have tooling support for this at the moment, let's confer with @zepumph to see if we actually want this data in the state object or if it should be done another way before adding tooling.

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

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?

@zepumph
Copy link
Member

zepumph commented Jul 1, 2021

Over in #387 it seemed like we suppressed a bunch of information, returning null when Tandem.API_GENERATION was true. Can we do that here for simVersion as well?

@samreid
Copy link
Member

samreid commented Jul 2, 2021

Great idea, I applied the same strategy and tested that it worked well in studio. @zepumph can you please review? Close this issue is all is well.

@samreid samreid assigned zepumph and unassigned samreid Jul 2, 2021
@zepumph
Copy link
Member

zepumph commented Jul 7, 2021

It is considered "API_GENERATION" when during build we create the API json file. Is that important to have? Does studio need this? Or is it only important in the state of the living sim?

@zepumph zepumph assigned samreid and unassigned zepumph Jul 7, 2021
@samreid
Copy link
Member

samreid commented Jul 8, 2021

It is considered "API_GENERATION" when during build we create the API json file. Is that important to have?

The simVersion should not appear in the built API JSON file--the version should be obtained elsewhere.

Does studio need this? Or is it only important in the state of the living sim?

This seems useful in data streams and #386 (comment)

@samreid samreid removed their assignment Jul 8, 2021
@zepumph
Copy link
Member

zepumph commented Jul 11, 2021

I'm happy you feel that way. It sure is easier. Since you are the studio-customization-state expert, I'm glad to have gotten your opinion.

I mentioned it because this looked weird to me at first, and no longer does

https://github.com/phetsims/phet-io/blob/718e451e56a8184c6e63ac64903feca3c6580adb/api/natural-selection.json#L290

Ready to close for me, please reopen if you disagree.

@zepumph zepumph closed this as completed Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants