fix: always rename caller to legal name (google#6014)

* fix: always rename caller to legal name * fix: enable tests for non-matching callers which overlap names getting renamed * fix: remove TODOs
alschmiedt · Mar 21, 2022 · c430800 · c430800
1 parent 7abf3de
commit c430800
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 20 deletions.
diff --git 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
@@ -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(`
             <block type="procedures_defreturn">
               <field name="NAME">do something</field>
@@ -310,10 +314,9 @@ suite('Procedures', function() {
             '</block>'
         ), 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(`
             <block type="procedures_defreturn">
               <field name="NAME">do something</field>
@@ -330,10 +333,10 @@ suite('Procedures', function() {
             </block>
         `), 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(`
             <block type="procedures_defnoreturn">
               <field name="NAME">do something</field>
@@ -348,10 +351,9 @@ suite('Procedures', function() {
             '</block>'
         ), 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(`
             <block type="procedures_defnoreturn">
               <field name="NAME">do something</field>
@@ -368,7 +370,8 @@ suite('Procedures', function() {
             </block>
         `), 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
@@ -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<string>=} args An array of argument names.
  * @param {Array<string>=} 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;