Added Field Value Tests #2459

BeksOmega · 2019-05-11T16:00:40Z

The basics

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

The details

Resolves

Work on #2169 specifically this.

Proposed Changes

Adds tests to document how fields should react to different values.

Also clarifies which fields have default values and which do not.

Reason for Changes

Tests needed to be added before #2441 possibly breaks the world.

Test Coverage

This is tests silly!

Additional Information

So... a lot of these tests are actually failing! But that's not because the tests are broken, it's because the fields are broken.

Examples:

Most validator tests fail because atm most fields don't validate unless they are attached to a block. (link)
All checkbox field tests are failing because checkboxes don't handle their text_ properly.
Lots of fields have less severe checks on setValue than on their constructors, so several setValue tests are failing (e.g. most fields fail the non-parsable string test).

Plus a smattering of other field-specific bugs.

Important information

I have some TODO notes in the tests, these are requests for information on what the intended behavior is.

For example, most fields will revert to a default value (meaning the value they would have had had they been constructed no parameters) when passed a non-parsable value. I believe this is a bug, and non-parsable values should validated to null inside a field's doClassValidation_ function, but I wanted confirmation.

RoboErikG

Still need to work through the test cases, but wanted to ask why you're changing the behavior in labels and text inputs.

RoboErikG · 2019-05-13T23:01:23Z

