From f9c19db146128384e87b7888ebceeab46049c52d Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Mon, 21 Mar 2022 18:50:51 +0000 Subject: [PATCH 1/3] fix: always rename caller to legal name --- blocks/procedures.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index d9de9e09898..e5ca8e80640 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); From 85c6bbfec728fefef1be93386a4da3329eb8397a Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Mon, 21 Mar 2022 19:16:36 +0000 Subject: [PATCH 2/3] fix: enable tests for non-matching callers which overlap names getting renamed --- tests/mocha/procedures_test.js | 25 ++++++++++++++++--------- tests/mocha/test_helpers/procedures.js | 9 +++++++-- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/tests/mocha/procedures_test.js b/tests/mocha/procedures_test.js index f884e065efe..c78ce3831e8 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,7 +294,12 @@ suite('Procedures', function() { }); }); suite('caller param mismatch', function() { - test.skip('callreturn with missing args', function() { + setup(function() { + this.TEST_VAR_ID = 'test-id'; + this.genUidStub = createGenUidStubWithReturns(this.TEST_VAR_ID); + }); + + test('callreturn with missing args', function() { // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` @@ -310,9 +315,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() { + test('callreturn with bad args', function() { // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` @@ -330,9 +335,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() { + test('callnoreturn with missing args', function() { // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` @@ -348,9 +354,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() { + test('callnoreturn with bad args', function() { // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` @@ -368,7 +374,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 6978e283eb9..23843c9f62d 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; From 75bf5b9e94131ba2d94374226b66ea24569547d4 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Mon, 21 Mar 2022 19:22:10 +0000 Subject: [PATCH 3/3] fix: remove TODOs --- tests/mocha/procedures_test.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/mocha/procedures_test.js b/tests/mocha/procedures_test.js index c78ce3831e8..6d7888a9f2e 100644 --- a/tests/mocha/procedures_test.js +++ b/tests/mocha/procedures_test.js @@ -300,7 +300,6 @@ suite('Procedures', function() { }); test('callreturn with missing args', function() { - // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something @@ -318,7 +317,6 @@ suite('Procedures', function() { assertCallBlockStructure(callBlock, [], [], 'do something2'); }); test('callreturn with bad args', function() { - // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something @@ -339,7 +337,6 @@ suite('Procedures', function() { callBlock, ['y'], [this.TEST_VAR_ID], 'do something2'); }); test('callnoreturn with missing args', function() { - // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something @@ -357,7 +354,6 @@ suite('Procedures', function() { assertCallBlockStructure(callBlock, [], [], 'do something2'); }); test('callnoreturn with bad args', function() { - // TODO: How do we want it to behave in this situation? const defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(` do something