feat(login): Login form unit test #64

fluturecode · 2021-03-23T01:32:08Z

Tied to Feature Set #70

Changes

Created unit test for Login Form (LoginForm.tsx) with React-Testing-Library.
Created Login Form component (LoginForm.test.tsx) with React-Hook-Form.
Added a Home Page component to pull in the Login Form.
Added form validation with Yup.

Build unit test for Login form.
Build login form.
Add form validation.
Check that validation refactor passes all unit tests.

Purpose

To add unit testing to the login form component.
To create the basic login form through TDD.

Approach

There was no login form previously.

Learning

Researched libraries for forms as well as validation. Chose to use React-Hook-Form and Yup. For more information '(https://react-hook-form.com/)' '(https://github.com/jquense/yup)'

Testing

Pull in the changes to your local copy of this branch.
Run [yarn test]
Check that all tests have passed.
Run [yarn start]
Check that the Login form renders in the browser.

Screenshots

Closes #70

…orm.tsx

thomascking · 2021-04-06T16:52:09Z

src/components/login/LoginForm.test.tsx

+          userEvent.click(submitButton)
+        })
+
+        expect(mockOnSubmit).not.toHaveBeenCalled()


I think it might be worth ensuring that the validation messages are correct as well.

@thomascking Agreed. This is being refactored, and will be pushed back up, moving the validators outside this component, as well as clarifying the messages.

@thomascking Great suggestion. I refactored the code to move schema validation into a separate test file and added the following tests for the LoginForm component:

check that validation messages are not displayed when inputs are valid

check that validation messages are displayed when inputs are invalid

check that only email validation message is displayed when email is valid but password is valid

check that only password validation message is displayed when password is invalid but email is valid}

thomascking · 2021-04-06T16:53:41Z

src/components/login/LoginForm.tsx

+      <label htmlFor="username">
+        <span>Username</span>
+        <input id="username" type="text" name={LoginKey.username} ref={register} />
+      </label>
+      {errors.username && <p role="alert">{errors.username.message}</p>}


This code is very similar to the below code, maybe a FormControl component would make sense here.

@thomascking I'm new to working with React-hook-form, but it does offer a 'Controller' class that I'll try to implement as well which should address your comment.

thomascking · 2021-04-06T16:55:31Z

src/components/signup/SignUpForm.tsx

@@ -0,0 +1,9 @@
+import React from 'react'
+
+const SignUpForm = (): JSX.Element => (


For consistency, might be useful to type the function React.FC instead of the return type.

@thomascking I'm exploring working with both, as well as seeing how the linting and typescript types work. Still deciding the best way of doing things, but you're right I should stick to one throughout. However, that might be a refactor later on as we are still trying to test different things in the early stage. Thanks for pointing that out.

thomascking · 2021-04-06T16:56:42Z

tsconfig.json

-      "dom.iterable",
-      "esnext"
-    ],
+    "lib": ["dom", "dom.iterable", "esnext"],


I'm sure this is probably prettier or the like, but it is somewhat annoying that it made something multi-line earlier and made this single line.

@thomascking It's definitely annoying. Will figure out how to change that setting.

…tead of each individual test

codysmith287 · 2021-04-07T23:53:57Z

src/components/login/LoginForm.test.tsx

+  beforeEach(() => {
+    render(<LoginForm onSubmit={mockOnSubmit} />)
+    usernameField = screen.getByLabelText(/username/i)
+    passwordField = screen.getByLabelText(/password/i)
+    submitButton = screen.getByRole('button', {
+      name: /submit/i,
+    })
+    mockOnSubmit.mockReset()
+  })


Great job keeping the code DRY here.

codysmith287 · 2021-04-08T00:30:20Z

src/components/login/LoginForm.test.tsx

+      // screen.getAllByRole should throw an error if no matching elements are found.
+      // For some reason, `expect(getAllByRole('alert')).toThrow()` would not work,
+      // but it seems to work if I manually catch the error.
+      try {
+        screen.getAllByRole('alert')
+      } catch (error) {
+        expect(error).toBeDefined()
+      }