core/field_label.js

 * @param {string=} opt_class Optional CSS class for the field's text.
 * @extends {Blockly.Field}
 * @constructor
 */
 Blockly.FieldLabel = function(text, opt_class) {
+  if (!text) {


This will be false for the empty string as well, which I don't think is a behavior change you intended to make.

In general, why are you adding this check? It looks like setText will handled it by casting to a String if it's not null. Is it a problem with validators?

You're right, Fixed =)

RoboErikG · 2019-05-13T23:02:00Z

core/field_textinput.js

@@ -46,6 +46,9 @@ goog.require('goog.userAgent');
 * @constructor
 */
 Blockly.FieldTextInput = function(text, opt_validator) {
+  if (!text) {


Same issue, and empty string should be a valid starting point for text input fields.

RoboErikG

Reviewed Angle and Checkbox.

This is really thorough. Impressive work. =)
In general, we should probably fix any fields that reset to the default when they get bad input. I'll file an issue.

RoboErikG · 2019-05-14T18:16:14Z

tests/mocha/field_angle_test.js

+    test('Value -> Undefined', function() {
+      var angleField = new Blockly.FieldAngle(1);
+      angleField.setValue(undefined);
+      // TODO: Is this how we want this to work?


I think this is a bug in field angle. Should probably be treated the same as null.

RoboErikG · 2019-05-14T18:16:59Z

tests/mocha/field_angle_test.js

+    test('Value -> Non-Parsable String', function() {
+      var angleField = new Blockly.FieldAngle(1);
+      angleField.setValue('bad');
+      // TODO: Is this how we want this to work?


Ditto. Should probably not change the value if the input is invalid.

RoboErikG · 2019-05-14T18:20:25Z

tests/mocha/field_checkbox_test.js

+    var actualValue = checkboxField.getValue();
+    var actualText = checkboxField.getText();
+    assertEquals(actualValue, expectedValue);
+    assertEquals(actualText, expectedText);


+1 for checking both the value and the text. That'll make supporting screen readers easier.

RoboErikG · 2019-05-14T18:22:28Z

tests/mocha/field_checkbox_test.js

+        assertValue(checkboxField, 'TRUE', 'true');
+      });
+    });
+    suite('Always True Validator', function() {


s/True/False

RoboErikG · 2019-05-14T18:32:51Z

Filed #2462 for to track bad value handling in fields.

RoboErikG

colour and date reviewed.

RoboErikG · 2019-05-14T19:57:50Z

tests/mocha/field_colour_test.js

+      var colourField = new Blockly.FieldColour('bad');
+      assertValueDefault(colourField);
+    });
+    // TODO: Do we want the colour to convert to uppercase or to lowercase?


lowercase sgtm.

RoboErikG · 2019-05-14T20:26:25Z

tests/mocha/field_colour_test.js

+  });
+  suite('setValue', function() {
+    test('#ffffff -> Null', function() {
+      var colourField = new Blockly.FieldColour('#ffffff');


Creating the colourField can be moved into setup for this suite. Also, you should use a value other than the default so you can distinguish when it's falling back to the default vs the current value.

RoboErikG · 2019-05-14T20:26:49Z

tests/mocha/field_colour_test.js

+  suite('Validators', function() {
+    var colourField;
+    setup(function() {
+      colourField = new Blockly.FieldColour('#ffffff');


Use a colour string other than the default so you can distinguish default vs previous value.

RoboErikG · 2019-05-14T20:29:58Z

tests/mocha/field_colour_test.js

+    assertEquals(actualText, expectedText);
+  }
+  function assertValueDefault(colourField) {
+    var expectedValue = Blockly.FieldColour.COLOURS[0];


For testing consistency you might want to change Blockly.FieldColour.COLOURS to a specific set of colours in setup and change it back in teardown.

RoboErikG · 2019-05-14T20:36:33Z

tests/mocha/field_date_test.js

+    var actualValue = dateField.getValue();
+    var actualText = dateField.getText();
+    var stringExpectedValue = String(expectedValue);
+    assertEquals(String(actualValue), stringExpectedValue);


Why do you compare the values as strings?

RoboErikG · 2019-05-14T20:45:49Z

tests/mocha/field_date_test.js

+      assertValue(dateField, '2020-02-20');
+    });
+    test('Invalid Date - Month(2020-13-20)', function() {
+      // TODO: What do we want it to do in this case?


Test as written seems reasonable to me.

RoboErikG · 2019-05-14T20:46:00Z

tests/mocha/field_date_test.js

+      assertValueDefault(dateField);
+    });
+    test('Invalid Date - Day(2020-02-32)', function() {
+      // TODO: What do we want it to do in this case?


RoboErikG · 2019-05-14T20:47:15Z

tests/mocha/field_date_test.js

+    test('Value -> Undefined', function() {
+      var dateField = new Blockly.FieldDate('2020-02-20');
+      dateField.setValue(undefined);
+      // TODO: Is this how we want this to work?


As above, probably want it to leave the current value instead of reverting to the default.

rachel-fenichel · 2019-05-14T21:59:01Z

For tests that fail now because the fields do the wrong thing--you can write them as test.skip('Name', function() {}); or suite.skip('Name', function() {});

The tests will show up on the index page, but won't be run. Then you can enable them as you fix the bugs.

RoboErikG

Reviewed remaining test files.

RoboErikG · 2019-05-16T16:40:33Z

core/field_image.js

@@ -48,11 +48,20 @@ Blockly.FieldImage = function(src, width, height,
    opt_alt, opt_onClick, opt_flipRtl) {
  this.sourceBlock_ = null;

+  if (!src) {


This is a change in behavior. I think there's a valid use case for initializing src to null or an empty string and setting it later, so you should probably revert this and let the image element handle the error.

RoboErikG · 2019-05-16T16:43:26Z

tests/mocha/field_dropdown_test.js

+  function assertValue(dropdownField, expectedValue, expectedText) {
+    var actualValue = dropdownField.getValue();
+    var actualText = dropdownField.getText();
+    assertEquals(String(actualValue), expectedValue);


Why the cast to string?

RoboErikG · 2019-05-16T16:49:26Z

tests/mocha/field_image_test.js

+    });
+    test.skip('Without Alt', function() {
+      var imageField = new Blockly.FieldImage('src', 1, 1);
+      // TODO: currently getText returns undefined, is that what we want?


It probably makes more sense to have it default to an empty string like you're testing for. =)

RoboErikG · 2019-05-16T16:51:56Z

tests/mocha/field_label_serializable_test.js

+  });
+  suite('setValue', function() {
+    test('Null', function() {
+      var labelField = new Blockly.FieldLabelSerializable('value');


pull out into setup for the setValue suite.

RoboErikG · 2019-05-16T16:52:42Z

tests/mocha/field_label_test.js

+  });
+  suite('setValue', function() {
+    test('Null', function() {
+      var labelField = new Blockly.FieldLabel('value');


pull out into setup method

RoboErikG · 2019-05-16T17:02:57Z

tests/mocha/field_number_test.js

+      });
+      test('When Not Editing', function() {
+        numberFieldField.setValue(2);
+        assertValue(numberFieldField, 2);


Shouldn't a null validator cause the value to be reverted?

RoboErikG · 2019-05-16T17:04:43Z

tests/mocha/field_textinput_test.js

+  });
+  suite('setValue', function() {
+    test('Null', function() {
+      var textInputField = new Blockly.FieldTextInput('value');


pull out into setup?

RoboErikG · 2019-05-16T17:11:36Z

tests/mocha/field_variable_test.js

@@ -8,54 +8,46 @@ suite('Variable Fields', function() {
      }


Looks like we forgot to add a license header. Would you mind adding one?

RoboErikG · 2019-05-16T17:17:03Z

tests/mocha/field_variable_test.js

+    assertNotNull(actualText);
+    assertNotUndefined(actualText);
+  }
+  function assertValueName(variableField, expectedText) {


Ideally you'd want to assert the id and the text are as expected. You may need to replace the method that generates ids for variables with a mock to do this, though. Specificially getOrCreateVariablePackage.

I ended up stubbing Blockly.utils.genUid and Blockly.Variables.generateUniqueName instead. It should work the same.

RoboErikG · 2019-05-16T17:38:39Z

tests/mocha/field_variable_test.js

+        { variable:variableName }), workspace);
+  }
+  function assertValueDefault(variableField) {
+    var actualValue = variableField.getValue();


From code, the default name will be 'i' and the default value will be a random UUID. It might be worth mocking some of the methods in Blockly.Variables so you can control the defaults and make the tests for default clearer.

Note: It looks like sinon is the proper way to do mocking in mocha.

Example: https://github.com/google/blockly/blob/develop/tests/mocha/event_test.js#L84
Docs: https://sinonjs.org/

BeksOmega · 2019-05-16T20:53:56Z

@RoboErikG should be good to go.

RoboErikG

Looks pretty good to me. A few final comments and I think Rachel wanted to glance over it too.

This was a lot of work. Thanks for doing it so thoroughly and thoughtfully, Beks!

RoboErikG · 2019-05-16T20:56:16Z

core/field_image.js

 * @param {string=} opt_alt Optional alt text for when block is collapsed.
 * @param {Function=} opt_onClick Optional function to be called when the image
 *     is clicked.  If opt_onClick is defined, opt_alt must also be defined.
 * @param {boolean=} opt_flipRtl Whether to flip the icon in RTL.
 * @extends {Blockly.Field}
 * @constructor
 */
-Blockly.FieldImage = function(src, width, height,
+Blockly.FieldImage = function(opt_src, width, height,


We typically only label something opt if you can leave it out of the list of parameters. Since it's before width and height you have to supply something, but it's allowed to be null.

RoboErikG · 2019-05-16T20:58:05Z

tests/mocha/field_label_test.js

+      assertValue(labelField, '1');
+    });
+    test('Boolean', function() {
+      var labelField = new Blockly.FieldLabel(true);


I just realized, you're testing for true but not for false. Will false fail since you've updated the code to replace falsey things with an empty string?

RoboErikG · 2019-05-16T21:04:57Z

core/field_label.js

 * @param {string=} opt_class Optional CSS class for the field's text.
 * @extends {Blockly.Field}
 * @constructor
 */
 Blockly.FieldLabel = function(text, opt_class) {
  this.size_ = new goog.math.Size(0, 17.5);
  this.class_ = opt_class;
-  this.setValue(text);
+  this.setValue(text || '');


I didn't think enough about this before. In your tests you require 1 -> '1' and true -> 'true', but not 0 -> '0' and false -> 'false' . It looks like those values will result in the empty string instead.

Here and elsewhere can we update this to explicitly check for null/undefined instead and let the majority of things be auto-cast to strings?

Fixed for all applicable fields & added tests to cover the changes.

rachel-fenichel · 2019-05-17T18:51:24Z

I'm taking a look now, will comment when I'm done. @RoboErikG thanks for the thorough review, and thanks @BeksOmega for the thorough tests!

rachel-fenichel

I have one comment on a specific test, and one comment on structure.

rachel-fenichel · 2019-05-17T19:05:40Z

tests/mocha/field_angle_test.js

+      assertValue(angleField, 1);
+    });
+    test('Value -> >360°', function() {
+      var angleField = new Blockly.FieldAngle(1);


Set initial value to 2 to make sure it changed.

rachel-fenichel · 2019-05-17T19:08:08Z

tests/mocha/field_angle_test.js

+    });
+  });
+  suite.skip('Validators', function() {
+    var angleField;


Why is angleField defined here instead of in setup?

What I expected to see:

setup(function() { this.angleField = new Blockly.FieldAngle(1); ... }); test('Test name', function() { this.angleField.isBeingEdited_ = true; ... });

rachel-fenichel · 2019-05-17T22:03:55Z

I'm happy with this! @RoboErikG please confirm and then merge.

RoboErikG

Confirmed lgtm.

RoboErikG reviewed May 13, 2019

View reviewed changes

RoboErikG reviewed May 14, 2019

View reviewed changes

RoboErikG reviewed May 16, 2019

View reviewed changes

Added field value tests.

c160964

BeksOmega force-pushed the tests/setValue branch from c8a5eee to c160964 Compare May 16, 2019 20:53

RoboErikG reviewed May 16, 2019

View reviewed changes

BeksOmega added 7 commits May 16, 2019 14:58

Fixed field image src param.

4a13f20

Fixed falsy values with label fields.

264d1da

Fixed falsy values with text input fields.

5f4b5df

Fixed some angle field tests.

2c6fd86

Fixed other text input when editing tests.

4c8de89

Fixed colour tests.

3afb88e

Cleaned up some number and variable field tests.

bcecfe3

BeksOmega added 2 commits May 17, 2019 11:53

Added angle field > 360 degrees tests.

0313dac

Fixed variable validator tests.

d94dc2d

rachel-fenichel reviewed May 17, 2019

View reviewed changes

BeksOmega added 3 commits May 17, 2019 14:37

Split setValue tests into sub-suites.

0a25d39

Fixed angle >360 tests

e1092dd

Changed var declarations to property declarations.

eacc02f

RoboErikG approved these changes May 17, 2019

View reviewed changes

RoboErikG merged commit acd96aa into google:develop May 17, 2019

BeksOmega deleted the tests/setValue branch May 19, 2019 14:21

rachel-fenichel mentioned this pull request Jul 15, 2019

July 2019 release candidate (Q2 2019) #2644

Merged

Added Field Value Tests #2459

Added Field Value Tests #2459

Conversation

BeksOmega commented May 11, 2019

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

Important information

RoboErikG left a 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

RoboErikG left a 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

RoboErikG commented May 14, 2019

RoboErikG left a 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

rachel-fenichel commented May 14, 2019

RoboErikG left a 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 May 16, 2019

RoboErikG left a 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 May 16, 2019 • edited

Choose a reason for hiding this comment

rachel-fenichel commented May 17, 2019

rachel-fenichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachel-fenichel commented May 17, 2019

RoboErikG left a comment

Choose a reason for hiding this comment

BeksOmega May 16, 2019 •

edited