From c4308007bc4b58d51adf1fda7b51ffa9f1d3f093 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Mon, 21 Mar 2022 15:38:23 -0700 Subject: [PATCH] fix: always rename caller to legal name (#6014) * fix: always rename caller to legal name * fix: enable tests for non-matching callers which overlap names getting renamed * fix: remove TODOs --- blocks/procedures.js | 9 ++++---- tests/mocha/procedures_test.js | 29 ++++++++++++++------------ tests/mocha/test_helpers/procedures.js | 9 ++++++-- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index d9de9e098..e5ca8e806 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -968,11 +968,10 @@ const PROCEDURE_CALL_COMMON = { block.appendChild(mutation); const field = xmlUtils.createElement('field'); field.setAttribute('name', 'NAME'); - let callName = this.getProcedureCall(); - if (!callName) { - // Rename if name is empty string. - callName = Procedures.findLegalName('', this); - this.renameProcedure('', callName); + const callName = this.getProcedureCall(); + const newName = Procedures.findLegalName(callName, this); + if (callName !== newName) { + this.renameProcedure(callName, newName); } field.appendChild(xmlUtils.createTextNode(callName)); block.appendChild(field); diff --git a/tests/mocha/procedures_test.js b/tests/mocha/procedures_test.js index f884e065e..6d7888a9f 100644 --- a/tests/mocha/procedures_test.js +++ b/tests/mocha/procedures_test.js @@ -10,7 +10,7 @@ goog.require('Blockly'); goog.require('Blockly.Msg'); const {assertCallBlockStructure, assertDefBlockStructure, createProcDefBlock, createProcCallBlock} = goog.require('Blockly.test.helpers.procedures'); const {runSerializationTestSuite} = goog.require('Blockly.test.helpers.serialization'); -const {sharedTestSetup, sharedTestTeardown, workspaceTeardown} = goog.require('Blockly.test.helpers.setupTeardown'); +const {createGenUidStubWithReturns, sharedTestSetup, sharedTestTeardown, workspaceTeardown} = goog.require('Blockly.test.helpers.setupTeardown'); suite('Procedures', function() { @@ -294,8 +294,12 @@ suite('Procedures', function() { }); }); suite('caller param mismatch', function() { - test.skip('callreturn with missing args', function() { - // TODO: How do we want it to behave in this situation? + setup(function() { + this.TEST_VAR_ID = 'test-id'; + this.genUidStub = createGenUidStubWithReturns(this.TEST_VAR_ID); + }); + + test('callreturn with missing args', function() { const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something @@ -310,10 +314,9 @@ suite('Procedures', function() { '' ), this.workspace); assertDefBlockStructure(defBlock, true, ['x'], ['arg']); - assertCallBlockStructure(callBlock, ['x'], ['arg']); + assertCallBlockStructure(callBlock, [], [], 'do something2'); }); - test.skip('callreturn with bad args', function() { - // TODO: How do we want it to behave in this situation? + test('callreturn with bad args', function() { const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something @@ -330,10 +333,10 @@ suite('Procedures', function() { `), this.workspace); assertDefBlockStructure(defBlock, true, ['x'], ['arg']); - assertCallBlockStructure(callBlock, ['x'], ['arg']); + assertCallBlockStructure( + callBlock, ['y'], [this.TEST_VAR_ID], 'do something2'); }); - test.skip('callnoreturn with missing args', function() { - // TODO: How do we want it to behave in this situation? + test('callnoreturn with missing args', function() { const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something @@ -348,10 +351,9 @@ suite('Procedures', function() { '' ), this.workspace); assertDefBlockStructure(defBlock, false, ['x'], ['arg']); - assertCallBlockStructure(callBlock, ['x'], ['arg']); + assertCallBlockStructure(callBlock, [], [], 'do something2'); }); - test.skip('callnoreturn with bad args', function() { - // TODO: How do we want it to behave in this situation? + test('callnoreturn with bad args', function() { const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something @@ -368,7 +370,8 @@ suite('Procedures', function() { `), this.workspace); assertDefBlockStructure(defBlock, false, ['x'], ['arg']); - assertCallBlockStructure(callBlock, ['x'], ['arg']); + assertCallBlockStructure( + callBlock, ['y'], [this.TEST_VAR_ID], 'do something2'); }); }); }); diff --git a/tests/mocha/test_helpers/procedures.js b/tests/mocha/test_helpers/procedures.js index 6978e283e..23843c9f6 100644 --- a/tests/mocha/test_helpers/procedures.js +++ b/tests/mocha/test_helpers/procedures.js @@ -85,13 +85,15 @@ function assertDefBlockStructure(defBlock, hasReturn = false, exports.assertDefBlockStructure = assertDefBlockStructure; /** - * Asserts that the procedure definition block has the expected inputs and + * Asserts that the procedure call block has the expected inputs and * fields. * @param {!Blockly.Block} callBlock The procedure call block. * @param {Array=} args An array of argument names. * @param {Array=} varIds An array of variable ids. + * @param {string=} name The name we expect the caller to have. */ -function assertCallBlockStructure(callBlock, args = [], varIds = []) { +function assertCallBlockStructure( + callBlock, args = [], varIds = [], name = undefined) { if (args.length) { chai.assert.include(callBlock.toString(), 'with'); } else { @@ -100,6 +102,9 @@ function assertCallBlockStructure(callBlock, args = [], varIds = []) { assertCallBlockArgsStructure(callBlock, args); assertBlockVarModels(callBlock, varIds); + if (name !== undefined) { + chai.assert(callBlock.getFieldValue('NAME'), name); + } } exports.assertCallBlockStructure = assertCallBlockStructure;