This test confused me a little at first. I would clarify that errors have the role of 'alert' so screen.getAllByRole('alert') throws an error because there are no actual errors on the page. Not a big deal at the moment.

Yeah, I was trying to verify that no error messages are displayed when the inputs are valid. It's expected behavior for getAllByRole to throw an error because no validation errors are on the page, but usually you assert that by chaining a .toThrow assertion after expect. In this case, that wouldn't work. toThrow wouldn't catch the error and it would cause the test to fail. This is a workaround until I figure out how to clean it up.

codysmith287 · 2021-04-08T00:31:52Z

src/components/login/LoginForm.tsx

+    <form onSubmit={onSubmit}>
+      <label htmlFor="username">
+        <span>Username</span>
+        <input id="username" type="text" name="username" ref={register} />
+      </label>
+      {errors.username && <span role="alert">{errors.username.message}</span>}
+      <label htmlFor="password">
+        <span>Password</span>
+        <input id="password" type="password" name="password" ref={register} />
+      </label>
+      {errors.password && <span role="alert">{errors.password.message}</span>}
+      <button type="submit">Submit</button>
+    </form>


Nice null guarding here.

Another way you could null guard since we're using typescript would be errors?.username?.message so it would just be

<span role="alert">{errors?.username?.message}</span>

What you have is good though so no need to change it.

@codysmith287 That's a great point. I'm going to add that change.

Actually never mind in this case, that throws off your tests because the span with role="alert" still appears.

@codysmith287 oh you're right on that!

codysmith287

Great work getting some solid tests going! Everything looks great, just had one comment on a test that was a little confusing, but definitely something that can be addressed later.

coreyshuman

This is a super strong start to the project! Typing, unit tests, small components... it's got it all!

coreyshuman · 2021-04-08T00:28:16Z

src/components/login/LoginForm.test.tsx

+      try {
+        screen.getAllByRole('alert')
+      } catch (error) {
+        expect(error).toBeDefined()


We might want to be more specific on this check, since lots of things could create an exception to be thrown. We want to make sure we see the right error here.

It looks like the official React Testing Library docs recommend using queryBy* methods instead to test that elements are not present since they don't throw an error. So we should be testing this condition as

expect(screen.queryAllByRole('alert')).toHaveLength(0)

This makes sense since the error thrown is an internal implementation of the testing library and making our test dependent on that error type can break our tests if they change the implementation.

Yeah that makes sense, good catch!

coreyshuman · 2021-04-08T00:30:39Z

src/components/login/LoginForm.test.tsx

+
+        expect(screen.getAllByRole('alert')).toHaveLength(1)
+        expect(screen.getByRole('alert').innerHTML).toBe(Constants.validationMessages.username.MUST_BE_VALID_EMAIL)
+      })


Very nice, this is good specificity on this test. And the use of the constant will allow us to change the error message without rewriting the test. I love it!

coreyshuman · 2021-04-08T00:37:53Z

src/components/login/LoginForm.tsx

+  const { register, handleSubmit, errors } = useForm<ILoginFormData>({
+    resolver: yupResolver(loginFormSchema),
+  })
+
+  const onSubmit = handleSubmit((formData: ILoginFormData) => {
+    props.onSubmit(formData.username, formData.password)
+  })
+
+  return (
+    <form onSubmit={onSubmit}>
+      <label htmlFor="username">
+        <span>Username</span>
+        <input id="username" type="text" name="username" ref={register} />
+      </label>
+      {errors.username && <span role="alert">{errors.username.message}</span>}
+      <label htmlFor="password">
+        <span>Password</span>
+        <input id="password" type="password" name="password" ref={register} />
+      </label>
+      {errors.password && <span role="alert">{errors.password.message}</span>}
+      <button type="submit">Submit</button>
+    </form>
+  )


Just a recommendation for a future refactor once we have multiple forms. We're going to have this structure very regularly: useForm, submitHandler, form HTML. If it makes sense, we can create a generic
component to wrap this common functionality, and pass into it the names/types of fields and buttons. It would basically be a form builder.

I think this suggestion will make more sense and bring value once we get to the point of adding locale and translation support. Then we can build out that logic into one reusable form component, instead of needing to refactor every individual component which has a form in it.

coreyshuman · 2021-04-08T00:41:28Z

src/components/login/validation.test.ts

+  const validUsername = '[email protected]'
+  const validPassword = 'password'
+
+  const testValidInput = {


Nitpicking, but I would add the ILoginFormData interface to all of these test case objects. If the implementation (and interface) were to change later, this makes the refactor easier and also lets the developer know the test needs attention before even running it.

coreyshuman · 2021-04-08T00:51:23Z

src/utils/constants.ts

+  patterns: {
+    DIGIT_REGEX: /[0-9]/,
+    // eslint-disable-next-line no-useless-escape
+    EMAIL_REGEX: /^[a-z0-9!#$%&'*+\/=?^_\`{|}~.-]+@[a-z0-9]([a-z0-9-])+(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/i,


I would recommend we have a set of passing and failing email formats, and iterate tests across them to verify the Yup format validator behaves in a way that we understand and agree with.

For context, client-side email validation is a touchy subject. Check out this S&P discussion for context (and some email samples to test against):
Shift3/standards-and-practices#39

… present

fluturecode · 2021-04-08T18:34:47Z

Closes #64

fluturecode · 2021-04-08T18:35:15Z

Closes #64

fluturecode added 3 commits March 22, 2021 15:40

feat(login):login form test

c13e1bf

feat(login):removed errors from App.tsx & updated interface in LoginF…

51b421d

…orm.tsx

feat(login):moved test into loginForm component and updated eslintrc.js

b602e59

fluturecode added Feature This is a feature from the scope of work feature sets. test Code tests only labels Mar 23, 2021

fluturecode self-assigned this Mar 23, 2021

fluturecode changed the title ~~Ee 63 login form test~~ feat(login): Login form unit test Mar 23, 2021

fluturecode requested review from codysmith287 and AramayisO March 23, 2021 01:35

fluturecode added 20 commits March 24, 2021 16:28

feat(login):added all tests for login form

0b26333

feat(login):removed comments in LoginForm.test.tsx

5663c4a

feat(lgin):added formik fields to LoginForm.tsx

ae28fc0

feat(login): updated values in LoginForm.tsx

9c61fed

feat(login):updated LoginForm.tsx

66d1ead

feat(login):updated tsconfig.json

1add9b0

feat(login):added prop values to LoginForm.tsx

fde92b3

feat(login):updated LoginForm.tsx;

387d20c

feat(login):added LoginFormSelect.tsx and updated LoginForm.tsx

215e4d9

feat*(login):updated LoginFormSelec.tsx

a32e0dc

feat(login):fixed merge conflicts

ecbbaed

feat(login):added plugin:react-hooks/recommended to .eslintrc.json

9acbbd5

feat(login):added useForm hook to LoginForm.tsx

6158666

feat(login):updated fields in LoginForm.tsx

459b732

feat(login):fixed errors in LoginForm.tsx

fdfb8b1

feat(login):added validation to LoginForm.tsx;

aeff603

feat(login):removed onSubmit from all tests in LoginForm.test.tsx

4511174

feat(login):removed console.log error

209a0d9

feat(login):added Yup detailed validation to LoginForm.tsx

c320706

feat(login):removed classnames from LoginForm.tsx

4005bca

thomascking reviewed Apr 6, 2021

View reviewed changes

fluturecode assigned AramayisO Apr 6, 2021

michaelachrisco approved these changes Apr 6, 2021

View reviewed changes

AramayisO added 2 commits April 6, 2021 16:30

fix(HomePage): remove console log statement exposing login credentials

1c481d8

refactor(LoginForm.test): reset mock function in beforeEach block ins…

c4d4216

…tead of each individual test

AramayisO and others added 2 commits April 7, 2021 02:14

refactor(LoginForm): move validation out of form component

dddebb9

feat(login):updated CODEOWNERS

de1e63a

codysmith287 reviewed Apr 7, 2021

View reviewed changes

codysmith287 reviewed Apr 8, 2021

View reviewed changes

codysmith287 approved these changes Apr 8, 2021

View reviewed changes

coreyshuman approved these changes Apr 8, 2021

View reviewed changes

fluturecode force-pushed the ee-63-login-form-test branch from 389ace8 to de1e63a Compare April 8, 2021 00:56

AramayisO added 3 commits April 8, 2021 09:47

test(LoginForm): use queryBy* methods to check that an element is not…

2a39071

… present

test(LoginForm/validation): add ILoginFormData typing to all test data

16dc56f

test(LoginForm/validation): add description block for valid input test

50b6fb1

fluturecode closed this Apr 8, 2021

fluturecode reopened this Apr 8, 2021

fluturecode merged commit 2f3d27d into development Apr 8, 2021

fluturecode deleted the ee-63-login-form-test branch April 8, 2021 18:35

fluturecode mentioned this pull request Apr 8, 2021

FS1 Login Page Epic LP1 - Add login page fields #13

Closed

klmcca3 unassigned AramayisO Apr 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(login): Login form unit test #64

feat(login): Login form unit test #64

fluturecode commented Mar 23, 2021 •

edited

Loading

thomascking Apr 6, 2021

fluturecode Apr 6, 2021 •

edited

Loading

AramayisO Apr 7, 2021

thomascking Apr 6, 2021

fluturecode Apr 6, 2021

thomascking Apr 6, 2021

fluturecode Apr 6, 2021 •

edited

Loading

thomascking Apr 6, 2021

fluturecode Apr 6, 2021 •

edited

Loading

codysmith287 Apr 7, 2021

codysmith287 Apr 8, 2021

AramayisO Apr 8, 2021

codysmith287 Apr 8, 2021

codysmith287 Apr 8, 2021 •

edited

Loading

fluturecode Apr 8, 2021 •

edited

Loading

codysmith287 Apr 8, 2021 •

edited

Loading

fluturecode Apr 8, 2021 •

edited

Loading

codysmith287 left a comment

coreyshuman left a comment

coreyshuman Apr 8, 2021

AramayisO Apr 8, 2021

coreyshuman Apr 8, 2021

coreyshuman Apr 8, 2021

coreyshuman Apr 8, 2021

coreyshuman Apr 8, 2021

coreyshuman Apr 8, 2021 •

edited

Loading

fluturecode commented Apr 8, 2021

fluturecode commented Apr 8, 2021

		@@ -0,0 +1,9 @@
		import React from 'react'

		const SignUpForm = (): JSX.Element => (

feat(login): Login form unit test #64

feat(login): Login form unit test #64

Conversation

fluturecode commented Mar 23, 2021 • edited Loading

Changes

Purpose

Approach

Learning

Testing

Screenshots

Closes #70

Choose a reason for hiding this comment

fluturecode Apr 6, 2021 • 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

fluturecode Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluturecode Apr 6, 2021 • 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

codysmith287 Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

fluturecode Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

codysmith287 Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

fluturecode Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

codysmith287 left a comment

Choose a reason for hiding this comment

coreyshuman 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

coreyshuman Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

fluturecode commented Apr 8, 2021

fluturecode commented Apr 8, 2021

fluturecode commented Mar 23, 2021 •

edited

Loading

fluturecode Apr 6, 2021 •

edited

Loading

fluturecode Apr 6, 2021 •

edited

Loading

fluturecode Apr 6, 2021 •

edited

Loading

codysmith287 Apr 8, 2021 •

edited

Loading

fluturecode Apr 8, 2021 •

edited

Loading

codysmith287 Apr 8, 2021 •

edited

Loading

fluturecode Apr 8, 2021 •

edited

Loading

coreyshuman Apr 8, 2021 •

edited

Loading