From ecd8427a51e9db42a348cbc642a8c478aa37e73f Mon Sep 17 00:00:00 2001 From: John Mitchell Date: Tue, 30 Oct 2018 15:07:45 -0400 Subject: [PATCH 1/5] fix eslint issues with app and conditional redirect tests --- __tests__/tests/App.test.jsx | 2 +- __tests__/tests/ConditionalRedirect.test.jsx | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/__tests__/tests/App.test.jsx b/__tests__/tests/App.test.jsx index 8be9df0e6b..ef0e9fa5f0 100644 --- a/__tests__/tests/App.test.jsx +++ b/__tests__/tests/App.test.jsx @@ -34,4 +34,4 @@ describe('', () => { const login = appWrapper.find(Login); expect(login.length).toBe(0); }); -}); \ No newline at end of file +}); diff --git a/__tests__/tests/ConditionalRedirect.test.jsx b/__tests__/tests/ConditionalRedirect.test.jsx index af241810eb..c437ae6971 100644 --- a/__tests__/tests/ConditionalRedirect.test.jsx +++ b/__tests__/tests/ConditionalRedirect.test.jsx @@ -1,4 +1,4 @@ -import React, { Component } from 'react'; +import React from 'react'; import { Route, Redirect @@ -9,7 +9,11 @@ import ConditionalRedirect from '../../src/components/ConditionalRedirect'; describe('', () => { test('renders Redirect when shouldRedirect is passed truthy func', () => { const truthyFunc = () => true; - const shouldHaveRedirectChild = shallow( truthyFunc()} />); + const shouldHaveRedirectChild = shallow( + truthyFunc()} + /> + ); const redirectChild = shouldHaveRedirectChild.find(Redirect); expect(redirectChild.length).toBe(1); const routeChild = shouldHaveRedirectChild.find(Route); @@ -18,10 +22,14 @@ describe('', () => { test('renders Route when shouldRedirect is passed falsy func', () => { const falsyFunc = () => false; - const shouldHaveRouteChild = shallow( falsyFunc()} />); + const shouldHaveRouteChild = shallow( + falsyFunc()} + /> + ); const routeChild = shouldHaveRouteChild.find(Route); expect(routeChild.length).toBe(1); const redirectChild = shouldHaveRouteChild.find(Redirect); expect(redirectChild.length).toBe(0); }); -}); \ No newline at end of file +}); From 7c97989e8461bbd18deaad5b36a467c62465c578 Mon Sep 17 00:00:00 2001 From: John Mitchell Date: Tue, 30 Oct 2018 15:08:45 -0400 Subject: [PATCH 2/5] add more login page tests --- __tests__/tests/LoginPage.test.jsx | 174 ++++++++++++++++++++++++++--- src/pages/Login.jsx | 25 +++-- 2 files changed, 175 insertions(+), 24 deletions(-) diff --git a/__tests__/tests/LoginPage.test.jsx b/__tests__/tests/LoginPage.test.jsx index af79bcdd88..26af4924de 100644 --- a/__tests__/tests/LoginPage.test.jsx +++ b/__tests__/tests/LoginPage.test.jsx @@ -1,37 +1,185 @@ import React from 'react'; import { MemoryRouter } from 'react-router-dom'; -import { shallow, mount } from 'enzyme'; +import { mount, shallow } from 'enzyme'; import LoginPage from '../../src/pages/Login'; import api from '../../src/api'; -import Dashboard from '../../src/pages/Dashboard'; + +const LOGIN_ERROR_MESSAGE = 'Invalid username or password. Please try again.'; describe('', () => { - let loginWrapper, usernameInput, passwordInput, errorTextArea, submitButton; + let loginWrapper; + let loginPage; + let loginForm; + let usernameInput; + let passwordInput; + let errorTextArea; + let submitButton; + let defaultLogo; - beforeEach(() => { - loginWrapper = mount(); + const findChildren = () => { + loginPage = loginWrapper.find('LoginPage'); + loginForm = loginWrapper.find('form.pf-c-form'); usernameInput = loginWrapper.find('.pf-c-form__group#username TextInput'); passwordInput = loginWrapper.find('.pf-c-form__group#password TextInput'); errorTextArea = loginWrapper.find('.pf-c-form__helper-text.pf-m-error'); submitButton = loginWrapper.find('Button[type="submit"]'); + defaultLogo = loginWrapper.find('TowerLogo'); + }; + + beforeEach(() => { + loginWrapper = mount(); + findChildren(); + }); + + afterEach(() => { + loginWrapper.unmount(); }); test('initially renders without crashing', () => { expect(loginWrapper.length).toBe(1); + expect(loginForm.length).toBe(1); expect(usernameInput.length).toBe(1); + expect(usernameInput.props().value).toBe(''); expect(passwordInput.length).toBe(1); + expect(passwordInput.props().value).toBe(''); expect(errorTextArea.length).toBe(1); + expect(loginPage.state().error).toBe(''); expect(submitButton.length).toBe(1); + expect(submitButton.props().isDisabled).toBe(false); + expect(defaultLogo.length).toBe(1); }); - // initially renders empty username and password fields, sets empty error message and makes submit button not disabled + test('custom logo renders Brand component', () => { + loginWrapper = mount(); + findChildren(); + expect(defaultLogo.length).toBe(0); + }); - // typing into username and password fields (if the component is not unmounting) will clear out any error message + test('state maps to un/pw input value props', () => { + loginPage.setState({ username: 'un', password: 'pw' }); + expect(loginPage.state().username).toBe('un'); + expect(loginPage.state().password).toBe('pw'); + findChildren(); + expect(usernameInput.props().value).toBe('un'); + expect(passwordInput.props().value).toBe('pw'); + }); - // when the submit Button is clicked, as long as it is not disabled state.loading is set to true - // api.login is called with param 1 username and param 2 password - // if api.login returns an error, the state.error should be set to LOGIN_ERROR_MESSAGE, if the error object returned has response.status set to 401. - // regardless of error or not, after api.login returns, state.loading should be set to false + test('updating un/pw clears out error', () => { + loginPage.setState({ error: 'error!' }); + usernameInput.instance().props.onChange('uname'); + expect(loginPage.state().username).toBe('uname'); + expect(loginPage.state().error).toBe(''); + loginPage.setState({ error: 'error!' }); + passwordInput.instance().props.onChange('pword'); + expect(loginPage.state().password).toBe('pword'); + expect(loginPage.state().error).toBe(''); + }); - // if api.isAuthenticated mock returns true, Redirect to / should be rendered -}); \ No newline at end of file + test('submit calls api.login successfully', (done) => { + api.login = jest.fn().mockImplementation(() => { + const loginPromise = Promise.resolve({}); + + // wrapped in timeout so this .then happens after state.loading is set to + // false in the actual .then handler + // + // would be improved by using .finally but that's not available for + // some reason + setTimeout(() => { + loginPromise.then(() => { + expect(loginPage.state().loading).toBe(false); + done(); + }); + }, 50); + + return loginPromise; + }); + expect(loginPage.state().loading).toBe(false); + loginPage.setState({ username: 'unamee', password: 'pwordd', loading: true }); + submitButton.simulate('submit'); + expect(api.login).toHaveBeenCalledTimes(0); + loginPage.setState({ loading: false }); + submitButton.simulate('submit'); + expect(api.login).toHaveBeenCalledTimes(1); + expect(api.login).toHaveBeenCalledWith('unamee', 'pwordd'); + expect(loginPage.state().loading).toBe(true); + }); + + test('submit calls api.login handles 401 error', (done) => { + api.login = jest.fn().mockImplementation(() => { + const err = { + response: { status: 401, message: 'problem' }, + }; + + const loginPromise = Promise.reject(err); + + // wrapped in timeout so this .then happens after state.loading is set to + // false in the actual .then handler + // + // would be improved by using .finally but that's not available for + // some reason + setTimeout(() => { + loginPromise.catch(() => { + expect(loginPage.state().loading).toBe(false); + expect(loginPage.state().error).toBe(LOGIN_ERROR_MESSAGE); + done(); + }); + }, 50); + + return loginPromise; + }); + expect(loginPage.state().loading).toBe(false); + loginPage.setState({ username: 'unamee', password: 'pwordd', loading: true }); + submitButton.simulate('submit'); + expect(api.login).toHaveBeenCalledTimes(0); + loginPage.setState({ loading: false }); + expect(loginPage.state().error).toBe(''); + submitButton.simulate('submit'); + expect(api.login).toHaveBeenCalledTimes(1); + expect(api.login).toHaveBeenCalledWith('unamee', 'pwordd'); + expect(loginPage.state().loading).toBe(true); + }); + + test('submit calls api.login handles 401 error', (done) => { + api.login = jest.fn().mockImplementation(() => { + const err = { + response: { status: 500, message: 'problem' }, + }; + + const loginPromise = Promise.reject(err); + + // wrapped in timeout so this .then happens after state.loading is set to + // false in the actual .then handler + // + // would be improved by using .finally but that's not available for + // some reason + setTimeout(() => { + loginPromise.catch(() => { + expect(loginPage.state().loading).toBe(false); + expect(loginPage.state().error).toBe(''); + done(); + }); + }, 50); + + return loginPromise; + }); + expect(loginPage.state().loading).toBe(false); + loginPage.setState({ username: 'unamee', password: 'pwordd', loading: true }); + submitButton.simulate('submit'); + expect(api.login).toHaveBeenCalledTimes(0); + loginPage.setState({ loading: false }); + expect(loginPage.state().error).toBe(''); + submitButton.simulate('submit'); + expect(api.login).toHaveBeenCalledTimes(1); + expect(api.login).toHaveBeenCalledWith('unamee', 'pwordd'); + expect(loginPage.state().loading).toBe(true); + }); + + test('render Redirect to / when already authenticated', () => { + api.isAuthenticated = jest.fn(); + api.isAuthenticated.mockReturnValue(true); + loginWrapper = shallow(); + const redirectElem = loginWrapper.find('Redirect'); + expect(redirectElem.length).toBe(1); + expect(redirectElem.props().to).toBe('/'); + }); +}); diff --git a/src/pages/Login.jsx b/src/pages/Login.jsx index 2a735dd70c..0ad82ef756 100644 --- a/src/pages/Login.jsx +++ b/src/pages/Login.jsx @@ -42,21 +42,24 @@ class LoginPage extends Component { handlePasswordChange = value => this.safeSetState({ password: value, error: '' }); handleSubmit = event => { - const { username, password } = this.state; + const { username, password, loading } = this.state; event.preventDefault(); - this.safeSetState({ loading: true }); + if (!loading) { + this.safeSetState({ loading: true }); - api.login(username, password) - .catch(error => { - if (error.response.status === 401) { - this.safeSetState({ error: LOGIN_ERROR_MESSAGE }); - } - }) - .finally(() => { - this.safeSetState({ loading: false }); - }); + api.login(username, password) + .then(() => { + this.safeSetState({ loading: false }); + }) + .catch(error => { + this.safeSetState({ loading: false }); + if (error.response.status === 401) { + this.safeSetState({ error: LOGIN_ERROR_MESSAGE }); + } + }); + } } render () { From 986d299961f4bab83caa090f35a399b7f7303c6b Mon Sep 17 00:00:00 2001 From: John Mitchell Date: Wed, 31 Oct 2018 10:28:51 -0400 Subject: [PATCH 3/5] move login back to using finally handler and update tests --- __tests__/tests/LoginPage.test.jsx | 33 ++++++++---------------------- src/pages/Login.jsx | 7 +++---- 2 files changed, 12 insertions(+), 28 deletions(-) diff --git a/__tests__/tests/LoginPage.test.jsx b/__tests__/tests/LoginPage.test.jsx index 26af4924de..3e47936111 100644 --- a/__tests__/tests/LoginPage.test.jsx +++ b/__tests__/tests/LoginPage.test.jsx @@ -79,17 +79,12 @@ describe('', () => { api.login = jest.fn().mockImplementation(() => { const loginPromise = Promise.resolve({}); - // wrapped in timeout so this .then happens after state.loading is set to - // false in the actual .then handler - // - // would be improved by using .finally but that's not available for - // some reason setTimeout(() => { - loginPromise.then(() => { + loginPromise.finally(() => { expect(loginPage.state().loading).toBe(false); done(); }); - }, 50); + }, 1); return loginPromise; }); @@ -109,21 +104,16 @@ describe('', () => { const err = { response: { status: 401, message: 'problem' }, }; - const loginPromise = Promise.reject(err); - // wrapped in timeout so this .then happens after state.loading is set to - // false in the actual .then handler - // - // would be improved by using .finally but that's not available for - // some reason setTimeout(() => { loginPromise.catch(() => { - expect(loginPage.state().loading).toBe(false); expect(loginPage.state().error).toBe(LOGIN_ERROR_MESSAGE); + }).finally(() => { + expect(loginPage.state().loading).toBe(false); done(); }); - }, 50); + }, 1); return loginPromise; }); @@ -139,26 +129,21 @@ describe('', () => { expect(loginPage.state().loading).toBe(true); }); - test('submit calls api.login handles 401 error', (done) => { + test('submit calls api.login handles non-401 error', (done) => { api.login = jest.fn().mockImplementation(() => { const err = { response: { status: 500, message: 'problem' }, }; - const loginPromise = Promise.reject(err); - // wrapped in timeout so this .then happens after state.loading is set to - // false in the actual .then handler - // - // would be improved by using .finally but that's not available for - // some reason setTimeout(() => { loginPromise.catch(() => { - expect(loginPage.state().loading).toBe(false); expect(loginPage.state().error).toBe(''); + }).finally(() => { + expect(loginPage.state().loading).toBe(false); done(); }); - }, 50); + }, 1); return loginPromise; }); diff --git a/src/pages/Login.jsx b/src/pages/Login.jsx index 0ad82ef756..37f5ece0df 100644 --- a/src/pages/Login.jsx +++ b/src/pages/Login.jsx @@ -50,14 +50,13 @@ class LoginPage extends Component { this.safeSetState({ loading: true }); api.login(username, password) - .then(() => { - this.safeSetState({ loading: false }); - }) .catch(error => { - this.safeSetState({ loading: false }); if (error.response.status === 401) { this.safeSetState({ error: LOGIN_ERROR_MESSAGE }); } + }) + .finally(() => { + this.safeSetState({ loading: false }); }); } } From 55586b9b2a2a787a2c0968ab9fa55d295b8d2d26 Mon Sep 17 00:00:00 2001 From: John Mitchell Date: Wed, 31 Oct 2018 12:04:33 -0400 Subject: [PATCH 4/5] convert post-api.login code to using async/await for Login.jsx --- src/pages/Login.jsx | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/pages/Login.jsx b/src/pages/Login.jsx index 37f5ece0df..c806ef24b4 100644 --- a/src/pages/Login.jsx +++ b/src/pages/Login.jsx @@ -41,7 +41,7 @@ class LoginPage extends Component { handlePasswordChange = value => this.safeSetState({ password: value, error: '' }); - handleSubmit = event => { + handleSubmit = async event => { const { username, password, loading } = this.state; event.preventDefault(); @@ -49,15 +49,15 @@ class LoginPage extends Component { if (!loading) { this.safeSetState({ loading: true }); - api.login(username, password) - .catch(error => { - if (error.response.status === 401) { - this.safeSetState({ error: LOGIN_ERROR_MESSAGE }); - } - }) - .finally(() => { - this.safeSetState({ loading: false }); - }); + try { + await api.login(username, password); + } catch (error) { + if (error.response.status === 401) { + this.safeSetState({ error: LOGIN_ERROR_MESSAGE }); + } + } finally { + this.safeSetState({ loading: false }); + } } } From e53a6a91d67a68276825983cf320ad0e26576d55 Mon Sep 17 00:00:00 2001 From: John Mitchell Date: Wed, 31 Oct 2018 12:04:59 -0400 Subject: [PATCH 5/5] remove setTimeout hack for testing api.login Login.jsx handlers --- __tests__/tests/LoginPage.test.jsx | 97 +++++++++++------------------- jest.setup.js | 3 + 2 files changed, 37 insertions(+), 63 deletions(-) diff --git a/__tests__/tests/LoginPage.test.jsx b/__tests__/tests/LoginPage.test.jsx index 3e47936111..63675a98ab 100644 --- a/__tests__/tests/LoginPage.test.jsx +++ b/__tests__/tests/LoginPage.test.jsx @@ -1,6 +1,7 @@ import React from 'react'; import { MemoryRouter } from 'react-router-dom'; import { mount, shallow } from 'enzyme'; +import { asyncFlush } from '../../jest.setup'; import LoginPage from '../../src/pages/Login'; import api from '../../src/api'; @@ -75,88 +76,58 @@ describe('', () => { expect(loginPage.state().error).toBe(''); }); - test('submit calls api.login successfully', (done) => { - api.login = jest.fn().mockImplementation(() => { - const loginPromise = Promise.resolve({}); - - setTimeout(() => { - loginPromise.finally(() => { - expect(loginPage.state().loading).toBe(false); - done(); - }); - }, 1); - - return loginPromise; - }); + test('api.login not called when loading', () => { + api.login = jest.fn().mockImplementation(() => Promise.resolve({})); expect(loginPage.state().loading).toBe(false); - loginPage.setState({ username: 'unamee', password: 'pwordd', loading: true }); + loginPage.setState({ loading: true }); submitButton.simulate('submit'); expect(api.login).toHaveBeenCalledTimes(0); - loginPage.setState({ loading: false }); - submitButton.simulate('submit'); - expect(api.login).toHaveBeenCalledTimes(1); - expect(api.login).toHaveBeenCalledWith('unamee', 'pwordd'); - expect(loginPage.state().loading).toBe(true); }); - test('submit calls api.login handles 401 error', (done) => { - api.login = jest.fn().mockImplementation(() => { - const err = { - response: { status: 401, message: 'problem' }, - }; - const loginPromise = Promise.reject(err); - - setTimeout(() => { - loginPromise.catch(() => { - expect(loginPage.state().error).toBe(LOGIN_ERROR_MESSAGE); - }).finally(() => { - expect(loginPage.state().loading).toBe(false); - done(); - }); - }, 1); - - return loginPromise; - }); + test('submit calls api.login successfully', async () => { + api.login = jest.fn().mockImplementation(() => Promise.resolve({})); expect(loginPage.state().loading).toBe(false); - loginPage.setState({ username: 'unamee', password: 'pwordd', loading: true }); - submitButton.simulate('submit'); - expect(api.login).toHaveBeenCalledTimes(0); - loginPage.setState({ loading: false }); - expect(loginPage.state().error).toBe(''); + loginPage.setState({ username: 'unamee', password: 'pwordd' }); submitButton.simulate('submit'); expect(api.login).toHaveBeenCalledTimes(1); expect(api.login).toHaveBeenCalledWith('unamee', 'pwordd'); expect(loginPage.state().loading).toBe(true); + await asyncFlush(); + expect(loginPage.state().loading).toBe(false); }); - test('submit calls api.login handles non-401 error', (done) => { + test('submit calls api.login handles 401 error', async () => { api.login = jest.fn().mockImplementation(() => { - const err = { - response: { status: 500, message: 'problem' }, - }; - const loginPromise = Promise.reject(err); - - setTimeout(() => { - loginPromise.catch(() => { - expect(loginPage.state().error).toBe(''); - }).finally(() => { - expect(loginPage.state().loading).toBe(false); - done(); - }); - }, 1); - - return loginPromise; + const err = new Error('401 error'); + err.response = { status: 401, message: 'problem' }; + return Promise.reject(err); }); expect(loginPage.state().loading).toBe(false); - loginPage.setState({ username: 'unamee', password: 'pwordd', loading: true }); - submitButton.simulate('submit'); - expect(api.login).toHaveBeenCalledTimes(0); - loginPage.setState({ loading: false }); - expect(loginPage.state().error).toBe(''); + loginPage.setState({ username: 'unamee', password: 'pwordd' }); submitButton.simulate('submit'); expect(api.login).toHaveBeenCalledTimes(1); expect(api.login).toHaveBeenCalledWith('unamee', 'pwordd'); expect(loginPage.state().loading).toBe(true); + await asyncFlush(); + expect(loginPage.state().error).toBe(LOGIN_ERROR_MESSAGE); + expect(loginPage.state().loading).toBe(false); + }); + + test('submit calls api.login handles non-401 error', async () => { + api.login = jest.fn().mockImplementation(() => { + const err = new Error('500 error'); + err.response = { status: 500, message: 'problem' }; + return Promise.reject(err); + }); + expect(loginPage.state().loading).toBe(false); + loginPage.setState({ username: 'unamee', password: 'pwordd' }); + submitButton.simulate('submit'); + expect(api.login).toHaveBeenCalledTimes(1); + expect(api.login).toHaveBeenCalledWith('unamee', 'pwordd'); + expect(loginPage.state().loading).toBe(true); + await asyncFlush(); + expect(loginPage.state().error).toBe(''); + expect(loginPage.state().loading).toBe(false); }); test('render Redirect to / when already authenticated', () => { diff --git a/jest.setup.js b/jest.setup.js index 570acf4cf7..62cb566c00 100644 --- a/jest.setup.js +++ b/jest.setup.js @@ -1,5 +1,8 @@ require('@babel/polyfill'); +// eslint-disable-next-line import/prefer-default-export +export const asyncFlush = () => new Promise((resolve) => setImmediate(resolve)); + const enzyme = require('enzyme'); const Adapter = require('enzyme-adapter-react-16');