From 3728cfed45ef10559e12bcb95d0086c41e0a6626 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 16:21:19 -0800 Subject: [PATCH] Update tests in workspace_test. Get rid of renameVariable --- core/field_variable.js | 14 +- core/workspace.js | 27 +--- tests/jsunit/workspace_test.js | 284 +++++++++++++++------------------ 3 files changed, 147 insertions(+), 178 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 73fccd58f..9bff99214 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -168,13 +168,17 @@ Blockly.FieldVariable.prototype.getText = function() { */ Blockly.FieldVariable.prototype.setValue = function(id) { var workspace = this.sourceBlock_.workspace; - //var variable = this.variableMap_.getVariableById(id); - var potentialVariableMap = workspace.isFlyout ? - workspace.targetWorkspace.potentialVariableMap_ : null; var variable = workspace.getVariableById(id); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); + if (!variable) { + if (workspace.isFlyout && workspace.targetWorkspace) { + var potentialVariableMap = + workspace.targetWorkspace.potentialVariableMap_; + if (potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } } + if (!variable) { throw new Error('Variable id doesn\'t point to a real variable! ID was ' + id); diff --git a/core/workspace.js b/core/workspace.js index 6055dc4d0..3919190bd 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -241,12 +241,10 @@ Blockly.Workspace.prototype.updateVariableStore = function(clear) { * variable to rename with the given variable. * @param {?Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. - * @param {string=} opt_type The type of the variable to create if variable was - * null. */ Blockly.Workspace.prototype.renameVariableInternal_ = function( - variable, newName, opt_type) { - var type = variable ? variable.type : (opt_type || ''); + variable, newName) { + var type = variable ? variable.type : ''; var newVariable = this.getVariable(newName, type); // If a variable previously existed with the new name, we will coalesce the @@ -278,24 +276,6 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( Blockly.Events.setGroup(false); }; - -/** - * Rename a variable by updating its name in the variable map. Identify the - * variable to rename with the given name. - * TODO (#1199): Possibly delete this function. - * @param {string} oldName Variable to rename. - * @param {string} newName New variable name. - * @param {string=} opt_type The type of the variable. If not provided it - * defaults to the empty string, which is a specific type. - */ -Blockly.Workspace.prototype.renameVariable = function(oldName, newName, - opt_type) { - var type = opt_type || ''; - // Warning: Prefer to use renameVariableById. - var variable = this.getVariable(oldName, type); - this.renameVariableInternal_(variable, newName, opt_type); -}; - /** * Rename a variable by updating its name in the variable map. Identify the * variable to rename with the given ID. @@ -304,6 +284,9 @@ Blockly.Workspace.prototype.renameVariable = function(oldName, newName, */ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { var variable = this.getVariableById(id); + if (!variable) { + throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); + } this.renameVariableInternal_(variable, newName); }; diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 59a7fa902..d39eac1e4 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -31,6 +31,7 @@ Blockly.defineBlocksWithJsonArray([{ { "type": "field_variable", "name": "VAR", + "variableTypes": ["", "type1", "type2"] } ] }]); @@ -50,9 +51,12 @@ function workspaceTest_tearDown() { * @param {?string} variable_name The string to put into the variable field. * @return {!Blockly.Block} The created block. */ -function createMockBlock(variable_name) { +function createMockBlock(variable_id) { + // Turn off events to avoid testing XML at the same time. + Blockly.Events.disable(); var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variable_name); + block.inputList[0].fieldRow[0].setValue(variable_id); + Blockly.Events.enable(); return block; } @@ -158,19 +162,17 @@ function test_getBlockById() { function test_deleteVariable_InternalTrivial() { workspaceTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - var var_1 = workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name1'); - createMockBlock('name2'); + var var_1 = workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableInternal_(var_1); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; + var variable = workspace.getVariableById('id1'); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); assertEquals('name2', block_var_name); workspaceTest_tearDown(); } @@ -239,7 +241,7 @@ function test_updateVariableStore_ClearAndOneInUse() { try { workspace.updateVariableStore(true); checkVariableValues(workspace, 'name1', '', 'id1'); - var variabe = workspace.getVariable('name2', ''); + var variable = workspace.getVariable('name2', ''); assertNull(variable); } finally { workspaceTest_tearDown(); @@ -249,16 +251,21 @@ function test_updateVariableStore_ClearAndOneInUse() { function test_addTopBlock_TrivialFlyoutIsTrue() { workspaceTest_setUp(); workspace.isFlyout = true; - var block = createMockBlock(); - workspace.removeTopBlock(block); - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', [block], - [['name1']]); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); + var targetWorkspace = new Blockly.Workspace(); + workspace.targetWorkspace = targetWorkspace; + targetWorkspace.createVariable('name1', '', '1'); + + // Flyout.init usually does this binding. + workspace.getVariableById = + targetWorkspace.getVariableById.bind(targetWorkspace); try { + var block = createMockBlock('1'); + workspace.removeTopBlock(block); workspace.addTopBlock(block); checkVariableValues(workspace, 'name1', '', '1'); } finally { + targetWorkspace.dispose(); workspaceTest_tearDown(); } } @@ -297,52 +304,41 @@ function test_clear_NoVariables() { } } -function test_renameVariable_NoBlocks() { - // Expect 'renameVariable' to create new variable with newName. +function test_renameVariable_NoReference() { + // Test renaming a variable in the simplest case: when no blocks refer to it. workspaceTest_setUp(); + var id = 'id1'; + var type = 'type1'; var oldName = 'name1'; var newName = 'name2'; - // Mocked setGroup to ensure only one call to the mocked genUid. - setUpMockMethod(mockControl_, Blockly.Events, 'setGroup', [true, false], - null); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); + workspace.createVariable(oldName, type, id); try { - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, 'name2', '', '1'); - var variable = workspace.getVariable(oldName, ''); - assertNull(variable); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + // Renaming should not have created a new variable. + assertEquals(1, workspace.getAllVariables().length); } finally { workspaceTest_tearDown(); } } -function test_renameVariable_SameNameNoBlocks() { - // Expect 'renameVariable' to create new variable with newName. - workspaceTest_setUp(); - var name = 'name1'; - workspace.createVariable(name, 'type1', 'id1'); - - workspace.renameVariable(name, name); - checkVariableValues(workspace, name, 'type1', 'id1'); - workspaceTest_tearDown(); -} - -function test_renameVariable_OnlyOldNameBlockExists() { +function test_renameVariable_ReferenceExists() { + // Test renaming a variable when a reference to it exists. // Expect 'renameVariable' to change oldName variable name to newName. workspaceTest_setUp(); + var id = 'id1'; + var type = 'type1'; var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, '', 'id1'); - createMockBlock(oldName); + workspace.createVariable(oldName, type, id); + createMockBlock(id); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, '', 'id1'); - var variable = workspace.getVariable(oldName, ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; - assertNull(variable); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + // Renaming should not have created a new variable. + assertEquals(1, workspace.getAllVariables().length); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertEquals(newName, block_var_name); workspaceTest_tearDown(); } @@ -351,151 +347,137 @@ function test_renameVariable_TwoVariablesSameType() { // Expect 'renameVariable' to change oldName variable name to newName. // Expect oldName block name to change to newName workspaceTest_setUp(); + var id1 = 'id1'; + var id2 = 'id2'; + var type = 'type1'; + var oldName = 'name1'; var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(newName, '', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); + // Create two variables of the same type. + workspace.createVariable(oldName, type, id1); + workspace.createVariable(newName, type, id2); + // Create blocks to refer to both of them. + createMockBlock(id1); + createMockBlock(id2); - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; + workspace.renameVariableById(id1, newName); + checkVariableValues(workspace, newName, type, id2); + // The old variable should have been deleted. + var variable = workspace.getVariableById(id1); assertNull(variable); + + // There should only be one variable left. + assertEquals(1, workspace.getAllVariables().length); + + // References should have the correct names. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); + workspaceTest_tearDown(); } function test_renameVariable_TwoVariablesDifferentType() { - // Expect triggered error because of different types + // Expect the rename to succeed, because variables with different types are + // allowed to have the same name. workspaceTest_setUp(); + var id1 = 'id1'; + var id2 = 'id2'; + var type1 = 'type1'; + var type2 = 'type2'; + var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, 'type1', 'id1'); - workspace.createVariable(newName, 'type2', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); + // Create two variables of different types. + workspace.createVariable(oldName, type1, id1); + workspace.createVariable(newName, type2, id2); + // Create blocks to refer to both of them. + createMockBlock(id1); + createMockBlock(id2); - try { - workspace.renameVariable(oldName, newName); - fail(); - } catch (e) { - // expected - } - checkVariableValues(workspace, oldName, 'type1', 'id1'); - checkVariableValues(workspace, newName, 'type2', 'id2'); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; - assertEquals(oldName, block_var_name_1); + workspace.renameVariableById(id1, newName); + + checkVariableValues(workspace, newName, type1, id1); + checkVariableValues(workspace, newName, type2, id2); + + // References shoul have the correct names. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; + assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); + workspaceTest_tearDown(); } function test_renameVariable_OldCase() { - // Expect triggered error because of different types + // Rename a variable with a single reference. Update only the capitalization. workspaceTest_setUp(); var oldCase = 'Name1'; var newName = 'name1'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldCase, '', 'id1'); - createMockBlock(oldCase); + var type = 'type1'; + var id = 'id1'; + workspace.createVariable(oldCase, type, id); + createMockBlock(id); - workspace.renameVariable(oldCase, newName); - checkVariableValues(workspace, newName, '', 'id1'); - var result_oldCase = workspace.getVariable(oldCase, '').name; - assertNotEquals(oldCase, result_oldCase); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + var variable = workspace.getVariableById(id); + assertNotEquals(oldCase, id.name); workspaceTest_tearDown(); } function test_renameVariable_TwoVariablesAndOldCase() { - // Expect triggered error because of different types + // Test renaming a variable to an in-use name, but with different + // capitalization. The new capitalization should apply everywhere. + + // TODO (fenichel): What about different capitalization but also different + // types? workspaceTest_setUp(); var oldName = 'name1'; var oldCase = 'Name2'; var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(oldCase, '', 'id2'); - createMockBlock(oldName); - createMockBlock(oldCase); - workspace.renameVariable(oldName, newName); + var id1 = 'id1'; + var id2 = 'id2'; - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var result_oldCase = workspace.getVariable(oldCase).name; - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; + var type = 'type1'; + + workspace.createVariable(oldName, type, id1); + workspace.createVariable(oldCase, type, id2); + createMockBlock(id1); + createMockBlock(id2); + + workspace.renameVariableById(id1, newName); + + checkVariableValues(workspace, newName, type, id2); + + // The old variable should have been deleted. + var variable = workspace.getVariableById(id1); assertNull(variable); - assertNotEquals(oldCase, result_oldCase); + + // There should only be one variable left. + assertEquals(1, workspace.getAllVariables().length); + + // Blocks should now use the new capitalization. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); workspaceTest_tearDown(); } -// Extra testing not required for renameVariableById. It calls renameVariable -// and that has extensive testing. -function test_renameVariableById_TwoVariablesSameType() { - // Expect 'renameVariableById' to change oldName variable name to newName. - // Expect oldName block name to change to newName - workspaceTest_setUp(); - var oldName = 'name1'; - var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(newName, '', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); - - workspace.renameVariableById('id1', newName); - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; - assertNull(variable); - assertEquals(newName, block_var_name_1); - assertEquals(newName, block_var_name_2); - workspaceTest_tearDown(); -} - -function test_deleteVariable_Trivial() { - workspaceTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); - - workspace.deleteVariable('name1', ''); - checkVariableValues(workspace, 'name2', '', 'id2'); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; - assertNull(variable); - assertEquals('name2', block_var_name); - workspaceTest_tearDown(); -} - function test_deleteVariableById_Trivial() { workspaceTest_setUp(); - // TODO (#1199): Make a version of this test that uses different types. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; + checkVariableValues(workspace, 'name2', 'type2', 'id2'); + var variable = workspace.getVariableById('id1'); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable); assertEquals('name2', block_var_name); workspaceTest_tearDown();