feat: #1792 Migrate 'components/Inputs/' tests to React Testing Library #1891

udayanshevade · 2020-10-09T06:33:29Z

Description

Updates the tests in ‘components/Input/’ to use React Testing Library instead of Enzyme.

More Details

(1) I added comments where I ran into a couple of issues using userEvent and jest-dom matchers. I updated the dependencies to see if it would help (Narrator: it didn’t). But that amounted to just a minor version bump in '@testing-library/react' so I left it in.

(2) I also added some coverage for the Textarea component. I noticed a TODO there about stubbing the Pell editor methods. I can always revert the newly added tests, or make the necessary changes if it should be done a better way.

(3) Also, I was wondering whether to add more coverage to a couple of the Input components. I decided just to migrate the existing ones for starters, but I can definitely add more if time permits.

(4) Lastly, I updated minor details in a couple components themselves. They still work the same (including screenshots below).

Of course, if anything looks out of place (or missing), please LMK and I'll make the necessary changes.

Thanks! ☕

Corresponding Issue

#1792

Screenshots

Switch (updated):

Tag (updated):

Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

udayanshevade · 2020-10-09T06:42:10Z

client/app/components/Input/InputSwitch.jsx

@@ -37,7 +37,7 @@ export function InputSwitch({
    setKey(Utils.randomString());
  };

-  const onKeyPress = (e: SyntheticKeyboardEvent<HTMLInputElement>) => {


Small update: replaces onKeyPress with onKeyDown, should still work without issue

udayanshevade · 2020-10-09T06:43:31Z

client/app/components/Input/InputCheckboxGroup.jsx

@@ -6,7 +6,7 @@ import type { Checkbox } from './utils';
 export type Props = {
  checkboxes: Checkbox[],
  required?: boolean, // At least one checkbox must be checked
-  hasError?: Function,
+  hasError?: (errorPresent: boolean) => void,


Specified the type here to reflect the fix in the mock prop passed to the component in the test.

udayanshevade · 2020-10-09T06:46:03Z

client/app/components/Input/__tests__/InputCheckboxGroup.spec.jsx

@@ -10,7 +10,8 @@ const label = 'Some Label';
 const idTwo = 'some-other-id';
 const nameTwo = 'some-other-name';
 const labelTwo = 'Some Other Label';
-const someEvent = () => {
+const someEvent = (hasError) => {


Minor fix: previously, window.alert would be invoked below even if there was no error. The assertion on line 55 helped catch this issue.

julianguyen

Fantastic work!! :D Thanks so much for taking this on!

I have some questions but overall it looks good to me!

julianguyen · 2020-10-09T15:57:04Z

client/app/components/Input/__tests__/Input.spec.jsx

+
+        expect(input).toBeInTheDocument();
+
+        expect(accordionContainer).toHaveClass(css.accordionClose);


We shouldn't need to test that the styles appear. Our assertions should be on the presence of elements. Can we test that the input element is present instead?

It should be screen.queryByLabelText(label).toBeNull() before toggling the accordion open and then screen.getByLabelText(label). toBeInTheDocument() after.

Absolutely, I agree. Initially I tried going the same route as well.

I found that the previous tests had been checking for the presence of a hierarchy in the DOM, rather than a target element:

wrapper.find('.accordionContent').find('input').exists()

It turns out the input is always present in the DOM (but '.accordionContent' in the parent gets swapped for css.accordionClose):

The visibility gets toggled by the class accordionClose in the parent, rather than removing the input.

So with queryByLabelText I get:

(Aside: I was wondering whether it would be a good idea to ask and revisit the implementation itself, but I felt it was probably better to stay in the scope of the issue.)

That's when I thought to use the more idiomatic toBeVisible matcher from 'jest-dom', which ought to detect whether an element is viewable or accessible to a screen reader.

But there I ran into a possible external issue with styles not updating correctly (leaving a comment there for more visibility), even though the functionality itself seems to work:

So in the end I thought it might be better to revisit. I can definitely keep digging in that direction? Please LMK if I'm missing something!

Ooo it looks like you're right, ok I think what you have in this PR is fine. Then in another PR you can revisit the implementation. Thanks for clarifying!

julianguyen · 2020-10-09T16:04:17Z

client/app/components/Input/__tests__/InputCheckbox.spec.jsx

@@ -18,9 +19,13 @@ describe('InputCheckbox', () => {
    jest.spyOn(window, 'alert');
  });

+  afterEach(() => {
+    jest.clearAllMocks();


Is this necessary?

Oh right! Let me remove those. The tests should work without them. At one point, I think I ran into an issue with a subsequent assertion passing even though it shouldn't (due to a previous call). But then I think I might have resolved it and forgotten to remove these. Should have noted it down

So I went back and double-checked here just to make sure. I think maybe this reset in particular makes sense, as both blocks of tests use the same assertions.

It's possible for the functionality underlying the second block to break (somehow?) and its tests could still pass due to the previous calls from the first block of tests.

julianguyen · 2020-10-09T16:04:35Z

client/app/components/Input/__tests__/InputCheckbox.spec.jsx

+      expect(screen.queryByRole('input')).not.toBeInTheDocument();
+      // Since '@testing-library/react' does not get hidden inputs,
+      // it can be queried directly from the container for this test.
+      const hiddenInput = container.querySelector('input[type="hidden"]');


Good call to put a comment here :)

julianguyen · 2020-10-09T16:05:13Z

client/app/components/Input/__tests__/InputCheckboxGroup.spec.jsx

+
+      // toggle both checkboxes false
+      userEvent.click(checkbox);
+      userEvent.dblClick(otherCheckbox);


Can this just be a regular click?

Oh right I think I should maybe just leave otherCheckbox alone, since it has an initial value of false anyways. This double click essentially just toggles it on and off again 😅 I think I transcribed the former test too literally, where it simulated the onChange prop firing for a false value. Let me change it so that the test just assumes otherCheckbox is false to begin with.

Edit: so just for the record, I ended up adding assertions to ensure the checkboxes are both unchecked as prerequisites for validating whether or not the error handler is invoked.

julianguyen · 2020-10-09T16:05:21Z

client/app/components/Input/__tests__/InputDefault.spec.jsx

@@ -18,9 +19,13 @@ describe('InputDefault', () => {
    jest.spyOn(window, 'alert');
  });

+  afterEach(() => {
+    jest.clearAllMocks();


Is this necessary?

Similarly I think I added this reset as a way to future-proof any extended tests relying on window.alert, which might accidentally pass when they shouldn't because of a separate call.

Should that situation be handled when the time comes? I'm worried someone might roll with a false positive. But that can be caught in code review, so I can still definitely remove this.

julianguyen · 2020-10-09T16:07:27Z

client/app/components/Input/__tests__/InputSwitch.spec.jsx

+      /**
+       * TODO: Follow up on `userEvent.type(inputSwitch, '{enter}')` in v12.1.7.
+       * Temporarily including `fireEvent` from RTL, for which the switch must have focus first:
+       * https://github.com/testing-library/react-testing-library/issues/376#issuecomment-541242684


Thanks for documenting this!

julianguyen · 2020-10-09T16:08:57Z

client/app/components/Input/__tests__/InputTextarea.spec.jsx

@@ -0,0 +1,173 @@
+// @flow
+import { render, screen } from '@testing-library/react';


Thank you for writing tests for this 🏆

julianguyen · 2020-10-09T16:09:41Z

client/app/components/Input/__tests__/InputTextarea.spec.jsx

+        expect(pell.exec).toHaveBeenCalledWith(...expectedArgs);
+      });
+
+      jest.restoreAllMocks();


Is this necessary?

…ch, remove unused act; fix hasError test prop; add more detailed hasError prop flow type

…Each spy on window.alert

…fterEach to reset spy

… spec to RTL, minor: remove unused type prop

…ate for RTL; comment InputSwitch keyDown test

… test cleanup

udayanshevade commented Oct 9, 2020

View reviewed changes

julianguyen reviewed Oct 9, 2020

View reviewed changes

julianguyen approved these changes Oct 9, 2020

View reviewed changes

udayanshevade added 17 commits October 9, 2020 15:11

[1792] migrate InputCheckbox tests to rtl, add afterEach to reset spy

40ccb4b

[1792] migrate InputCheckboxGroup tests to use RTL, reset spy afterEa…

e368b33

…ch, remove unused act; fix hasError test prop; add more detailed hasError prop flow type

[1792] migrate InputError tests to use RTL

9e5068f

[1792] migrate InputLabel tests to use RTL

3351ea3

[1792] migrate InputLocation tests to use RTL

acbebd1

[1792] migrate InputRadioGroup tests to use RTL, remove unused before…

bb0e278

…Each spy on window.alert

[1792] migrate InputSelect tests to use RTL; add further tests; add a…

29ba886

…fterEach to reset spy

[1792] migrate InputSubmit tests to use RTL, add further tests, add a…

c32a903

…fterEach to reset spy

[1792] migrate InputTag tests to use RTL

7a4006c

replace InputSwitch deprecated keypress handler with keydown, migrate…

517ed9a

… spec to RTL, minor: remove unused type prop

[1792] minor: replace deprecated InputTag keypress; minor version upd…

4b6e0e0

…ate for RTL; comment InputSwitch keyDown test

[1792] migrate InputDefault tests to use RTL

0125ef2

[1792] migrate InputPassword tests to use RTL

5405b06

[1792] migrate Input.spec.jsx tests to use RTL

fba1195

[1792] add InputTextarea tests using RTL

21796ca

[1792] small input tests cleanups

3ce9d02

[1792] code review updates: refine checkbox group assertions, cleanup…

030761f

… test cleanup

udayanshevade force-pushed the feat-1792-migrate-inputs-tests-to-rtl branch from be84440 to 030761f Compare October 10, 2020 01:27

udayanshevade merged commit c2aadce into ifmeorg:main Oct 10, 2020

julianguyen added the hacktoberfest-accepted label Oct 10, 2020

udayanshevade mentioned this pull request Oct 11, 2020

Migrate tests in client/app/components/Input to use React Testing Library instead of Enzyme #1792

Closed

udayanshevade deleted the feat-1792-migrate-inputs-tests-to-rtl branch October 18, 2020 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: #1792 Migrate 'components/Inputs/' tests to React Testing Library #1891

feat: #1792 Migrate 'components/Inputs/' tests to React Testing Library #1891

udayanshevade commented Oct 9, 2020

udayanshevade Oct 9, 2020

udayanshevade Oct 9, 2020

udayanshevade Oct 9, 2020

julianguyen left a comment

julianguyen Oct 9, 2020

udayanshevade Oct 9, 2020

julianguyen Oct 9, 2020

julianguyen Oct 9, 2020

udayanshevade Oct 9, 2020

udayanshevade Oct 9, 2020

julianguyen Oct 9, 2020

julianguyen Oct 9, 2020

udayanshevade Oct 9, 2020 •

edited

Loading

julianguyen Oct 9, 2020

udayanshevade Oct 9, 2020

julianguyen Oct 9, 2020

julianguyen Oct 9, 2020

julianguyen Oct 9, 2020


		expect(input).toBeInTheDocument();

		expect(accordionContainer).toHaveClass(css.accordionClose);

		@@ -0,0 +1,173 @@
		// @flow
		import { render, screen } from '@testing-library/react';

feat: #1792 Migrate 'components/Inputs/' tests to React Testing Library #1891

feat: #1792 Migrate 'components/Inputs/' tests to React Testing Library #1891

Conversation

udayanshevade commented Oct 9, 2020

Description

More Details

Corresponding Issue

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianguyen 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

udayanshevade Oct 9, 2020 • edited Loading

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

udayanshevade Oct 9, 2020 •

edited

Loading