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

[Google Blockly] Skip logging if workspace is readonly #51996

Merged
merged 1 commit into from
May 19, 2023

Conversation

mikeharv
Copy link
Contributor

@mikeharv mikeharv commented May 19, 2023

This updates the logging that was recently implemented for comparing blocks created from XML/JSON sources:

During the second round of the experiment this morning, we logged difference records for two types of cases:

  1. We had some noisy logging to checking readOnly workspaces. We don't need to save for readOnly workspaces, so we can skip the test in those cases.
  2. We had a few legitimate differences reported related to a specialized block field in Dance Party. However, I've been unable to reproduce the failure with those projects. In these cases, an array of generatedOptions_ was found to have increased in length by one when serializing with JSON. To allow us to see the specific values that are changing, we can stringify the actual object arguments being compared.

For item 2, this would be expected to improve the clarity of certain records. Here's a hypothetical comparison for a difference found in the number of dropdown options in a block:
Before

{
  "result": "different number of keys",
  "path": ".0.inputList.0.fieldRow.0.generatedOptions_",
  "keys1": "0,1,2",
  "keys2": "0,1,2,3"
}

After:

{
  "result": "different number of keys",
  "path": ".0.inputList.0.fieldRow.0.generatedOptions_",
  "keys1": "myDancer,]Tm$k.6A?M.aLF!]$ir:,Rename all myDancer,RENAME_ALL_ID,Rename this variable,RENAME_THIS_ID",
  "keys2": "myDancer,]Tm$k.6A?M.aLF!]$ir:,Rename all myDancer,RENAME_ALL_ID,Rename this variable,RENAME_THIS_ID,some new option label,some new value"
}

(Scroll to the right to see full strings. Note that the last option above is for illustrative purposes only. I haven't been able to repro the problem!)

Additional context on Slack: https://codedotorg.slack.com/archives/C03DBDN67B7/p1684504970060669

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@mikeharv mikeharv requested review from bencodeorg and a team May 19, 2023 15:20
Copy link
Contributor

@fisher-alice fisher-alice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed documentation of this iterative process.

@mikeharv mikeharv merged commit 39cf393 into staging May 19, 2023
2 checks passed
@mikeharv mikeharv deleted the mike/do-not-test-read-only-workspaces branch May 19, 2023 17:23
pablo-code-org pushed a commit that referenced this pull request May 25, 2023
snickell pushed a commit that referenced this pull request Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants