Diff wrapper is broken. #271
Labels
Comments
4 tasks
|
Discussed at 4/22/21 phet-io meeting. @kathy-phet would @zepumph to identify a fix, then evaluate whether to include in the 1.3 release. |
|
Looks like a potential CORS problem trying to access phet-io.colorado.edu from phet-dev, I will take a look after meetings. |
|
@mattpen helped me discover that this was in fact a 401 authentication problem under the thinly veiled guise of a CORS issue. When our phet-io server doesn't authenticate, it also doesn't include the needed CORS header that it normally does. For example, when MP was pinged the phet-io json api with credentials: But when we did so without credentials: no CORS header because we need to authenticate. Looks like the Chrome api cares about CORS before it cares about authentication, so that is why we would get this error, when really the request was a 401 unauthorized. Looks like the last time we messed with password protection was early 2020, so likely most recent sims are password protected. We should build into the diff wrapper a way to authenticate the requested resource. I'll make a new issue in phet-io-wrappers to figure this out. UPDATE: also tagging https://github.com/phetsims/phet-io-website/issues/115 since it is the most recent I have thought about password protection in phet-io deploys. |
|
It sounds like there's also some room for improvement of error handling in the Diff wrapper. A problem like that should definitely not result in something that looks like success and "no API changes". |
|
Agreed! |
|
This issue is dependent on resolution of https://github.com/phetsims/phet-io-wrappers/issues/405. In https://github.com/phetsims/phet-io-wrappers/issues/405#issuecomment-826008418, @zepumph described that issue as blocking:
In 4/23/21 Slack, @zepumph described that issue as NOT blocking:
@zepumph @kathy-phet please clarify: Is this blocking for NS 1.3 ? |
|
Yes, the central point of this issue, in which the diff wrapper didn't identify changes, comes from https://github.com/phetsims/phet-io-wrappers/issues/406, not authentication issues in https://github.com/phetsims/phet-io-wrappers/issues/405. So however you want to set up NS specific issues is fine (I see #274), but https://github.com/phetsims/phet-io-wrappers/issues/406 certainly blocks NS. |
|
Over in https://github.com/phetsims/phet-io-wrappers/issues/405#issuecomment-827037164, @zepumph said regarding phetsims/phet-io-wrappers#405:
So I'm going assume that this issue can be closed, and that #274 is sufficient for tracking the "broken diff wrapper" problem. @zepumph If that's incorrect, please reopening this issue. Closing. |
|
Oh wait... Since this was the original report by QA, I think it's probably better to keep this issue open, and close #274. To summarize:
|
This was referenced Apr 27, 2021
|
From https://github.com/phetsims/phet-io-wrappers/issues/406, a fix is ready. The commits to cherry-pick are: phetsims/chipper@22fede3 This will require a new chipper branch named "natural-selection-1.3". |
|
Merge confliics when attemoting to cherry-pick phetsims/chipper@22fede3: In Slack, @zepumph said:
... which presumably means https://github.com/phetsims/phet-io-wrappers/issues/406 and: https://github.com/phetsims/phet-io-wrappers/commit/2c2bc9ec6d5f423bf4b70e28e810557f49d31e9d But that does not explain the conflict in chipper. On hold until someone from the PhET-iO team can work with me on Zoom. I've requested assistenance on Slack phet-io channel. |
pixelzoom
changed the title
|
Zoom meeting: @samreid @zepumph @kathy-phet @pixelzoom @kathy-phet would like to deliver a working Diff wrapper with NS 1.3 @zepumph advised against trying to move forward to the new PhET-iO API format. It's cosmetic and hasn't been sufficiently vetted by QA. @zepumph and @samreid will handle patching NS 1.3 to meet the Diff Wrapper requirements. I'm going to consolidate the various natural-selection GitHub issues related to the Diff Wrapper into this one issue. The general issues that need to be addressed are:
|
|
Minor update on the above. @samreid and I completed the diff wrapper requirements on master in https://github.com/phetsims/phet-io-wrappers/issues/410. This issue, that we made before we saw phetsims/phet-io-wrappers#408 or phetsims/phet-io-wrappers#409, encompasses both of those issues. Thus, the issues that will require cherry picking are: https://github.com/phetsims/phet-io-wrappers/issues/405 UPDATE from @samreid: let's also include |
|
I cherry-picked the commits for the issues mentioned in the preceding comment. Here are my notes for which commits were applied and which were skipped: 405 406 407 410 411 In summary, there were 4 cherry-picks for phet-io-wrappers, one of which required manual merge resolution. And there was one independent commit for chipper (not a cherry-pick). Here's what I've done to test these changes:
Run local built and compare to the same list. Every test passed, but in one run a whitespace in the URL led to difficulty--I think that can be fixed in master and left alone in the branch. I'll make a new issue for that. I think the next step for this issue is for @zepumph to review these changes and test, then we can move to RC. |
|
@zepumph @samreid When this issue is ready for QA, please provide the follow. I will use the info you provide here to verify the RC before passing it on to QA.
|
|
Here are our instructions for how QA should test this:
Breaking Changes
|
|
I built a local version of NS 1.3: ... and verified using the instructions in #271 (comment). This is ready for QA in the next RC test. |
6 tasks
|
Deploy of the Diff wrapper is still broken, see https://github.com/phetsims/phet-io-wrappers/issues/405. This is no longer ready for QA. @KatieWoe please skip this issue for phetsims/qa#643 until further notice. |
1 task
|
@KatieWoe Instructions for QA verification are in #271 (comment). Let me or @zepumph know if you have questions. |
6 tasks
|
Looks good in rc.3 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
For phetsims/qa#641 (comment)
In phetsims/qa#641 (comment), @KatieWoe asked:
No, that is definitely not correct.
Here's what was specified in phetsims/qa#641:
There have most definitely been API changes since 1.2. For example, in #263 the Studio tree structure was changed by introducing
environmentPanelas a new "container" node. And those changes have resulted in QA creating issues like #268.@zepumph can you comment on why the diff wrapper is showing no changes between 1.2 and 1.3.0-rc.1 ?
The text was updated successfully, but these errors were encountered: