Added Procedure Tests #2576

BeksOmega · 2019-06-13T23:47:21Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide

The details

Resolves

I think this should cover closing #860

Proposed Changes

Adds tests to start covering procedure behavior.

[EDIT: I also removed the old jsunit tests because they were inaccurate]

Reason for Changes

Procedures were dangerously undocumented.

Test Coverage

Adds tests for:

isNameUsed
rename
getCallers
getDefinition
Mutator / Arguments

Additional Information

Some things are broken, those tests are skipped. Here are two particularly bad ones I took gifs of:

But most things should be pretty easy to fix.

I'll create issues for the skipped tests once I get confirmation that all of them are bad.

Additional Additional Information

I'd also like to create XML tests for the procedures, would you like that in this file, the xml file, a new file, or none file (if you don't think it's useful)?

BeksOmega · 2019-06-13T23:48:30Z

blocks/procedures.js

@@ -164,7 +164,7 @@ Blockly.Blocks['procedures_defnoreturn'] = {
   * @this Blockly.Block
   */
  decompose: function(workspace) {
-    var containerBlock = workspace.newBlock('procedures_mutatorcontainer');
+    /*var containerBlock = workspace.newBlock('procedures_mutatorcontainer');


I had to change this because the initSvg() calls were breaking my tests.

BeksOmega · 2019-06-13T23:53:36Z

blocks/procedures.js

@@ -205,11 +234,7 @@ Blockly.Blocks['procedures_defnoreturn'] = {
      var varName = paramBlock.getFieldValue('NAME');
      this.arguments_.push(varName);
      var variable = this.workspace.getVariable(varName, '');
-      if (variable != null) {


This was added by #2186 to fix #1958, but fields are happy now so this is unnecessary. Tests cover this change.

BeksOmega · 2019-06-14T16:38:25Z

Set the broken mocha test to skip because I'm not sure why the dropdownCreate function looks like this:

options.push([Blockly.Msg['RENAME_VARIABLE'], Blockly.RENAME_VARIABLE_ID]);
  if (Blockly.Msg['DELETE_VARIABLE']) {
    options.push(
        [
          Blockly.Msg['DELETE_VARIABLE'].replace('%1', name),
          Blockly.DELETE_VARIABLE_ID
        ]
    );
  }

It broke because I had to add the msg files to the mocha and it started adding the delete option.

alschmiedt · 2019-06-25T20:34:24Z

XML tests would be great. If you are planning on writing more than 2 or 3 tests then I would create a xml_procedures_test.js file, otherwise I would just put them into the xml_test.js file

alschmiedt · 2019-06-25T17:43:18Z

tests/mocha/procedures_test.js

+ * @license
+ * Blockly Tests
+ *
+ * Copyright 2017 Google Inc.


nit: Change to 2019

alschmiedt · 2019-06-25T17:43:53Z

tests/mocha/index.html

@@ -6,6 +6,16 @@

    <link href="https://unpkg.com/mocha@5.2.0/mocha.css" rel="stylesheet" />
    <script src="../../blockly_uncompressed.js"></script>
+    <script src="../../msg/messages.js"></script>


Do we need to include all these blocks? If we don't could you remove the ones that you are not using?

alschmiedt · 2019-06-25T17:46:15Z

tests/mocha/procedures_test.js

+        context.defType = types[0];
+        context.callType = types[1];
+
+        context.defBlock = new Blockly.Block(this.workspace, types[0]);


Use context.defType instead of types[0]

alschmiedt · 2019-06-25T17:46:55Z

tests/mocha/procedures_test.js

+
+        context.defBlock = new Blockly.Block(this.workspace, types[0]);
+        context.defBlock.setFieldValue(startName, 'NAME');
+        context.callBlock = new Blockly.Block(this.workspace, types[1]);


Same as above

alschmiedt · 2019-06-25T19:22:46Z

tests/mocha/procedures_test.js

+        var callers = Blockly.Procedures.getCallers('name1', this.workspace);
+        chai.assert.equal(callers.length, 1);
+        chai.assert.equal(callers[0], this.callBlock);
+      }, 'name');


Can you ad a bit more detailed comment here?

I wrote down a procedure for how to replicate it, does it sound good to you?

Yup that is great thanks!

alschmiedt · 2019-06-25T19:53:55Z

tests/mocha/procedures_test.js

+          clearVariables.call(this);
+        }, 'name');
+      });
+      test('Set Arg Empty (#1958)', function() {


Add #1958 in a comment instead of in the test case name

alschmiedt · 2019-06-25T20:24:38Z

blocks/procedures.js

@@ -185,7 +185,36 @@ Blockly.Blocks['procedures_defnoreturn'] = {
      paramBlock.oldLocation = i;
      connection.connect(paramBlock.previousConnection);
      connection = paramBlock.nextConnection;
+    }*/
+


Remove commented out code

alschmiedt · 2019-06-25T21:55:37Z

tests/mocha/field_variable_test.js

@@ -68,7 +68,7 @@ suite('Variable Fields', function() {
    sinon.restore();
  });

-  test('Dropdown contains variables', function() {
+  test.skip('Dropdown contains variables', function() {


I think the move here is to update the test to expect 5 instead of 4 since having the delete option is expected behavior.

BeksOmega · 2019-06-25T22:10:45Z

XML tests would be great. If you are planning on writing more than 2 or 3 tests then I would create a xml_procedures_test.js file, otherwise I would just put them into the xml_test.js file

Noted, probably won't get to it today but I have put it in my task management system.

alschmiedt

lgtm

BeksOmega · 2019-06-26T14:39:35Z

@alschmiedt Did you want me to create issues for those skipped tests?

alschmiedt · 2019-06-26T15:51:59Z

Yup that would be great : ) I looked over them and they all seem like valid issues.

BeksOmega commented Jun 13, 2019

View reviewed changes

RoboErikG requested review from NeilFraser and alschmiedt June 21, 2019 20:38

RoboErikG assigned alschmiedt Jun 24, 2019

alschmiedt reviewed Jun 25, 2019

View reviewed changes

BeksOmega added 6 commits June 25, 2019 15:08

Added procedure tests.

85c4e41

Deleted inaccurate jsunit procedure tests.

942c7c2

Added message.js to mocha tests file.

2620a50

Added skipping bad variable test.

427291c

Added all block files to mocha index.

347519b

Fixed review comments.

b34ca2f

BeksOmega force-pushed the fixes/ProcedureRenaming branch from c9eabf9 to b34ca2f Compare June 25, 2019 22:08

alschmiedt approved these changes Jun 25, 2019

View reviewed changes

alschmiedt merged commit 83c5d27 into google:develop Jun 26, 2019

BeksOmega deleted the fixes/ProcedureRenaming branch July 1, 2019 21:45

rachel-fenichel mentioned this pull request Jul 15, 2019

July 2019 release candidate (Q2 2019) #2644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Procedure Tests #2576

Added Procedure Tests #2576

BeksOmega commented Jun 13, 2019 •

edited

Loading

BeksOmega Jun 13, 2019

BeksOmega Jun 13, 2019

BeksOmega commented Jun 14, 2019

alschmiedt commented Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

alschmiedt Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

alschmiedt Jun 25, 2019

BeksOmega Jun 25, 2019

BeksOmega commented Jun 25, 2019

alschmiedt left a comment

BeksOmega commented Jun 26, 2019

alschmiedt commented Jun 26, 2019

Added Procedure Tests #2576

Added Procedure Tests #2576

Conversation

BeksOmega commented Jun 13, 2019 • edited Loading

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

Additional Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeksOmega commented Jun 14, 2019

alschmiedt commented Jun 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeksOmega commented Jun 25, 2019

alschmiedt left a comment

Choose a reason for hiding this comment

BeksOmega commented Jun 26, 2019

alschmiedt commented Jun 26, 2019

BeksOmega commented Jun 13, 2019 •

edited

Loading