Form tests - Phase 1 #196

chrisronline · 2017-12-07T19:44:12Z

Relates to #177

Adds tests for:

Checkbox
CheckboxGroup
FieldNumber
FieldPassword
FieldSearch
FieldText

For CheckboxGroup, there are two separate test files. This is because for one, we want to mock Checkbox and the other we do not. It's difficult/complicated to handle mocking and unmocking in a single file.

cjcenizal

Sweet! I have some suggestions, including ways we can reduce the size of these snapshots.

cjcenizal · 2017-12-07T20:52:35Z

src/components/form/checkbox/checkbox.test.js

+    });
+
+    describe('label', () => {
+      [(<span>Label</span>)].forEach(value => {


I think we need to change this to "Label" and wrap the value in <span> on line 71, or else the description text for this test outputs [object Object] in the snapshot.

Also, if we're not actually iterating over multiple values, I think it makes sense to make the test simpler and terser. This makes the test more readable and eliminates any confusion that we may intend to support multiple different types of label inputs.

describe('label', () => { test(`is rendered`, () => { const component = render( <EuiCheckbox {...checkboxRequiredProps} label={<span>Label</span>} /> ); expect(component) .toMatchSnapshot(); }); });

Sounds good!

cjcenizal · 2017-12-07T20:58:29Z

src/components/form/checkbox/checkbox.test.js

+
+  describe('props', () => {
+    test('id is required', () => {
+      expect(() => render(


We can remove this render call since we're not generating a snapshot or simulating user interaction with the component.

Nice, good call!

cjcenizal · 2017-12-07T20:58:34Z

src/components/form/checkbox/checkbox.test.js

+    });
+
+    test('onChange is required', () => {
+      expect(() => render(


cjcenizal · 2017-12-07T21:00:57Z

src/components/form/checkbox/checkbox.test.js

+    });
+
+    describe('checked', () => {
+      [true, false].forEach(value => {


For boolean props like this, I've usually just been asserting the true value out of pragmatism, since it seems unlikely we'd break the behavior of this component when the prop is false. Also, since false is the default then we're already implicitly testing this behavior in all of our other tests. This reduces snapshot code a bit. What do you think? Do you think it's worth keeping both assertions?

describe('checked', () => { test(`is rendered`, () => { const component = render( <EuiCheckbox {...checkboxRequiredProps} checked /> ); expect(component) .toMatchSnapshot(); }); });

Let's do it!

cjcenizal · 2017-12-07T21:02:13Z

src/components/form/checkbox/checkbox.test.js

+    onChange: PropTypes.func.isRequired,
+    type: PropTypes.oneOf(TYPES),
+    disabled: PropTypes.bool,
+    */


We can delete this commented-out code.

Ugh I'm trying to not leave these in, but apparently need to do better

cjcenizal · 2017-12-07T21:04:56Z

src/components/form/checkbox/checkbox_group.mocked_checkbox.test.js

+      <EuiCheckboxGroup
+        options={[
+          { id: '1', label: 'kibana', disabled: false },
+          { id: '2', label: 'elastic', disabled: true },


Looks like disabled is a prop for the EuiCheckboxGroup itself. So we can remove it from these options objects and on lines 38-39. We need to add another test for this prop though.

cjcenizal · 2017-12-07T21:05:57Z

src/components/form/checkbox/checkbox_group.mocked_checkbox.test.js

+describe('EuiCheckboxGroupMockedCheckbox', () => {
+  test('is rendered', () => {
+    const component = render(
+      <EuiCheckboxGroup onChange={() => {}} {...requiredProps} id="foobar" />


Do we need to pass in id?

I'm passing it in as an arbitrary property that will fall into the ...rest and ensure they are still rendered. I can use a different property, but I feel that's a valuable thing to ensure

I think you're right that we should be testing that we're able to pass arbitrary props in and they'll pass-through. But that's also what the requiredProps are for, so I think passing those in already fulfills this intention. What do you think?

cjcenizal · 2017-12-07T21:11:55Z

src/components/form/checkbox/checkbox_group.unmocked_checkbox.test.js

@@ -0,0 +1,32 @@
+import React from 'react';


We can use sinon stubs to simplify our assertion:

import React from 'react'; import { mount } from 'enzyme'; import sinon from 'sinon'; import { EuiCheckboxGroup } from './checkbox_group'; // This exists because we need to run the following tests // without mocking the Checkbox component, such as testing // an interaction that is handled by the Checkbox component. describe('EuiCheckboxGroupUnmockedCheckbox', () => { test('onChange receives id of changed checkbox', () => { const onChangeHandler = sinon.stub(); const component = mount( <EuiCheckboxGroup options={[ { id: '1', label: 'kibana', disabled: false }, ]} idToSelectedMap={{ '1': true, }} onChange={onChangeHandler} /> ); component.find('input[type="checkbox"]').simulate('change'); sinon.assert.calledWith(onChangeHandler, '1'); }); });

cjcenizal · 2017-12-07T21:14:40Z

src/components/form/checkbox/checkbox_group.mocked_checkbox.test.js

@@ -0,0 +1,52 @@
+import React from 'react';


Since this file contains our basic set of tests, I think we can treat this file as the "regular" test file and call it checkbox_group.test.js. And then we can treat the other one as the extension beyond these basic tests. Since it's mostly testing behavior in response to user interaction, how about we call it checkbox_group.behavior.test.js?

Sounds good!

cjcenizal · 2017-12-07T21:37:12Z

src/components/form/field_number/field_number.test.js

+  describe('props', () => {
+    describe('isInvalid', () => {
+      [true, false].forEach(value => {
+        test(`${value} is rendered`, () => {


isInvalid will actually call setCustomValidity('Invalid') on the input under the hood (https://github.com/elastic/eui/blob/master/src/components/form/validatable_control/validatable_control.js#L19), so it won't render anything. I think this test should ideally look something like this:

describe('isInvalid', () => { test(`sets custom validity error`, () => { const component = mount( <EuiFieldNumber isInvalid /> ); expect(component.find('input').hostNodes().getDOMNode().validity.customError).toBe(true); }); });

Unfortunately, jsdom doesn't support this API yet so it doesn't work. What do you think of leaving a TODO for now?

describe('isInvalid', () => { test('sets custom validity error', () => { // TODO: Assert that the input's validity.customError is set to true. Blocked by // https://github.com/tmpvar/jsdom/issues/544. What we want to do is: // expect(component.find('input').hostNodes().getDOMNode().validity.customError).toBe(true); }); });

Nice find for sure.

My thoughts are since this is a unit test, we shouldn't need to worry about what the dependencies are doing. We just need to verify that the property is indeed added, which I think the snapshot test does.

This also means we can freely change the implementation details of EuiValidatableControl without needing to update these tests.

Thoughts?

Ahh you're right. Sounds good!

cjcenizal

Looking good! Just had 3 suggestions and 1 question.

cjcenizal · 2017-12-09T00:56:32Z

src/components/form/checkbox/checkbox.test.js

+    });
+
+    describe('disabled', () => {
+      [true, false].forEach(value => {


Can we get away with just testing true?

cjcenizal · 2017-12-09T00:57:24Z

src/components/form/checkbox/checkbox_group.behavior.test.js

+// This exists because we need to run the following tests
+// without mocking the Checkbox component, such as testing
+// an interaction that is handled by the Checkbox component.
+describe('EuiCheckboxGroupUnmockedCheckbox', () => {


Can we rephrase this as "EuiCheckboxGroup behavior" to make it a bit more readable and reflect the file name?

cjcenizal · 2017-12-09T00:59:06Z

src/components/form/checkbox/checkbox_group.test.js

  test('is rendered', () => {
    const component = render(
-      <EuiCheckboxGroup onChange={() => {}} {...requiredProps} />
+      <EuiCheckboxGroup onChange={() => {}} {...requiredProps} id="foobar" />


What were your thoughts on id here? I feel like requiredProps is sufficient for verifying that DOM attributes get passed-through to the element that gets rendered.

Sounds good!

cjcenizal · 2017-12-09T01:00:15Z

src/components/form/checkbox/checkbox_group.test.js

-describe('EuiCheckboxGroup', () => {
+jest.mock('./checkbox', () => ({ EuiCheckbox: 'eui_checkbox' }));
+
+describe('EuiCheckboxGroupMockedCheckbox', () => {


Can we rephrase this as "EuiCheckboxGroup (mocked checkbox)" to make it a bit more readable?

chrisronline · 2017-12-14T14:17:51Z

@cjcenizal Ready for another pass!

cjcenizal

🎸 LGTM!

chrisronline added 9 commits December 7, 2017 09:54

Checkbox tests

e147736

Checkbox group tests

d71fa54

Add comments for mock vs unmock

39212ed

Use a delimiter

1df35cb

Field number tests

bf67fa5

Field password tests

f211fa5

Remove comment

6654bc0

Field search tests

7077992

Field test tests

4beeacc

chrisronline requested a review from cjcenizal December 7, 2017 19:44

cjcenizal mentioned this pull request Dec 7, 2017

Jest test coverage #177

Closed

22 tasks

cjcenizal suggested changes Dec 7, 2017

View reviewed changes

Simplify some tests

934cc11

cjcenizal suggested changes Dec 9, 2017

View reviewed changes

PR feedback

41b26f5

cjcenizal approved these changes Dec 14, 2017

View reviewed changes

chrisronline merged commit 0efd099 into elastic:master Dec 14, 2017

chrisronline deleted the form-tests branch December 14, 2017 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Form tests - Phase 1 #196

Form tests - Phase 1 #196

chrisronline commented Dec 7, 2017

cjcenizal left a comment

cjcenizal Dec 7, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017

chrisronline Dec 8, 2017

cjcenizal Dec 7, 2017 •

edited

Loading

chrisronline Dec 8, 2017

cjcenizal Dec 8, 2017

cjcenizal left a comment

cjcenizal Dec 9, 2017

cjcenizal Dec 9, 2017

cjcenizal Dec 9, 2017

chrisronline Dec 14, 2017

cjcenizal Dec 9, 2017

chrisronline commented Dec 14, 2017

cjcenizal left a comment

Form tests - Phase 1 #196

Form tests - Phase 1 #196

Conversation

chrisronline commented Dec 7, 2017

cjcenizal 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

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

cjcenizal Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal 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

chrisronline commented Dec 14, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Dec 7, 2017 •

edited

Loading