Adding procedure tests and handling procedures instantiated without name (#4428)

* Expand procedure tests and fix bug with default ids

* Add tests

* Remove xml_procedures_test.js and add non-overlapping test cases into procedures_test.js

blocks/procedures.js

@@ -28,7 +28,8 @@ Blockly.Blocks['procedures_defnoreturn'] = {
    * @this {Blockly.Block}
    */
   init: function() {
-    var nameField = new Blockly.FieldTextInput('',
+    var initName = Blockly.Procedures.findLegalName('', this);
+    var nameField = new Blockly.FieldTextInput(initName,
         Blockly.Procedures.rename);
     nameField.setSpellcheck(false);
     this.appendDummyInput()
@@ -405,7 +406,8 @@ Blockly.Blocks['procedures_defreturn'] = {
    * @this {Blockly.Block}
    */
   init: function() {
-    var nameField = new Blockly.FieldTextInput('',
+    var initName = Blockly.Procedures.findLegalName('', this);
+    var nameField = new Blockly.FieldTextInput(initName,
         Blockly.Procedures.rename);
     nameField.setSpellcheck(false);
     this.appendDummyInput()
@@ -592,7 +594,7 @@ Blockly.Blocks['procedures_callnoreturn'] = {
    */
   init: function() {
     this.appendDummyInput('TOPROW')
-        .appendField(this.id, 'NAME');
+        .appendField('', 'NAME');
     this.setPreviousStatement(true);
     this.setNextStatement(true);
     this.setStyle('procedure_blocks');
@@ -873,8 +875,13 @@ Blockly.Blocks['procedures_callnoreturn'] = {
         block.appendChild(mutation);
         var field = Blockly.utils.xml.createElement('field');
         field.setAttribute('name', 'NAME');
-        field.appendChild(Blockly.utils.xml.createTextNode(
-            this.getProcedureCall()));
+        var callName = this.getProcedureCall();
+        if (!callName) {
+          // Rename if name is empty string.
+          callName = Blockly.Procedures.findLegalName('', this);
+          this.renameProcedure('', callName);
+        }
+        field.appendChild(Blockly.utils.xml.createTextNode(callName));
         block.appendChild(field);
         xml.appendChild(block);
         Blockly.Xml.domToWorkspace(xml, this.workspace);
@@ -955,6 +962,7 @@ Blockly.Blocks['procedures_callreturn'] = {
     // Tooltip is set in domToMutation.
     this.setHelpUrl(Blockly.Msg['PROCEDURES_CALLRETURN_HELPURL']);
     this.arguments_ = [];
+    this.argumentVarModels_ = [];
     this.quarkConnections_ = {};
     this.quarkIds_ = null;
     this.previousEnabledState_ = true;

tests/mocha/.eslintrc.json

@@ -10,6 +10,8 @@
         "addBlockTypeToCleanup": true,
         "addMessageToCleanup": true,
         "assertArrayEquals": true,
+        "assertCallBlockStructure": true,
+        "assertDefBlockStructure": true,
         "assertDeprecationWarningCall": true,
         "assertEventEquals": true,
         "assertEventFired": true,

tests/mocha/index.html

@@ -88,6 +88,7 @@
     <script src="names_test.js"></script>
     <script src="navigation_modify_test.js"></script>
     <script src="navigation_test.js"></script>
+    <script src="procedures_test_helpers.js"></script>
     <script src="procedures_test.js"></script>
     <script src="registry_test.js"></script>
     <script src="theme_test.js"></script>
@@ -102,7 +103,6 @@
     <script src="workspace_test.js"></script>
     <script src="workspace_svg_test.js"></script>
     <script src="workspace_comment_test.js"></script>
-    <script src="xml_procedures_test.js"></script>
     <script src="xml_test.js"></script>
     <script src="zoom_controls_test.js"></script>
 

tests/mocha/procedures_test.js

@@ -11,11 +11,14 @@ suite('Procedures', function() {
   setup(function() {
     sharedTestSetup.call(this);
     this.workspace = new Blockly.Workspace();
+    this.workspace.createVariable('preCreatedVar', '', 'preCreatedVarId');
+    this.workspace.createVariable(
+        'preCreatedTypedVar', 'type', 'preCreatedTypedVarId');
   });
   teardown(function() {
     sharedTestTeardown.call(this);
   });
-
+  
   suite('allProcedures', function() {
     test('Only Procedures', function() {
       var noReturnBlock = new Blockly.Block(this.workspace, 'procedures_defnoreturn');
@@ -190,6 +193,180 @@ suite('Procedures', function() {
     });
   });
 
+  suite('Multiple block serialization', function() {
+    function assertDefAndCallBlocks(workspace, noReturnNames, returnNames, hasCallers) {
+      const allProcedures = Blockly.Procedures.allProcedures(workspace);
+      const defNoReturnBlocks = allProcedures[0];
+      chai.assert.lengthOf(defNoReturnBlocks, noReturnNames.length);
+      for (let i = 0; i < noReturnNames.length; i++) {
+        const expectedName = noReturnNames[i];
+        chai.assert.equal(defNoReturnBlocks[i][0], expectedName);
+        if (hasCallers) {
+          const callers =
+              Blockly.Procedures.getCallers(expectedName, workspace);
+          chai.assert.lengthOf(callers, 1);
+        }
+      }
+      const defReturnBlocks = allProcedures[1];
+      chai.assert.lengthOf(defReturnBlocks, returnNames.length);
+      for (let i = 0; i < returnNames.length; i++) {
+        const expectedName = returnNames[i];
+        chai.assert.equal(defReturnBlocks[i][0], expectedName);
+        if (hasCallers) {
+          const callers =
+              Blockly.Procedures.getCallers(expectedName, workspace);
+          chai.assert.lengthOf(callers, 1);
+        }
+      }
+
+      // Expecting def and caller blocks are the only blocks on workspace
+      let expectedCount = noReturnNames.length + returnNames.length;
+      if (hasCallers) {
+        expectedCount *= 2;
+      }
+      const blocks = workspace.getAllBlocks(false);
+      chai.assert.lengthOf(blocks, expectedCount);
+    }
+    suite('no name renamed to unnamed', function() {
+      test('defnoreturn and defreturn', function() {
+        var xml = Blockly.Xml.textToDom(`
+              <xml xmlns="https://developers.google.com/blockly/xml">
+                <block type="procedures_defnoreturn"/>
+                <block type="procedures_defreturn"/>
+              </xml>`);
+        Blockly.Xml.domToWorkspace(xml, this.workspace);
+        assertDefAndCallBlocks(
+            this.workspace, ['unnamed'], ['unnamed2'], false);
+      });
+      test('defreturn and defnoreturn', function() {
+        var xml = Blockly.Xml.textToDom(`
+              <xml xmlns="https://developers.google.com/blockly/xml">
+                <block type="procedures_defreturn"/>
+                <block type="procedures_defnoreturn"/>
+              </xml>`);
+        Blockly.Xml.domToWorkspace(xml, this.workspace);
+        assertDefAndCallBlocks(
+            this.workspace, ['unnamed2'], ['unnamed'], false);
+      });
+      test('callnoreturn (no def in xml)', function() {
+        var xml = Blockly.Xml.textToDom(`
+              <xml xmlns="https://developers.google.com/blockly/xml">
+                <block type="procedures_callnoreturn"/>
+              </xml>`);
+        Blockly.Xml.domToWorkspace(xml, this.workspace);
+        assertDefAndCallBlocks(
+            this.workspace, ['unnamed'], [], true);
+      });
+      test('callreturn (no def in xml)', function() {
+        var xml = Blockly.Xml.textToDom(`
+              <xml xmlns="https://developers.google.com/blockly/xml">
+                <block type="procedures_callreturn"/>
+              </xml>`);
+        Blockly.Xml.domToWorkspace(xml, this.workspace);
+        assertDefAndCallBlocks(
+            this.workspace, [], ['unnamed'], true);
+      });
+      test('callnoreturn and callreturn (no def in xml)', function() {
+        var xml = Blockly.Xml.textToDom(`
+              <xml xmlns="https://developers.google.com/blockly/xml">
+                <block type="procedures_callnoreturn"/>
+                <block type="procedures_callreturn"/>
+              </xml>`);
+        Blockly.Xml.domToWorkspace(xml, this.workspace);
+        assertDefAndCallBlocks(
+            this.workspace, ['unnamed'], ['unnamed2'], true);
+      });
+      test('callreturn and callnoreturn (no def in xml)', function() {
+        var xml = Blockly.Xml.textToDom(`
+              <xml xmlns="https://developers.google.com/blockly/xml">
+                <block type="procedures_callreturn"/>
+                <block type="procedures_callnoreturn"/>
+              </xml>`);
+        Blockly.Xml.domToWorkspace(xml, this.workspace);
+        assertDefAndCallBlocks(
+            this.workspace, ['unnamed2'], ['unnamed'], true);
+      });
+    });
+    suite('caller param mismatch', function() {
+      test.skip('callreturn with missing args', function() {
+        // TODO: How do we want it to behave in this situation?
+        var defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(`
+            <block type="procedures_defreturn">
+              <field name="NAME">do something</field>
+              <mutation>
+                <arg name="x" varid="arg"></arg>
+              </mutation>
+            </block>
+        `), this.workspace);
+        var callBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(
+            '<block type="procedures_callreturn">' +
+            '  <mutation name="do something"/>' +
+            '</block>'
+        ), this.workspace);
+        assertDefBlockStructure(defBlock, true, ['x'], ['arg']);
+        assertCallBlockStructure(callBlock, ['x'], ['arg']);
+      });
+      test.skip('callreturn with bad args', function() {
+        // TODO: How do we want it to behave in this situation?
+        var defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(`
+            <block type="procedures_defreturn">
+              <field name="NAME">do something</field>
+              <mutation>
+                <arg name="x" varid="arg"></arg>
+              </mutation>
+            </block>
+        `), this.workspace);
+        var callBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(`
+            <block type="procedures_callreturn">
+              <mutation name="do something">
+                <arg name="y"></arg>
+              </mutation>
+            </block>
+        `), this.workspace);
+        assertDefBlockStructure(defBlock, true, ['x'], ['arg']);
+        assertCallBlockStructure(callBlock, ['x'], ['arg']);
+      });
+      test.skip('callnoreturn with missing args', function() {
+        // TODO: How do we want it to behave in this situation?
+        var defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(`
+            <block type="procedures_defnoreturn">
+              <field name="NAME">do something</field>
+              <mutation>
+                <arg name="x" varid="arg"></arg>
+              </mutation>
+            </block>
+        `), this.workspace);
+        var callBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(
+            '<block type="procedures_callnoreturn">' +
+            '  <mutation name="do something"/>' +
+            '</block>'
+        ), this.workspace);
+        assertDefBlockStructure(defBlock, false, ['x'], ['arg']);
+        assertCallBlockStructure(callBlock, ['x'], ['arg']);
+      });
+      test.skip('callnoreturn with bad args', function() {
+        // TODO: How do we want it to behave in this situation?
+        var defBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(`
+            <block type="procedures_defnoreturn">
+              <field name="NAME">do something</field>
+              <mutation>
+                <arg name="x" varid="arg"></arg>
+              </mutation>
+            </block>
+        `), this.workspace);
+        var callBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom(`
+            <block type="procedures_callnoreturn">
+              <mutation name="do something">
+                <arg name="y"></arg>
+              </mutation>
+            </block>
+        `), this.workspace);
+        assertDefBlockStructure(defBlock, false, ['x'], ['arg']);
+        assertCallBlockStructure(callBlock, ['x'], ['arg']);
+      });
+    });
+  });
+
   const testSuites = [
     {title: 'procedures_defreturn', hasReturn: true,
       defType: 'procedures_defreturn', callType: 'procedures_callreturn'},
@@ -199,15 +376,30 @@ suite('Procedures', function() {
 
   testSuites.forEach((testSuite) => {
     suite(testSuite.title, function() {
-      setup(function() {
-        this.defType = testSuite.defType;
-        this.callType = testSuite.callType;
-        this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
-        this.defBlock.setFieldValue('proc name', 'NAME');
-        this.callBlock = new Blockly.Block(this.workspace, testSuite.callType);
-        this.callBlock.setFieldValue('proc name', 'NAME');
+      suite('Structure', function() {
+        setup(function() {
+          this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
+          this.defBlock.setFieldValue('proc name', 'NAME');
+        });
+        test('Definition block', function() {
+          assertDefBlockStructure(this.defBlock, testSuite.hasReturn);
+        });
+
+        test('Call block', function() {
+          this.callBlock = new Blockly.Block(
+              this.workspace, testSuite.callType);
+          this.callBlock.setFieldValue('proc name', 'NAME');
+          assertCallBlockStructure(this.callBlock);
+        });
       });
       suite('isNameUsed', function() {
+        setup(function() {
+          this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
+          this.defBlock.setFieldValue('proc name', 'NAME');
+          this.callBlock = new Blockly.Block(
+              this.workspace, testSuite.callType);
+          this.callBlock.setFieldValue('proc name', 'NAME');
+        });
         test('True', function() {
           chai.assert.isTrue(
               Blockly.Procedures.isNameUsed('proc name', this.workspace));
@@ -219,6 +411,11 @@ suite('Procedures', function() {
       });
       suite('rename', function() {
         setup(function() {
+          this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
+          this.defBlock.setFieldValue('proc name', 'NAME');
+          this.callBlock = new Blockly.Block(
+              this.workspace, testSuite.callType);
+          this.callBlock.setFieldValue('proc name', 'NAME');
           sinon.stub(this.defBlock.getField('NAME'), 'resizeEditor_');
         });
         test('Simple, Programmatic', function() {
@@ -323,7 +520,7 @@ suite('Procedures', function() {
 
           defInput.htmlInput_.value = '';
           defInput.onHtmlInputChange_(null);
-          var newDefBlock = new Blockly.Block(this.workspace, this.defType);
+          var newDefBlock = new Blockly.Block(this.workspace, testSuite.defType);
           newDefBlock.setFieldValue('new name', 'NAME');
           chai.assert.equal(
               this.defBlock.getFieldValue('NAME'),
@@ -334,6 +531,13 @@ suite('Procedures', function() {
         });
       });
       suite('getCallers', function() {
+        setup(function() {
+          this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
+          this.defBlock.setFieldValue('proc name', 'NAME');
+          this.callBlock = new Blockly.Block(
+              this.workspace, testSuite.callType);
+          this.callBlock.setFieldValue('proc name', 'NAME');
+        });
         test('Simple', function() {
           var callers =
               Blockly.Procedures.getCallers('proc name', this.workspace);
@@ -341,9 +545,9 @@ suite('Procedures', function() {
           chai.assert.equal(callers[0], this.callBlock);
         });
         test('Multiple Callers', function() {
-          var caller2 = new Blockly.Block(this.workspace, this.callType);
+          var caller2 = new Blockly.Block(this.workspace, testSuite.callType);
           caller2.setFieldValue('proc name', 'NAME');
-          var caller3 = new Blockly.Block(this.workspace, this.callType);
+          var caller3 = new Blockly.Block(this.workspace, testSuite.callType);
           caller3.setFieldValue('proc name', 'NAME');
 
           var callers =
@@ -354,9 +558,9 @@ suite('Procedures', function() {
           chai.assert.equal(callers[2], caller3);
         });
         test('Multiple Procedures', function() {
-          var def2 = new Blockly.Block(this.workspace, this.defType);
+          var def2 = new Blockly.Block(this.workspace, testSuite.defType);
           def2.setFieldValue('proc name2', 'NAME');
-          var caller2 = new Blockly.Block(this.workspace, this.callType);
+          var caller2 = new Blockly.Block(this.workspace, testSuite.callType);
           caller2.setFieldValue('proc name2', 'NAME');
 
           var callers =
@@ -382,9 +586,9 @@ suite('Procedures', function() {
         test('Multiple Workspaces', function() {
           var workspace = new Blockly.Workspace();
           try {
-            var def2 = new Blockly.Block(workspace, this.defType);
+            var def2 = new Blockly.Block(workspace, testSuite.defType);
             def2.setFieldValue('proc name', 'NAME');
-            var caller2 = new Blockly.Block(workspace, this.callType);
+            var caller2 = new Blockly.Block(workspace, testSuite.callType);
             caller2.setFieldValue('proc name', 'NAME');
 
             var callers =
@@ -401,15 +605,22 @@ suite('Procedures', function() {
         });
       });
       suite('getDefinition', function() {
+        setup(function() {
+          this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
+          this.defBlock.setFieldValue('proc name', 'NAME');
+          this.callBlock = new Blockly.Block(
+              this.workspace, testSuite.callType);
+          this.callBlock.setFieldValue('proc name', 'NAME');
+        });
         test('Simple', function() {
           var def =
               Blockly.Procedures.getDefinition('proc name', this.workspace);
           chai.assert.equal(def, this.defBlock);
         });
         test('Multiple Procedures', function() {
-          var def2 = new Blockly.Block(this.workspace, this.defType);
+          var def2 = new Blockly.Block(this.workspace, testSuite.defType);
           def2.setFieldValue('proc name2', 'NAME');
-          var caller2 = new Blockly.Block(this.workspace, this.callType);
+          var caller2 = new Blockly.Block(this.workspace, testSuite.callType);
           caller2.setFieldValue('proc name2', 'NAME');
 
           var def =
@@ -419,9 +630,9 @@ suite('Procedures', function() {
         test('Multiple Workspaces', function() {
           var workspace = new Blockly.Workspace();
           try {
-            var def2 = new Blockly.Block(workspace, this.defType);
+            var def2 = new Blockly.Block(workspace, testSuite.defType);
             def2.setFieldValue('proc name', 'NAME');
-            var caller2 = new Blockly.Block(workspace, this.callType);
+            var caller2 = new Blockly.Block(workspace, testSuite.callType);
             caller2.setFieldValue('proc name', 'NAME');
 
             var def =
@@ -555,6 +766,11 @@ suite('Procedures', function() {
       });
       suite('Mutation', function() {
         setup(function() {
+          this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
+          this.defBlock.setFieldValue('proc name', 'NAME');
+          this.callBlock = new Blockly.Block(
+              this.workspace, testSuite.callType);
+          this.callBlock.setFieldValue('proc name', 'NAME');
           this.findParentStub = sinon.stub(Blockly.Mutator, 'findParentWs')
               .returns(this.workspace);
         });
@@ -764,6 +980,180 @@ suite('Procedures', function() {
           });
         });
       });
+      /**
+       * Test cases for serialization tests.
+       * @type {Array<SerializationTestCase>}
+       */
+      const testCases = [
+        {
+          title: 'Minimal definition',
+          xml: '<block type="' + testSuite.defType + '"/>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.defType + '" id="1">\n' +
+              '  <field name="NAME">unnamed</field>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertDefBlockStructure(block, testSuite.hasReturn);
+              },
+        },
+        {
+          title: 'Common definition',
+          xml:
+              '<block type="' + testSuite.defType + '">' +
+              '  <field name="NAME">do something</field>' +
+              '</block>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.defType + '" id="1">\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertDefBlockStructure(block, testSuite.hasReturn);
+              },
+        },
+        {
+          title: 'With vars definition',
+          xml:
+              '<block type="' + testSuite.defType + '">\n' +
+              '  <mutation>\n' +
+              '    <arg name="x" varid="arg1"></arg>\n' +
+              '    <arg name="y" varid="arg2"></arg>\n' +
+              '  </mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.defType + '" id="1">\n' +
+              '  <mutation>\n' +
+              '    <arg name="x" varid="arg1"></arg>\n' +
+              '    <arg name="y" varid="arg2"></arg>\n' +
+              '  </mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertDefBlockStructure(
+                    block, testSuite.hasReturn, ['x', 'y'], ['arg1', 'arg2']);
+              },
+        },
+        {
+          title: 'With pre-created vars definition',
+          xml:
+              '<block type="' + testSuite.defType + '">\n' +
+              '  <mutation>\n' +
+              '    <arg name="preCreatedVar" varid="preCreatedVarId"></arg>\n' +
+              '  </mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.defType + '" id="1">\n' +
+              '  <mutation>\n' +
+              '    <arg name="preCreatedVar" varid="preCreatedVarId"></arg>\n' +
+              '  </mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertDefBlockStructure(block, testSuite.hasReturn,
+                    ['preCreatedVar'], ['preCreatedVarId']);
+              },
+        },
+        {
+          title: 'With pre-created typed vars definition',
+          xml:
+              '<block type="' + testSuite.defType + '">\n' +
+              '  <mutation>\n' +
+              '    <arg name="preCreatedTypedVar" varid="preCreatedTypedVarId"></arg>\n' +
+              '  </mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.defType + '" id="1">\n' +
+              '  <mutation>\n' +
+              '    <arg name="preCreatedTypedVar" varid="preCreatedTypedVarId"></arg>\n' +
+              '  </mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertDefBlockStructure(block, testSuite.hasReturn,
+                    ['preCreatedTypedVar'], ['preCreatedTypedVarId']);
+              },
+        },
+        {
+          title: 'No statements definition',
+          xml:
+              '<block type="procedures_defreturn">\n' +
+              '  <mutation statements="false"></mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="procedures_defreturn" id="1">\n' +
+              '  <mutation statements="false"></mutation>\n' +
+              '  <field name="NAME">do something</field>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertDefBlockStructure(block, true, [], [], false);
+              },
+        },
+        {
+          title: 'Minimal caller',
+          xml: '<block type="' + testSuite.callType + '"/>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.callType + '" id="1">\n' +
+              '  <mutation name="unnamed"></mutation>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertCallBlockStructure(block);
+              },
+        },
+        {
+          title: 'Common caller',
+          xml:
+              '<block type="' + testSuite.callType + '">\n' +
+              '  <mutation name="do something"/>\n' +
+              '</block>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.callType + '" id="1">\n' +
+              '  <mutation name="do something"></mutation>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertCallBlockStructure(block);
+              },
+        },
+        {
+          title: 'With pre-created vars caller',
+          xml:
+              '<block type="' + testSuite.callType + '">\n' +
+              '  <mutation name="do something">\n' +
+              '    <arg name="preCreatedVar"></arg>\n' +
+              '  </mutation>\n' +
+              '</block>',
+          expectedXml:
+              '<block xmlns="https://developers.google.com/blockly/xml" ' +
+              'type="' + testSuite.callType + '" id="1">\n' +
+              '  <mutation name="do something">\n' +
+              '    <arg name="preCreatedVar"></arg>\n' +
+              '  </mutation>\n' +
+              '</block>',
+          assertBlockStructure:
+              (block) => {
+                assertCallBlockStructure(block, ['preCreatedVar'], ['preCreatedVarId']);
+              },
+        },
+      ];
+      testHelpers.runSerializationTestSuite(testCases);
     });
   });
 });

tests/mocha/procedures_test_helpers.js

@@ -0,0 +1,135 @@
+/**
+ * Asserts that the procedure definition or call block has the expected var
+ * models.
+ * @param {!Blockly.Block} block The procedure definition or call block to
+ *    check.
+ * @param {!Array<string>} varIds An array of variable ids.
+ * @private
+ */
+function assertBlockVarModels(block, varIds) {
+  const expectedVarModels = [];
+  for (let i = 0; i < varIds.length; i++) {
+    expectedVarModels.push(block.workspace.getVariableById(varIds[i]));
+  }
+  chai.assert.sameDeepOrderedMembers(block.getVarModels(), expectedVarModels);
+}
+
+/**
+ * Asserts that the procedure call block has the expected arguments.
+ * @param {!Blockly.Block} callBlock The procedure definition block.
+ * @param {Array<string>=} args An array of argument names.
+ * @private
+ */
+function assertCallBlockArgsStructure_(callBlock, args) {
+  // inputList also contains "TOPROW"
+  chai.assert.equal(callBlock.inputList.length - 1, args.length,
+      'call block has the expected number of args');
+
+  for (let i = 0; i < args.length; i++) {
+    const expectedName = args[i];
+    const callInput = callBlock.inputList[i + 1];
+    chai.assert.equal(callInput.type, Blockly.INPUT_VALUE);
+    chai.assert.equal(callInput.name, 'ARG' + i);
+    chai.assert.equal(callInput.fieldRow[0].getValue(), expectedName,
+        'Call block consts did not match expected.');
+  }
+  chai.assert.sameOrderedMembers(callBlock.getVars(), args);
+}
+
+/**
+ * Asserts that the procedure definition block has the expected inputs and
+ *    fields.
+ * @param {!Blockly.Block} defBlock The procedure definition block.
+ * @param {boolean=} hasReturn If we expect the procedure def to have a return
+ *     input or not.
+ * @param {Array<string>=} args An array of argument names.
+ * @param {Array<string>=} varIds An array of variable ids.
+ * @param {boolean=} hasStatements If we expect the procedure def to have a
+ *     statement input or not.
+ */
+function assertDefBlockStructure(defBlock, hasReturn = false,
+    args = [], varIds = [], hasStatements = true) {
+  if (hasStatements) {
+    chai.assert.isNotNull(defBlock.getInput('STACK'),
+        'Def block should have STACK input');
+  } else {
+    chai.assert.isNull(defBlock.getInput('STACK'),
+        'Def block should not have STACK input');
+  }
+  if (hasReturn) {
+    chai.assert.isNotNull(defBlock.getInput('RETURN'),
+        'Def block should have RETURN input');
+  } else {
+    chai.assert.isNull(defBlock.getInput('RETURN'),
+        'Def block should not have RETURN input');
+  }
+  if (args.length) {
+    chai.assert.include(defBlock.toString(), 'with',
+        'Def block string should include "with"');
+  } else {
+    chai.assert.notInclude(defBlock.toString(), 'with',
+        'Def block string should not include "with"');
+  }
+
+  chai.assert.sameOrderedMembers(defBlock.getVars(), args);
+  assertBlockVarModels(defBlock, varIds);
+}
+
+/**
+ * Asserts that the procedure definition 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.
+ */
+function assertCallBlockStructure(callBlock, args = [], varIds = []) {
+  if (args.length) {
+    chai.assert.include(callBlock.toString(), 'with');
+  } else {
+    chai.assert.notInclude(callBlock.toString(), 'with');
+  }
+
+  assertCallBlockArgsStructure_(callBlock, args);
+  assertBlockVarModels(callBlock, varIds);
+}
+
+/**
+ * Creates procedure definition block using domToBlock call.
+ * @param {!Blockly.Workspace} workspace The Blockly workspace.
+ * @param {boolean=} hasReturn Whether the procedure definition should have
+ *    return.
+ * @param {Array<string>=} args An array of argument names.
+ * @return {Blockly.Block} The created block.
+ */
+function createProcDefBlock(
+    workspace, hasReturn = false, args = []) {
+  const type = hasReturn ?
+      'procedures_defreturn' : 'procedures_defnoreturn';
+  let xml = '<block type="' + type + '">';
+  for (let i = 0; i < args.length; i ++) {
+    xml +=
+        '    <mutation><arg name="' + args[i] + '"></arg></mutation>\n';
+  }
+  xml +=
+      '  <field name="NAME">proc name</field>' +
+      '</block>';
+  return Blockly.Xml.domToBlock(Blockly.Xml.textToDom(xml), workspace);
+}
+
+/**
+ * Creates procedure call block using domToBlock call.
+ * @param {!Blockly.Workspace} workspace The Blockly workspace.
+ * @param {boolean=} hasReturn Whether the corresponding procedure definition
+ *    has return.
+ * @return {Blockly.Block} The created block.
+ */
+function createProcCallBlock(
+    workspace, hasReturn = false) {
+  const type = hasReturn ?
+      'procedures_callreturn' : 'procedures_callnoreturn';
+  return Blockly.Xml.domToBlock(Blockly.Xml.textToDom(
+      '<block type="' + type + '">' +
+      '  <mutation name="proc name"/>' +
+      '</block>'
+  ), workspace);
+}

tests/mocha/xml_procedures_test.js

@@ -1,308 +0,0 @@
-/**
- * @license
- * Copyright 2019 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-
-goog.require('Blockly.Blocks.procedures');
-goog.require('Blockly.Msg');
-
-suite('Procedures XML', function() {
-  setup(function() {
-    sharedTestSetup.call(this);
-  });
-  teardown(function() {
-    sharedTestTeardown.call(this);
-  });
-
-  suite('Deserialization', function() {
-    setup(function() {
-      this.workspace = new Blockly.Workspace();
-
-      this.callForAllTypes = function(func) {
-        var typesArray = [
-          ['procedures_defnoreturn', 'procedures_callnoreturn'],
-          ['procedures_defreturn', 'procedures_callreturn']
-        ];
-
-        for (var i = 0, types; (types = typesArray[i]); i++) {
-          var context = Object.create(null);
-          context.workspace = this.workspace;
-          context.defType = types[0];
-          context.callType = types[1];
-
-          func.call(context);
-
-          this.workspace.clear();
-        }
-      };
-    });
-    teardown(function() {
-      workspaceTeardown.call(this, this.workspace);
-    });
-
-    suite('Definition Blocks', function() {
-      test('Minimal', function() {
-        this.callForAllTypes(function() {
-          var xml = Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '"></block>'
-          );
-          var block = Blockly.Xml.domToBlock(xml, this.workspace);
-
-          // TODO: Is this how you want this to work? or do you want it to
-          //    be 'unnamed'?
-          chai.assert.equal(block.getFieldValue('NAME'), '');
-          chai.assert.isArray(block.arguments_);
-          chai.assert.isEmpty(block.arguments_);
-          chai.assert.isArray(block.argumentVarModels_);
-          chai.assert.isEmpty(block.argumentVarModels_);
-          chai.assert.isNotNull(block.getInput('STACK'));
-        });
-      });
-      // This is like what's in the toolbox.
-      test('Common', function() {
-        this.callForAllTypes(function() {
-          var xml = Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '</block>'
-          );
-          var block = Blockly.Xml.domToBlock(xml, this.workspace);
-
-          chai.assert.equal(
-              block.getFieldValue('NAME'),
-              'do something');
-          chai.assert.isArray(block.arguments_);
-          chai.assert.isEmpty(block.arguments_);
-          chai.assert.isArray(block.argumentVarModels_);
-          chai.assert.isEmpty(block.argumentVarModels_);
-          chai.assert.isNotNull(block.getInput('STACK'));
-        });
-      });
-      test('Arg Vars Pre-Created', function() {
-        this.callForAllTypes(function() {
-          this.workspace.createVariable('x', '', 'arg');
-          var xml = Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '  <mutation>' +
-              '    <arg name="x" varid="arg"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          );
-          var block = Blockly.Xml.domToBlock(xml, this.workspace);
-
-          chai.assert.equal(
-              block.getFieldValue('NAME'),
-              'do something');
-          chai.assert.deepEqual(block.arguments_, ['x']);
-          chai.assert.deepEqual(block.argumentVarModels_,
-              [this.workspace.getVariableById('arg')]);
-          chai.assert.isNotNull(block.getInput('STACK'));
-        });
-      });
-      test('Arg Vars Not Created', function() {
-        this.callForAllTypes(function() {
-          var xml = Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '  <mutation>' +
-              '    <arg name="x" varid="arg"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          );
-          var block = Blockly.Xml.domToBlock(xml, this.workspace);
-
-          chai.assert.equal(
-              block.getFieldValue('NAME'),
-              'do something');
-          chai.assert.deepEqual(block.arguments_, ['x']);
-          chai.assert.deepEqual(block.argumentVarModels_,
-              [this.workspace.getVariableById('arg')]);
-          chai.assert.isNotNull(block.getInput('STACK'));
-        });
-      });
-      // TODO: I don't know a lot about typing vars, and even less out it in
-      //  this context. Is allowing typed vars to be args the correct behavior?
-      test('Arg Vars Pre-Created Typed', function() {
-        this.callForAllTypes(function() {
-          this.workspace.createVariable('x', 'type', 'arg');
-          var xml = Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '  <mutation>' +
-              '    <arg name="x" varid="arg"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          );
-          var block = Blockly.Xml.domToBlock(xml, this.workspace);
-
-          chai.assert.equal(
-              block.getFieldValue('NAME'),
-              'do something');
-          chai.assert.deepEqual(block.arguments_, ['x']);
-          chai.assert.deepEqual(block.argumentVarModels_,
-              [this.workspace.getVariableById('arg')]);
-          chai.assert.isNotNull(block.getInput('STACK'));
-        });
-      });
-      test('Statements False', function() {
-        var xml = Blockly.Xml.textToDom(
-            '<block type="procedures_defreturn">' +
-            '  <field name="NAME">do something</field>' +
-            '  <mutation statements="false"></mutation>' +
-            '</block>'
-        );
-        var block = Blockly.Xml.domToBlock(xml, this.workspace);
-
-        chai.assert.equal(
-            block.getFieldValue('NAME'),
-            'do something');
-        chai.assert.isArray(block.arguments_);
-        chai.assert.isEmpty(block.arguments_);
-        chai.assert.isArray(block.argumentVarModels_);
-        chai.assert.isEmpty(block.argumentVarModels_);
-        chai.assert.isNull(block.getInput('STACK'));
-      });
-      test('Statements True', function() {
-        var xml = Blockly.Xml.textToDom(
-            '<block type="procedures_defreturn">' +
-            '  <field name="NAME">do something</field>' +
-            '  <mutation statements="true"></mutation>' +
-            '</block>'
-        );
-        var block = Blockly.Xml.domToBlock(xml, this.workspace);
-
-        chai.assert.equal(
-            block.getFieldValue('NAME'),
-            'do something');
-        chai.assert.isArray(block.arguments_);
-        chai.assert.isEmpty(block.arguments_);
-        chai.assert.isArray(block.argumentVarModels_);
-        chai.assert.isEmpty(block.argumentVarModels_);
-        chai.assert.isNotNull(block.getInput('STACK'));
-      });
-    });
-    suite('Call Blocks', function() {
-      test('Caller W/ Def', function() {
-        this.callForAllTypes(function() {
-          Blockly.Xml.domToBlock(Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '</block>'
-          ), this.workspace);
-          var callerXML = Blockly.Xml.textToDom(
-              '<block type="' + this.callType + '">' +
-              '  <mutation name="do something"/>' +
-              '</block>'
-          );
-          var block = Blockly.Xml.domToBlock(callerXML, this.workspace);
-
-          chai.assert.equal(
-              block.getFieldValue('NAME'),
-              'do something');
-          chai.assert.isArray(block.arguments_);
-          chai.assert.isEmpty(block.arguments_);
-          // TODO: argumentVarModels_ is undefined for call_return, but
-          //  defined for call_noreturn. Make it defined for both.
-          /* chai.assert.isArray(block.argumentVarModels_);
-          chai.assert.isEmpty(block.argumentVarModels_); */
-        });
-      });
-      // TODO: I couldn't get this test (of creating a definition) to work
-      //  b/c of the events delay.
-      test.skip('Caller No Def', function() {
-        this.callForAllTypes(function() {
-          var callerXML = Blockly.Xml.textToDom(
-              '<block type="' + this.callType + '">' +
-              '  <mutation name="do something"/>' +
-              '</block>'
-          );
-          var block = Blockly.Xml.domToBlock(callerXML, this.workspace);
-
-          chai.assert.equal(
-              block.getFieldValue('NAME'),
-              'do something');
-          chai.assert.isArray(block.arguments_);
-          chai.assert.isEmpty(block.arguments_);
-          // TODO: argumentVarModels_ is undefined for call_return, but
-          //  defined for call_noreturn. Make it defined for both.
-          /* chai.assert.isArray(block.argumentVarModels_);
-          chai.assert.isEmpty(block.argumentVarModels_); */
-          chai.assert.equal(this.workspace.getAllBlocks(false).count, 2);
-        });
-      });
-      test('Caller W/ Params', function() {
-        this.callForAllTypes(function() {
-          Blockly.Xml.domToBlock(Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '  <mutation>' +
-              '    <arg name="x" varid="arg"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          ), this.workspace);
-          var callerXML = Blockly.Xml.textToDom(
-              '<block type="' + this.callType + '">' +
-              '  <mutation name="do something">' +
-              '    <arg name="x"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          );
-          var block = Blockly.Xml.domToBlock(callerXML, this.workspace);
-
-          chai.assert.equal(
-              block.getFieldValue('NAME'),
-              'do something');
-          chai.assert.deepEqual(block.arguments_, ['x']);
-          chai.assert.deepEqual(block.argumentVarModels_,
-              [this.workspace.getVariableById('arg')]);
-        });
-      });
-      // TODO: How do you want it to behave in this situation?
-      test.skip('Caller W/out Params', function() {
-        this.callForAllTypes(function() {
-          Blockly.Xml.domToBlock(Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '  <mutation>' +
-              '    <arg name="x" varid="arg"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          ), this.workspace);
-          var callerXML = Blockly.Xml.textToDom(
-              '<block type="' + this.callType + '">' +
-              '  <mutation name="do something"></mutation>' +
-              '</block>'
-          );
-          // TODO: Remove this when you fix this test.
-          // eslint-disable-next-line no-unused-vars
-          var block = Blockly.Xml.domToBlock(callerXML, this.workspace);
-        });
-      });
-      // TODO: How do you want it to behave in this situation?
-      test.skip('Caller W/ Bad Params', function() {
-        this.callForAllTypes(function() {
-          Blockly.Xml.domToBlock(Blockly.Xml.textToDom(
-              '<block type="' + this.defType + '">' +
-              '  <field name="NAME">do something</field>' +
-              '  <mutation>' +
-              '    <arg name="x" varid="arg"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          ), this.workspace);
-          var callerXML = Blockly.Xml.textToDom(
-              '<block type="' + this.callType + '">' +
-              '  <mutation name="do something">' +
-              '    <arg name="y"></arg>' +
-              '  </mutation>' +
-              '</block>'
-          );
-          // TODO: Remove this when you fix this test.
-          // eslint-disable-next-line no-unused-vars
-          var block = Blockly.Xml.domToBlock(callerXML, this.workspace);
-        });
-      });
-    });
-  });
-});