-
Notifications
You must be signed in to change notification settings - Fork 13
fix: Broken tests + upgrade to Blockly 12.0.0. #527
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
fix: Broken tests + upgrade to Blockly 12.0.0. #527
Conversation
These correctly link against develop for v12 tip-of-tree, instead. This also includes some renaming in/of the workflows for clarity.
|
Currently investigating 1 new failure that is somehow different between beta.7 and 12.0.0. |
|
Ah. From testing it seems that RaspberryPiFoundation/blockly#9054 caused this new failure. Test |
Using 'v12' is more generic and will automatically be correct when the plugin moves to an eventual 12.0.1 or 12.1.0.
|
I've verified that the tests are passing when linked against RaspberryPiFoundation/blockly#9063. This PR should block on being merged until its CI passes (i.e. after RaspberryPiFoundation/blockly#9063 is merged), however it can still probably be reviewed now. |
BenHenning
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed changes.
FYI I noticed that my regeneration of package-lock.json includes some possibly unexpected removals. While everything is running well locally, I wonder if this has something to do with my local npm version.
a0bf76f
into
RaspberryPiFoundation:main
Fixes #525
These tests were broken as a result of RaspberryPiFoundation/blockly#9045 and have been updated as such:
getCurrentFocusNodeIdcalls have been updated to use a newgetCurrentFocusedBlockIdsince they actually want to match against a block's data ID rather than its focus-specific ID (which is new as of fix: Ensure BlockSvg is uniquely focusable on the page blockly#9045).getCurrentFocusNodeId(specifically fields).Separately,
clickBlockhas been removed. Its uses were removed in #482 (I think) in favor of direct keyboard navigation. I suspect this is the pattern we want to keep moving forward since it has better alignment with real user behavior, so removing the helper ensures tests don't fall back on it instead of trying to use real navigation.Finally, this PR also updates the plugin to 12.0.0 of Blockly now that it's fully launched. I've tested this against a variety of scenarios yesterday and today so it seems to be working pretty well (#526 notwithstanding). Additionally, the CI workflows have been updated to link against latest develop as the new "tip-of-tree" v12. Unfortunately there was a behavioral discrepancy between beta.7 and the full 12.0.0 release which resulted in 1 new test failing. This will be fixed by RaspberryPiFoundation/blockly#9063 but it means two things:
FieldImageis clickable when appropriate blockly#9063 is merged.mainbranch's CI won't pass once this is merged due to that one test failing. Once we push a 12.0.1 or 12.1.0 version of Blockly and update this plugin accordingly, the test will start passing inmain's CI.