From 257d5748389ccb1e787410ccf0ec685f34c446f4 Mon Sep 17 00:00:00 2001 From: tashee Date: Wed, 3 Sep 2025 08:59:27 -0400 Subject: [PATCH 1/4] refactor(VariableMap): Stop using deprecated wrapper methods --- blocks/procedures.ts | 12 +- tests/mocha/block_test.js | 9 +- tests/mocha/blocks/procedures_test.js | 13 +- tests/mocha/blocks/variables_test.js | 26 ++-- tests/mocha/field_variable_test.js | 8 +- tests/mocha/jso_serialization_test.js | 5 +- tests/mocha/test_helpers/workspace.js | 206 +++++++++++++------------- tests/mocha/variable_map_test.js | 1 - tests/mocha/xml_test.js | 14 +- 9 files changed, 152 insertions(+), 142 deletions(-) diff --git a/blocks/procedures.ts b/blocks/procedures.ts index 534bfba6e82..b8bc4fddda1 100644 --- a/blocks/procedures.ts +++ b/blocks/procedures.ts @@ -726,21 +726,21 @@ const PROCEDURES_MUTATORARGUMENT = { if (sourceBlock.isInFlyout) { return varName; } - - const model = outerWs.getVariable(varName, ''); + const variableMap = outerWs.getVariableMap(); + const model = variableMap.getVariable(varName, ''); if (model && model.getName() !== varName) { // Rename the variable (case change) - outerWs.renameVariableById(model.getId(), varName); + variableMap.renameVariable(model, varName); } if (!model) { if (this.editingInteractively) { if (!this.editingVariable) { - this.editingVariable = outerWs.createVariable(varName, ''); + this.editingVariable = variableMap.createVariable(varName, ''); } else { - outerWs.renameVariableById(this.editingVariable.getId(), varName); + variableMap.renameVariable(this.editingVariable, varName); } } else { - outerWs.createVariable(varName, ''); + variableMap.createVariable(varName, ''); } } return varName; diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index 1f8f9b1ee41..9f3860edc53 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -2088,6 +2088,7 @@ suite('Blocks', function () { ], }, ]); + this.variableMap = this.workspace.getVariableMap(); }); teardown(function () { eventUtils.enable(); @@ -2426,13 +2427,15 @@ suite('Blocks', function () { assertCollapsed(blockA); }); }); + suite('Renaming Vars', function () { + test('Simple Rename', function () { const blockA = createRenderedBlock(this.workspace, 'variable_block'); blockA.setCollapsed(true); const variable = this.workspace.getVariable('x', ''); - this.workspace.renameVariableById(variable.getId(), 'y'); + this.variableMap.renameVariable(variable, 'y'); this.clock.runAll(); assertCollapsed(blockA, 'y'); @@ -2441,8 +2444,8 @@ suite('Blocks', function () { const blockA = createRenderedBlock(this.workspace, 'variable_block'); blockA.setCollapsed(true); - const variable = this.workspace.createVariable('y'); - this.workspace.renameVariableById(variable.getId(), 'X'); + const variable = this.variableMap.createVariable('y'); + this.variableMap.renameVariable(variable, 'X'); this.clock.runAll(); assertCollapsed(blockA, 'X'); diff --git a/tests/mocha/blocks/procedures_test.js b/tests/mocha/blocks/procedures_test.js index 5ac651fe4cd..6921ef1a45c 100644 --- a/tests/mocha/blocks/procedures_test.js +++ b/tests/mocha/blocks/procedures_test.js @@ -33,6 +33,7 @@ suite('Procedures', function () { 'preCreatedTypedVarId', ); defineRowBlock(); + this.variableMap = this.workspace.getVariableMap(); }); teardown(function () { @@ -453,7 +454,7 @@ suite('Procedures', function () { mutatorIcon.setBubbleVisible(false); const variable = this.workspace.getVariable('param1', ''); - this.workspace.renameVariableById(variable.getId(), 'new name'); + this.variableMap.renameVariable(variable, 'new name'); assert.isNotNull( defBlock.getField('PARAMS'), @@ -480,7 +481,7 @@ suite('Procedures', function () { this.clock.runAll(); const variable = this.workspace.getVariable('param1', ''); - this.workspace.renameVariableById(variable.getId(), 'new name'); + this.variableMap.renameVariable(variable, 'new name'); assert.equal( paramBlock.getFieldValue('NAME'), @@ -506,7 +507,7 @@ suite('Procedures', function () { mutatorIcon.setBubbleVisible(false); const variable = this.workspace.getVariable('param1', ''); - this.workspace.renameVariableById(variable.getId(), 'new name'); + this.variableMap.renameVariable(variable, 'new name'); assert.isNotNull( callBlock.getInput('ARG0'), @@ -535,7 +536,7 @@ suite('Procedures', function () { mutatorIcon.setBubbleVisible(false); const variable = this.workspace.getVariable('param1', ''); - this.workspace.renameVariableById(variable.getId(), 'preCreatedVar'); + this.variableMap.renameVariable(variable, 'preCreatedVar'); assert.isNotNull( defBlock.getField('PARAMS'), @@ -562,7 +563,7 @@ suite('Procedures', function () { this.clock.runAll(); const variable = this.workspace.getVariable('param1', ''); - this.workspace.renameVariableById(variable.getId(), 'preCreatedVar'); + this.variableMap.renameVariable(variable, 'preCreatedVar'); assert.equal( paramBlock.getFieldValue('NAME'), @@ -588,7 +589,7 @@ suite('Procedures', function () { mutatorIcon.setBubbleVisible(false); const variable = this.workspace.getVariable('param1', ''); - this.workspace.renameVariableById(variable.getId(), 'preCreatedVar'); + this.variableMap.renameVariable(variable, 'preCreatedVar'); assert.isNotNull( callBlock.getInput('ARG0'), diff --git a/tests/mocha/blocks/variables_test.js b/tests/mocha/blocks/variables_test.js index a317fe11b52..066de52b875 100644 --- a/tests/mocha/blocks/variables_test.js +++ b/tests/mocha/blocks/variables_test.js @@ -32,9 +32,10 @@ suite('Variables', function () { ], }, ]); - this.workspace.createVariable('foo', 'type1', '1'); - this.workspace.createVariable('bar', 'type1', '2'); - this.workspace.createVariable('baz', 'type1', '3'); + this.variableMap = this.workspace.getVariableMap(); + this.variableMap.createVariable('foo', 'type1', '1'); + this.variableMap.createVariable('bar', 'type1', '2'); + this.variableMap.createVariable('baz', 'type1', '3'); }); teardown(function () { @@ -116,12 +117,11 @@ suite('Variables', function () { ); }); }); - suite('getVariable', function () { test('By ID', function () { - const var1 = this.workspace.createVariable('name1', 'type1', 'id1'); - const var2 = this.workspace.createVariable('name2', 'type1', 'id2'); - const var3 = this.workspace.createVariable('name3', 'type2', 'id3'); + const var1 = this.variableMap.createVariable('name1', 'type1', 'id1'); + const var2 = this.variableMap.createVariable('name2', 'type1', 'id2'); + const var3 = this.variableMap.createVariable('name3', 'type2', 'id3'); const result1 = Blockly.Variables.getVariable(this.workspace, 'id1'); const result2 = Blockly.Variables.getVariable(this.workspace, 'id2'); const result3 = Blockly.Variables.getVariable(this.workspace, 'id3'); @@ -132,9 +132,9 @@ suite('Variables', function () { }); test('By name and type', function () { - const var1 = this.workspace.createVariable('name1', 'type1', 'id1'); - const var2 = this.workspace.createVariable('name2', 'type1', 'id2'); - const var3 = this.workspace.createVariable('name3', 'type2', 'id3'); + const var1 = this.variableMap.createVariable('name1', 'type1', 'id1'); + const var2 = this.variableMap.createVariable('name2', 'type1', 'id2'); + const var3 = this.variableMap.createVariable('name3', 'type2', 'id3'); const result1 = Blockly.Variables.getVariable( this.workspace, null, @@ -161,9 +161,9 @@ suite('Variables', function () { }); test('Bad ID with name and type fallback', function () { - const var1 = this.workspace.createVariable('name1', 'type1', 'id1'); - const var2 = this.workspace.createVariable('name2', 'type1', 'id2'); - const var3 = this.workspace.createVariable('name3', 'type2', 'id3'); + const var1 = this.variableMap.createVariable('name1', 'type1', 'id1'); + const var2 = this.variableMap.createVariable('name2', 'type1', 'id2'); + const var3 = this.variableMap.createVariable('name3', 'type2', 'id3'); const result1 = Blockly.Variables.getVariable( this.workspace, 'badId', diff --git a/tests/mocha/field_variable_test.js b/tests/mocha/field_variable_test.js index 58a20977521..e32e9e52836 100644 --- a/tests/mocha/field_variable_test.js +++ b/tests/mocha/field_variable_test.js @@ -488,13 +488,15 @@ suite('Variable Fields', function () { this.variableField = this.variableBlock.getField('VAR'); }); test('Rename & Keep Old ID', function () { - this.workspace.renameVariableById('id1', 'name2'); + const variableMap = this.workspace.getVariableMap(); + variableMap.renameVariable(variableMap.getVariableById('id1'), 'name2'); assert.equal(this.variableField.getText(), 'name2'); assert.equal(this.variableField.getValue(), 'id1'); }); test('Rename & Get New ID', function () { - this.workspace.createVariable('name2', null, 'id2'); - this.workspace.renameVariableById('id1', 'name2'); + const variableMap = this.workspace.getVariableMap(); + const variable = variableMap.createVariable('name2', null, 'id2'); + variableMap.renameVariable(variableMap.getVariableById('id1'), 'name2'); assert.equal(this.variableField.getText(), 'name2'); assert.equal(this.variableField.getValue(), 'id2'); }); diff --git a/tests/mocha/jso_serialization_test.js b/tests/mocha/jso_serialization_test.js index 4a7d5e9e1d3..15255545d98 100644 --- a/tests/mocha/jso_serialization_test.js +++ b/tests/mocha/jso_serialization_test.js @@ -33,6 +33,7 @@ suite('JSO Serialization', function () { defineStatementBlock(); createGenUidStubWithReturns(new Array(10).fill().map((_, i) => 'id' + i)); + this.variableMap = this.workspace.getVariableMap(); }); teardown(function () { @@ -834,7 +835,7 @@ suite('JSO Serialization', function () { suite('Variables', function () { test('Without type', function () { - this.workspace.createVariable('testVar', '', 'testId'); + this.variableMap.createVariable('testVar', '', 'testId'); const jso = Blockly.serialization.workspaces.save(this.workspace); const variable = jso['variables'][0]; assertProperty(variable, 'name', 'testVar'); @@ -843,7 +844,7 @@ suite('JSO Serialization', function () { }); test('With type', function () { - this.workspace.createVariable('testVar', 'testType', 'testId'); + this.variableMap.createVariable('testVar', 'testType', 'testId'); const jso = Blockly.serialization.workspaces.save(this.workspace); const variable = jso['variables'][0]; assertProperty(variable, 'name', 'testVar'); diff --git a/tests/mocha/test_helpers/workspace.js b/tests/mocha/test_helpers/workspace.js index b5f3cadabd3..a47839be5e2 100644 --- a/tests/mocha/test_helpers/workspace.js +++ b/tests/mocha/test_helpers/workspace.js @@ -25,6 +25,8 @@ export function testAWorkspace() { ], }, ]); + console.log(this.workspace); + this.variableMap = this.workspace.getVariableMap(); }); teardown(function () { @@ -68,13 +70,13 @@ export function testAWorkspace() { suite('clear', function () { test('Trivial', function () { sinon.stub(eventUtils.TEST_ONLY, 'setGroupInternal').returns(null); - this.workspace.createVariable('name1', 'type1', 'id1'); - this.workspace.createVariable('name2', 'type2', 'id2'); + this.variableMap.createVariable('name1', 'type1', 'id1'); + this.variableMap.createVariable('name2', 'type2', 'id2'); this.workspace.newBlock(''); this.workspace.clear(); assert.equal(this.workspace.getTopBlocks(false).length, 0); - const varMapLength = this.workspace.getVariableMap().variableMap.size; + const varMapLength = this.variableMap.variableMap.size; assert.equal(varMapLength, 0); }); @@ -84,7 +86,7 @@ export function testAWorkspace() { this.workspace.clear(); assert.equal(this.workspace.getTopBlocks(false).length, 0); - const varMapLength = this.workspace.getVariableMap().variableMap.size; + const varMapLength = this.variableMap.variableMap.size; assert.equal(varMapLength, 0); }); }); @@ -92,8 +94,8 @@ export function testAWorkspace() { suite('deleteVariable', function () { setup(function () { // Create two variables of different types. - this.var1 = this.workspace.createVariable('name1', 'type1', 'id1'); - this.var2 = this.workspace.createVariable('name2', 'type2', 'id2'); + this.var1 = this.variableMap.createVariable('name1', 'type1', 'id1'); + this.var2 = this.variableMap.createVariable('name2', 'type2', 'id2'); // Create blocks to refer to both of them. createVarBlocksNoEvents(this.workspace, ['id1', 'id1', 'id2']); }); @@ -101,12 +103,12 @@ export function testAWorkspace() { test('deleteVariableById(id2) one usage', function () { // Deleting variable one usage should not trigger confirm dialog. const stub = sinon.stub(window, 'confirm').returns(true); - this.workspace.deleteVariableById('id2'); + this.variableMap.deleteVariableById('id2'); sinon.assert.notCalled(stub); - const variable = this.workspace.getVariableById('id2'); + const variable = this.variableMap.getVariableById('id2'); assert.isNull(variable); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); assertBlockVarModelName(this.workspace, 0, 'name1'); stub.restore(); @@ -115,12 +117,12 @@ export function testAWorkspace() { test('deleteVariableById(id1) multiple usages confirm', function () { // Deleting variable with multiple usages triggers confirm dialog. const stub = sinon.stub(window, 'confirm').returns(true); - this.workspace.deleteVariableById('id1'); + this.variableMap.deleteVariableById('id1'); sinon.assert.calledOnce(stub); - const variable = this.workspace.getVariableById('id1'); + const variable = this.variableMap.getVariableById('id1'); assert.isNull(variable); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); assertBlockVarModelName(this.workspace, 0, 'name2'); stub.restore(); @@ -129,11 +131,11 @@ export function testAWorkspace() { test('deleteVariableById(id1) multiple usages cancel', function () { // Deleting variable with multiple usages triggers confirm dialog. const stub = sinon.stub(window, 'confirm').returns(false); - this.workspace.deleteVariableById('id1'); + this.variableMap.deleteVariableById('id1'); sinon.assert.calledOnce(stub); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); assertBlockVarModelName(this.workspace, 0, 'name1'); assertBlockVarModelName(this.workspace, 1, 'name1'); assertBlockVarModelName(this.workspace, 2, 'name2'); @@ -144,50 +146,50 @@ export function testAWorkspace() { suite('renameVariableById', function () { setup(function () { - this.workspace.createVariable('name1', 'type1', 'id1'); + this.variableMap.createVariable('name1', 'type1', 'id1'); }); test('No references rename to name2', function () { - this.workspace.renameVariableById('id1', 'name2'); - assertVariableValues(this.workspace, 'name2', 'type1', 'id1'); + this.variableMap.renameVariableById('id1', 'name2'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id1'); // Renaming should not have created a new variable. - assert.equal(this.workspace.getAllVariables().length, 1); + assert.equal(this.variableMap.getAllVariables().length, 1); }); test('Reference exists rename to name2', function () { createVarBlocksNoEvents(this.workspace, ['id1']); - this.workspace.renameVariableById('id1', 'name2'); - assertVariableValues(this.workspace, 'name2', 'type1', 'id1'); + this.variableMap.renameVariableById('id1', 'name2'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id1'); // Renaming should not have created a new variable. - assert.equal(this.workspace.getAllVariables().length, 1); + assert.equal(this.variableMap.getAllVariables().length, 1); assertBlockVarModelName(this.workspace, 0, 'name2'); }); test('Reference exists different capitalization rename to Name1', function () { createVarBlocksNoEvents(this.workspace, ['id1']); - this.workspace.renameVariableById('id1', 'Name1'); - assertVariableValues(this.workspace, 'Name1', 'type1', 'id1'); + this.variableMap.renameVariableById('id1', 'Name1'); + assertVariableValues(this.variableMap, 'Name1', 'type1', 'id1'); // Renaming should not have created a new variable. - assert.equal(this.workspace.getAllVariables().length, 1); + assert.equal(this.variableMap.getAllVariables().length, 1); assertBlockVarModelName(this.workspace, 0, 'Name1'); }); suite('Two variables rename overlap', function () { test('Same type rename variable with id1 to name2', function () { - this.workspace.createVariable('name2', 'type1', 'id2'); + this.variableMap.createVariable('name2', 'type1', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - - this.workspace.renameVariableById('id1', 'name2'); + + this.variableMap.renameVariableById('id1', 'name2'); // The second variable should remain unchanged. assertVariableValues(this.workspace, 'name2', 'type1', 'id2'); // The first variable should have been deleted. - const variable = this.workspace.getVariableById('id1'); + const variable = this.variableMap.getVariableById('id1'); assert.isNull(variable); // There should only be one variable left. - assert.equal(this.workspace.getAllVariables().length, 1); + assert.equal(this.variableMap.getAllVariables().length, 1); // Both blocks should now reference variable with name2. assertBlockVarModelName(this.workspace, 0, 'name2'); @@ -195,14 +197,14 @@ export function testAWorkspace() { }); test('Different type rename variable with id1 to name2', function () { - this.workspace.createVariable('name2', 'type2', 'id2'); + this.variableMap.createVariable('name2', 'type2', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - this.workspace.renameVariableById('id1', 'name2'); + this.variableMap.renameVariableById('id1', 'name2'); // Variables with different type are allowed to have the same name. - assertVariableValues(this.workspace, 'name2', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); // Both blocks should now reference variable with name2. assertBlockVarModelName(this.workspace, 0, 'name2'); @@ -210,18 +212,18 @@ export function testAWorkspace() { }); test('Same type different capitalization rename variable with id1 to Name2', function () { - this.workspace.createVariable('name2', 'type1', 'id2'); + this.variableMap.createVariable('name2', 'type1', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - this.workspace.renameVariableById('id1', 'Name2'); + this.variableMap.renameVariableById('id1', 'Name2'); // The second variable should be updated. - assertVariableValues(this.workspace, 'Name2', 'type1', 'id2'); + assertVariableValues(this.variableMap, 'Name2', 'type1', 'id2'); // The first variable should have been deleted. - const variable = this.workspace.getVariableById('id1'); + const variable = this.variableMap.getVariableById('id1'); assert.isNull(variable); // There should only be one variable left. - assert.equal(this.workspace.getAllVariables().length, 1); + assert.equal(this.variableMap.getAllVariables().length, 1); // Both blocks should now reference variable with Name2. assertBlockVarModelName(this.workspace, 0, 'Name2'); @@ -229,15 +231,15 @@ export function testAWorkspace() { }); test('Different type different capitalization rename variable with id1 to Name2', function () { - this.workspace.createVariable('name2', 'type2', 'id2'); + this.variableMap.createVariable('name2', 'type2', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - - this.workspace.renameVariableById('id1', 'Name2'); + + this.variableMap.renameVariableById('id1', 'Name2'); // Variables with different type are allowed to have the same name. - assertVariableValues(this.workspace, 'Name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'Name2', 'type1', 'id1'); // Second variable should remain unchanged. - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); // Only first block should use new capitalization. assertBlockVarModelName(this.workspace, 0, 'Name2'); @@ -1477,180 +1479,180 @@ export function testAWorkspace() { suite('renameVariableById', function () { setup(function () { - this.workspace.createVariable('name1', 'type1', 'id1'); + this.variableMap.createVariable('name1', 'type1', 'id1'); }); test('Reference exists no usages rename to name2', function () { - this.workspace.renameVariableById('id1', 'name2'); + this.variableMap.renameVariableById('id1', 'name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id1'); }); test('Reference exists with usages rename to name2', function () { createVarBlocksNoEvents(this.workspace, ['id1']); - this.workspace.renameVariableById('id1', 'name2'); + this.variableMap.renameVariableById('id1', 'name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); assertBlockVarModelName(this.workspace, 0, 'name1'); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); this.workspace.undo(true); this.clock.runAll(); assertBlockVarModelName(this.workspace, 0, 'name2'); - assertVariableValues(this.workspace, 'name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id1'); }); test('Reference exists different capitalization no usages rename to Name1', function () { - this.workspace.renameVariableById('id1', 'Name1'); + this.variableMap.renameVariableById('id1', 'Name1'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'Name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'Name1', 'type1', 'id1'); }); test('Reference exists different capitalization with usages rename to Name1', function () { createVarBlocksNoEvents(this.workspace, ['id1']); - this.workspace.renameVariableById('id1', 'Name1'); + this.variableMap.renameVariableById('id1', 'Name1'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); assertBlockVarModelName(this.workspace, 0, 'name1'); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); this.workspace.undo(true); this.clock.runAll(); assertBlockVarModelName(this.workspace, 0, 'Name1'); - assertVariableValues(this.workspace, 'Name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'Name1', 'type1', 'id1'); }); suite('Two variables rename overlap', function () { test('Same type no usages rename variable with id1 to name2', function () { - this.workspace.createVariable('name2', 'type1', 'id2'); - this.workspace.renameVariableById('id1', 'name2'); + this.variableMap.createVariable('name2', 'type1', 'id2'); + this.variableMap.renameVariableById('id1', 'name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type1', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id2'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'name2', 'type1', 'id2'); - assert.isNull(this.workspace.getVariableById('id1')); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id2'); + assert.isNull(this.variableMap.getVariableById('id1')); }); test('Same type with usages rename variable with id1 to name2', function () { - this.workspace.createVariable('name2', 'type1', 'id2'); + var variable = this.variableMap.createVariable('name2', 'type1', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - this.workspace.renameVariableById('id1', 'name2'); + this.variableMap.renameVariableById('id1', 'name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); assertBlockVarModelName(this.workspace, 0, 'name1'); assertBlockVarModelName(this.workspace, 1, 'name2'); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type1', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id2'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'name2', 'type1', 'id2'); - assert.isNull(this.workspace.getVariableById('id1')); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id2'); + assert.isNull(this.variableMap.getVariableById('id1')); }); test('Same type different capitalization no usages rename variable with id1 to Name2', function () { - this.workspace.createVariable('name2', 'type1', 'id2'); - this.workspace.renameVariableById('id1', 'Name2'); + this.variableMap.createVariable('name2', 'type1', 'id2'); + this.variableMap.renameVariableById('id1', 'Name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type1', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id2'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'Name2', 'type1', 'id2'); - assert.isNull(this.workspace.getVariable('name1')); + assertVariableValues(this.variableMap, 'Name2', 'type1', 'id2'); + assert.isNull(this.variableMap.getVariable('name1')); }); test('Same type different capitalization with usages rename variable with id1 to Name2', function () { this.workspace.createVariable('name2', 'type1', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - this.workspace.renameVariableById('id1', 'Name2'); + this.variableMap.renameVariableById('id1', 'Name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); assertBlockVarModelName(this.workspace, 0, 'name1'); assertBlockVarModelName(this.workspace, 1, 'name2'); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type1', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id2'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'Name2', 'type1', 'id2'); - assert.isNull(this.workspace.getVariableById('id1')); + assertVariableValues(this.variableMap, 'Name2', 'type1', 'id2'); + assert.isNull(this.variableMap.getVariableById('id1')); assertBlockVarModelName(this.workspace, 0, 'Name2'); assertBlockVarModelName(this.workspace, 1, 'Name2'); }); test('Different type no usages rename variable with id1 to name2', function () { - this.workspace.createVariable('name2', 'type2', 'id2'); - this.workspace.renameVariableById('id1', 'name2'); + this.variableMap.createVariable('name2', 'type2', 'id2'); + this.variableMap.renameVariableById('id1', 'name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'name2', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); }); test('Different type with usages rename variable with id1 to name2', function () { - this.workspace.createVariable('name2', 'type2', 'id2'); + this.variableMap.createVariable('name2', 'type2', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - this.workspace.renameVariableById('id1', 'name2'); + this.variableMap.renameVariableById('id1', 'name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); assertBlockVarModelName(this.workspace, 0, 'name1'); assertBlockVarModelName(this.workspace, 1, 'name2'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'name2', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); assertBlockVarModelName(this.workspace, 0, 'name2'); assertBlockVarModelName(this.workspace, 1, 'name2'); }); test('Different type different capitalization no usages rename variable with id1 to Name2', function () { - this.workspace.createVariable('name2', 'type2', 'id2'); - this.workspace.renameVariableById('id1', 'Name2'); + this.variableMap.createVariable('name2', 'type2', 'id2'); + this.variableMap.renameVariableById('id1', 'Name2'); this.clock.runAll(); this.workspace.undo(); @@ -1660,27 +1662,27 @@ export function testAWorkspace() { this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'Name2', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'Name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); }); test('Different type different capitalization with usages rename variable with id1 to Name2', function () { - this.workspace.createVariable('name2', 'type2', 'id2'); + this.variableMap.createVariable('name2', 'type2', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - this.workspace.renameVariableById('id1', 'Name2'); + this.variableMap.renameVariableById('id1', 'Name2'); this.clock.runAll(); this.workspace.undo(); this.clock.runAll(); - assertVariableValues(this.workspace, 'name1', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'name1', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); assertBlockVarModelName(this.workspace, 0, 'name1'); assertBlockVarModelName(this.workspace, 1, 'name2'); this.workspace.undo(true); this.clock.runAll(); - assertVariableValues(this.workspace, 'Name2', 'type1', 'id1'); - assertVariableValues(this.workspace, 'name2', 'type2', 'id2'); + assertVariableValues(this.variableMap, 'Name2', 'type1', 'id1'); + assertVariableValues(this.variableMap, 'name2', 'type2', 'id2'); assertBlockVarModelName(this.workspace, 0, 'Name2'); assertBlockVarModelName(this.workspace, 1, 'name2'); }); diff --git a/tests/mocha/variable_map_test.js b/tests/mocha/variable_map_test.js index b0ceec28e4c..c7618797c1c 100644 --- a/tests/mocha/variable_map_test.js +++ b/tests/mocha/variable_map_test.js @@ -428,7 +428,6 @@ suite('Variable Map', function () { 'test id', ); this.variableMap.renameVariable(variable, 'new test name'); - assertEventFired( this.eventSpy, Blockly.Events.VarRename, diff --git a/tests/mocha/xml_test.js b/tests/mocha/xml_test.js index cd1bce128bb..210b8170585 100644 --- a/tests/mocha/xml_test.js +++ b/tests/mocha/xml_test.js @@ -119,6 +119,7 @@ suite('XML', function () { suite('blockToDom', function () { setup(function () { this.workspace = new Blockly.Workspace(); + this.variableMap = this.workspace.getVariableMap(); }); teardown(function () { workspaceTeardown.call(this, this.workspace); @@ -296,7 +297,7 @@ suite('XML', function () { ]); }); test('Variable Trivial', function () { - this.workspace.createVariable('name1', '', 'id1'); + this.variableMap.createVariable('name1', '', 'id1'); const block = new Blockly.Block( this.workspace, 'field_variable_test_block', @@ -306,7 +307,7 @@ suite('XML', function () { assertVariableDomField(resultFieldDom, 'VAR', null, 'id1', 'name1'); }); test('Variable Typed', function () { - this.workspace.createVariable('name1', 'string', 'id1'); + this.variableMap.createVariable('name1', 'string', 'id1'); const block = new Blockly.Block( this.workspace, 'field_variable_test_block', @@ -323,7 +324,7 @@ suite('XML', function () { }); test('Variable Default Case', function () { createGenUidStubWithReturns('1'); - this.workspace.createVariable('name1'); + this.variableMap.createVariable('name1'); Blockly.Events.disable(); const block = new Blockly.Block( @@ -439,13 +440,14 @@ suite('XML', function () { ], }, ]); + this.variableMap = this.workspace.getVariableMap(); }); teardown(function () { workspaceTeardown.call(this, this.workspace); }); test('One Variable', function () { createGenUidStubWithReturns('1'); - this.workspace.createVariable('name1'); + this.variableMap.createVariable('name1'); const resultDom = Blockly.Xml.variablesToDom( this.workspace.getAllVariables(), ); @@ -456,8 +458,8 @@ suite('XML', function () { assert.equal(resultVariableDom.getAttribute('id'), '1'); }); test('Two Variable one block', function () { - this.workspace.createVariable('name1', '', 'id1'); - this.workspace.createVariable('name2', 'type2', 'id2'); + this.variableMap.createVariable('name1', '', 'id1'); + this.variableMap.createVariable('name2', 'type2', 'id2'); // If events are enabled during block construction, it will create a // default variable. Blockly.Events.disable(); From 9864c8146df7300b11dfb5b28e3b2ec25d73d3ce Mon Sep 17 00:00:00 2001 From: tashee Date: Wed, 3 Sep 2025 10:20:16 -0400 Subject: [PATCH 2/4] fix format --- tests/mocha/block_test.js | 3 +-- tests/mocha/test_helpers/workspace.js | 10 +++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index 9f3860edc53..ce0085793c7 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -2427,9 +2427,8 @@ suite('Blocks', function () { assertCollapsed(blockA); }); }); - + suite('Renaming Vars', function () { - test('Simple Rename', function () { const blockA = createRenderedBlock(this.workspace, 'variable_block'); diff --git a/tests/mocha/test_helpers/workspace.js b/tests/mocha/test_helpers/workspace.js index a47839be5e2..6894181cb36 100644 --- a/tests/mocha/test_helpers/workspace.js +++ b/tests/mocha/test_helpers/workspace.js @@ -180,7 +180,7 @@ export function testAWorkspace() { test('Same type rename variable with id1 to name2', function () { this.variableMap.createVariable('name2', 'type1', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - + this.variableMap.renameVariableById('id1', 'name2'); // The second variable should remain unchanged. @@ -233,7 +233,7 @@ export function testAWorkspace() { test('Different type different capitalization rename variable with id1 to Name2', function () { this.variableMap.createVariable('name2', 'type2', 'id2'); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); - + this.variableMap.renameVariableById('id1', 'Name2'); // Variables with different type are allowed to have the same name. @@ -1558,7 +1558,11 @@ export function testAWorkspace() { }); test('Same type with usages rename variable with id1 to name2', function () { - var variable = this.variableMap.createVariable('name2', 'type1', 'id2'); + var variable = this.variableMap.createVariable( + 'name2', + 'type1', + 'id2', + ); createVarBlocksNoEvents(this.workspace, ['id1', 'id2']); this.variableMap.renameVariableById('id1', 'name2'); this.clock.runAll(); From 862a6538d1fcf77c5895a955926cf9066dce5857 Mon Sep 17 00:00:00 2001 From: Tasheena Date: Wed, 24 Sep 2025 11:13:39 -0400 Subject: [PATCH 3/4] fix: Apply review suggestions Co-authored-by: Christopher Allen --- tests/mocha/field_variable_test.js | 2 +- tests/mocha/test_helpers/workspace.js | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/mocha/field_variable_test.js b/tests/mocha/field_variable_test.js index e32e9e52836..c2b40326f2f 100644 --- a/tests/mocha/field_variable_test.js +++ b/tests/mocha/field_variable_test.js @@ -495,7 +495,7 @@ suite('Variable Fields', function () { }); test('Rename & Get New ID', function () { const variableMap = this.workspace.getVariableMap(); - const variable = variableMap.createVariable('name2', null, 'id2'); + variableMap.createVariable('name2', null, 'id2'); variableMap.renameVariable(variableMap.getVariableById('id1'), 'name2'); assert.equal(this.variableField.getText(), 'name2'); assert.equal(this.variableField.getValue(), 'id2'); diff --git a/tests/mocha/test_helpers/workspace.js b/tests/mocha/test_helpers/workspace.js index 6894181cb36..452933c081a 100644 --- a/tests/mocha/test_helpers/workspace.js +++ b/tests/mocha/test_helpers/workspace.js @@ -25,7 +25,6 @@ export function testAWorkspace() { ], }, ]); - console.log(this.workspace); this.variableMap = this.workspace.getVariableMap(); }); @@ -1558,7 +1557,7 @@ export function testAWorkspace() { }); test('Same type with usages rename variable with id1 to name2', function () { - var variable = this.variableMap.createVariable( + const variable = this.variableMap.createVariable( 'name2', 'type1', 'id2', From a48060afa04d7d8a20f4db8520c609f837c2c855 Mon Sep 17 00:00:00 2001 From: tashee Date: Wed, 24 Sep 2025 11:20:56 -0400 Subject: [PATCH 4/4] fix: restore blank line --- tests/mocha/variable_map_test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/mocha/variable_map_test.js b/tests/mocha/variable_map_test.js index c7618797c1c..b0ceec28e4c 100644 --- a/tests/mocha/variable_map_test.js +++ b/tests/mocha/variable_map_test.js @@ -428,6 +428,7 @@ suite('Variable Map', function () { 'test id', ); this.variableMap.renameVariable(variable, 'new test name'); + assertEventFired( this.eventSpy, Blockly.Events.VarRename,