[Google Blockly] sort blocks and skip childBlocks before comparing xml/json block arrays #51952
Conversation
mikeharv
changed the title
|
|
||
| // compareBlockArrays returns an array of differences found. | ||
| const differences = compareBlockArrays(xmlBlocks, jsonBlocks); | ||
| if (differences.length > 0 || experimentEnabled) { | ||
| // Log a record to Firehose/Redshift. | ||
| const recordData = { | ||
| projectUrl: dashboard.project.getShareUrl(), |
There was a problem hiding this comment.
Why did you remove projectURL?
There was a problem hiding this comment.
When looking at the RedShift data, I realized that the url is already included under the device column that we automatically collect. It's also a better url than the project share link because it includes things like /view or /edit or tells us if they were viewing a curriculum level and not in a standalone project. Was just going to add a note to explain!
|
After discussing this with Maribeth, neither of us were confident enough that
To be extra on the safe side, I've added a simple manual sort function will sort all blocks in each array alphabetically be block id. We don't care what order they're in really, as long as it's the same in each list. :) FYI @bencodeorg @fisher-alice LMK if you have any questions/concerns since I'm not planning to merge tonight. |
Sounds good - thanks for the update! I was curious about the ordering of the block arrays from the get go. |
…l/json block arrays (#51952) * sort blocks and skip childBlocks * stop logging project url, and move differences to front of record * manually sort by block id just to be safe * dispose temporary workspaces
…l/json block arrays (#51952) * sort blocks and skip childBlocks * stop logging project url, and move differences to front of record * manually sort by block id just to be safe * dispose temporary workspaces
This updates the logging that was recently implemented for comparing blocks created from XML/JSON sources:
While the experiment ran this morning, we did end up logging some differences. Many of these differences seem to be false signals due to the blocks being in a different order in each list used for the comparison. It turns out that the function we call to get the blocks (
getAllBlocks()) has an optional parameter that sorts the list based on the blocks position on the workspace. (Documentation: https://developers.google.com/blockly/reference/js/blockly.workspace_class.getallblocks_1_method)I confirmed with the Blockly team that without this argument, the blocks are returned in an undefined order. Based on some spot-checking of logged projects, it seems like this could be happening most often with blocks that are close together on the workspace, but this is merely anecdotal and hypothetical. For example, the
when hit an obstacleandwhen clickblocks were not in the same order in each list for this workspace:Separate from this issue, I also found some logged records that showed we were occasionally hitting a longer recursive loop when checking big stacks of blocks. This is because blocks stacked together have a
childBlocks_property that includes references to other blocks we'd already be checking. Adding this property tokeysToSkipshould make the test run more efficiently as we can skip comparing the same blocks multiple times.With these changes, it's possible there could still be other differences logged the next time we run the experiment, but removing the common red herrings I saw today should help us narrow in on any actual issues that may exist.
Links
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: