Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Unit test FormLogin #35

Merged
merged 5 commits into from
Feb 26, 2020
Merged

Unit test FormLogin #35

merged 5 commits into from
Feb 26, 2020

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Feb 25, 2020

part of #21

Comment on lines 66 to 69
if (isProcessing) {
expect(getByText(/login/i)).toBeDisabled();
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is only the case for U2F keys, maybe we should just remove this case from the testing table and treat it separately?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mainly concerned with the return statement, which skips the majority of the test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised

];

test.each`
auth2faType | authProviders | expTexts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tables work nicely where input/expected values are easier to understand and they follow 1-3 expect statements (like math functions, validation rules, etc). In this scenario it is another way around, we have a few cases in the table following a lot of logic with many expects.

Instead, we can have a separate test per each test-case: login with username/password only with OTP, with U2F

test('login with username and password only', () => {
  const onLogin = jest.fn();
  const { getByText, getByPlaceholderText } = render(
    <FormLogin
      title="titleText"
      auth2faType={Auth2faTypeEnum.DISABLED}     
      attempt={{ isFailed: false, isProcessing: false, message: '' }}
      onLogin={onLogin}
    />
  );

  fireEvent.change(getByPlaceholderText('User name'), { target: { value: 'username' }, });
  fireEvent.change(getByPlaceholderText('Password'), { target: { value: '123' }, });
  fireEvent.click(getByText('LOGIN'));

  expect(onLogin).toHaveBeenCalledWith('username', '123', '');
});

test('login with OTP', () => {
  ...
})

test('input validation errors', () => {
  const onLogin = jest.fn();
  const { getByText, getByPlaceholderText } = render(
    <FormLogin
      title="titleText"
      auth2faType={Auth2faTypeEnum.OTP}     
      attempt={{ isFailed: false, isProcessing: false, message: '' }}
      onLogin={onLogin}
    />
  );
  
  fireEvent.click(getByText('LOGIN'));

  expect(onLogin).not.toHaveBeenCalled()
  expect('error text....
  expect('error text....
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored

// test basics are rendered
expect(getByText('titleText')).toBeInTheDocument();
expect(getByText(/username/i)).toBeInTheDocument();
expect(getByText(/password/i)).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for these

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised, i kept one test to see if titleText rendered

}

let usernameLabel = getByText(/username is required/i);
expect(usernameLabel).toHaveStyle({ color: errorColor });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to test styling of InputField component. It would be enough to verify the presence of the error message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised

@kimlisa kimlisa merged commit c2679a1 into master Feb 26, 2020
@kimlisa kimlisa deleted the lisa/test-FormLogin branch February 26, 2020 17:16
pierrebeaucamp pushed a commit that referenced this pull request Mar 26, 2020
…/deployment-updates

* 'master' of github.com:gravitational/webapps: (27 commits)
  [teleport] Receive and display nodeCount and publicURL in cluster table (#52)
  Remove unused imports from makeEvent.ts
  Typescript migration (#51)
  [Teleport] Prompt user with a confirmation window for session tabs (#49)
  Refactor tabs creation to a separate hook and add unit-tests (#50)
  Do not rerender in-active document (#47)
  regenerate dist files
  New Terminal (#46)
  Unit test rest of Dialog*.jsx and TopNav*.jsx (#45)
  Read localAuthEnabled config from backend (#44)
  Unit Test Popover (#43)
  Unit test teleport/Login (#40) closes #39
  Test rendering of SideNav, SideNavItem, SideNavItemIcon (#41)
  Unit test featureBase (#38)
  Unit test useStore (#37)
  Unit test FormPassword (#36)
  Unit test FormLogin (#35)
  Unit test FieldSelect (#34)
  Test useRule unsubscribe behavior and some cleanup (#33)
  Unit test FieldInput (#32)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants