From 1421be3a23ef24a46a1b2198a9f2ff173fb85e88 Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Wed, 24 Aug 2022 14:21:11 +0200 Subject: [PATCH 1/8] fix: allow setting Textbox "value" dynamically #1578 * allow Textbox "value" to be set from wave app * add tests for TextField and MaskedTextField * fix warning about act() in tests closes #1578 --- ui/src/textbox.test.tsx | 60 ++++++++++++++++++++++++++++++++++++++--- ui/src/textbox.tsx | 13 ++++++--- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/ui/src/textbox.test.tsx b/ui/src/textbox.test.tsx index 03c5f07f06..5ec18d8b20 100644 --- a/ui/src/textbox.test.tsx +++ b/ui/src/textbox.test.tsx @@ -12,19 +12,21 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { fireEvent, render } from '@testing-library/react' +import { fireEvent, render, act } from '@testing-library/react' +import userEvent from '@testing-library/user-event' import React from 'react' import { Textbox, XTextbox } from './textbox' import { wave } from './ui' const name = 'textbox' -const textboxProps: Textbox = { name } +let textboxProps: Textbox = { name } describe('Textbox.tsx', () => { beforeEach(() => { jest.clearAllMocks() jest.useFakeTimers() wave.args[name] = null + textboxProps = { name } }) it('Renders data-test attr', () => { @@ -51,7 +53,7 @@ describe('Textbox.tsx', () => { it('Sets args on input', () => { const { getByTestId } = render() fireEvent.change(getByTestId(name), { target: { value: 'text' } }) - jest.runOnlyPendingTimers() + act(() => { jest.runOnlyPendingTimers() }) expect(wave.args[name]).toBe('text') }) @@ -79,10 +81,26 @@ describe('Textbox.tsx', () => { fireEvent.change(getByTestId(name), { target: { value: 'aaa' } }) expect(pushMock).not.toBeCalled() // Not called immediately, but after specified timeout. - jest.runOnlyPendingTimers() + act(() => { jest.advanceTimersByTime(250) }) + expect(pushMock).not.toBeCalled() + act(() => { jest.advanceTimersByTime(250) }) expect(pushMock).toBeCalled() }) + it('Debounces wave push', () => { + const { getByTestId } = render() + const pushMock = jest.fn() + wave.push = pushMock + + userEvent.type(getByTestId(name), 'a') + userEvent.type(getByTestId(name), 'a') + userEvent.type(getByTestId(name), 'a') + userEvent.type(getByTestId(name), 'a') + act(() => { jest.runOnlyPendingTimers() }) + + expect(pushMock).toBeCalledTimes(1) + }) + it('Does not call sync on change - trigger not specified', () => { const { getByTestId } = render() @@ -93,4 +111,38 @@ describe('Textbox.tsx', () => { expect(pushMock).not.toBeCalled() }) + + it('Display new value when "value" prop changes', () => { + const { getByTestId, rerender } = render() + expect(getByTestId(name)).toHaveValue('A') + + rerender() + expect(getByTestId(name)).toHaveValue('B') + }) + + it('Types value and then display new value when "value" prop changes', () => { + const { getByTestId, rerender } = render() + userEvent.type(getByTestId(name), '{backspace}A{Enter}') + expect(getByTestId(name)).toHaveValue('A') + + rerender() + expect(getByTestId(name)).toHaveValue('B') + }) + + it('Display new value when "value" prop changes (masked)', () => { + const { getByTestId, rerender } = render() + expect(getByTestId(name)).toHaveValue('(123)') + + rerender() + expect(getByTestId(name)).toHaveValue('(456)') + }) + + it('Types value and then display new value when "value" prop changes (masked)', () => { + const { getByTestId, rerender } = render() + userEvent.type(getByTestId(name), '{backspace}123{Enter}') + expect(getByTestId(name)).toHaveValue('(123)') + + rerender() + expect(getByTestId(name)).toHaveValue('(456)') + }) }) \ No newline at end of file diff --git a/ui/src/textbox.tsx b/ui/src/textbox.tsx index 1dcbc509a8..8ac95a1235 100644 --- a/ui/src/textbox.tsx +++ b/ui/src/textbox.tsx @@ -71,20 +71,25 @@ const DEBOUNCE_TIMEOUT = 500 export const XTextbox = ({ model: m }: { model: Textbox }) => { const + [value, setValue] = React.useState(m.value ?? ''), + debounceRef = React.useRef(debounce(DEBOUNCE_TIMEOUT, () => wave.push())), onChange = ({ target }: React.FormEvent, v?: S) => { v = v || (target as HTMLInputElement).value wave.args[m.name] = v ?? (m.value || '') - if (m.trigger) wave.push() + if (m.trigger) debounceRef.current() + setValue(v) + m.value = v }, textFieldProps: Fluent.ITextFieldProps & { 'data-test': S } = { 'data-test': m.name, label: m.label, + value, errorMessage: m.error, required: m.required, disabled: m.disabled, readOnly: m.readonly, - onChange: m.trigger ? debounce(DEBOUNCE_TIMEOUT, onChange) : onChange, + onChange, iconProps: m.icon ? { iconName: m.icon } : undefined, placeholder: m.placeholder, prefix: m.prefix, @@ -96,9 +101,10 @@ export const // eslint-disable-next-line react-hooks/exhaustive-deps React.useEffect(() => { wave.args[m.name] = m.value || '' }, []) + React.useEffect(() => { setValue(m.value ?? '')}, [m.value]) return m.mask - ? + ? : ( ) } \ No newline at end of file From e32d6816186461d3ceb89e40aa0156c9c656ff47 Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Mon, 5 Sep 2022 13:36:39 +0200 Subject: [PATCH 2/8] docs: Add comment on why we need to reset props for every test --- ui/src/textbox.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/src/textbox.test.tsx b/ui/src/textbox.test.tsx index 5ec18d8b20..a8d1cd19cd 100644 --- a/ui/src/textbox.test.tsx +++ b/ui/src/textbox.test.tsx @@ -26,6 +26,7 @@ describe('Textbox.tsx', () => { jest.clearAllMocks() jest.useFakeTimers() wave.args[name] = null + // Because we mutate props to programatically change "value" from Wave app we need to reset it before every test textboxProps = { name } }) From c63ad05ccb68fe290172b8c6888d64202c5cd5bb Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Mon, 5 Sep 2022 13:39:04 +0200 Subject: [PATCH 3/8] refactor: Move wave.push setup to the top --- ui/src/textbox.test.tsx | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/ui/src/textbox.test.tsx b/ui/src/textbox.test.tsx index a8d1cd19cd..24bf97e57b 100644 --- a/ui/src/textbox.test.tsx +++ b/ui/src/textbox.test.tsx @@ -18,8 +18,11 @@ import React from 'react' import { Textbox, XTextbox } from './textbox' import { wave } from './ui' -const name = 'textbox' +const + name = 'textbox', + pushMock = jest.fn() let textboxProps: Textbox = { name } +wave.push = pushMock describe('Textbox.tsx', () => { beforeEach(() => { @@ -76,9 +79,6 @@ describe('Textbox.tsx', () => { it('Calls sync on change - trigger specified', () => { const { getByTestId } = render() - const pushMock = jest.fn() - wave.push = pushMock - fireEvent.change(getByTestId(name), { target: { value: 'aaa' } }) expect(pushMock).not.toBeCalled() // Not called immediately, but after specified timeout. @@ -90,8 +90,6 @@ describe('Textbox.tsx', () => { it('Debounces wave push', () => { const { getByTestId } = render() - const pushMock = jest.fn() - wave.push = pushMock userEvent.type(getByTestId(name), 'a') userEvent.type(getByTestId(name), 'a') @@ -105,9 +103,6 @@ describe('Textbox.tsx', () => { it('Does not call sync on change - trigger not specified', () => { const { getByTestId } = render() - const pushMock = jest.fn() - wave.push = pushMock - fireEvent.change(getByTestId(name), { target: { value: 'aaa' } }) expect(pushMock).not.toBeCalled() From 96bee4190e057da76d4139b8ccf6db1fba75b091 Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Mon, 5 Sep 2022 13:46:41 +0200 Subject: [PATCH 4/8] refactor: Pass wave.push reference directly instead of using a callback --- ui/src/textbox.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/src/textbox.tsx b/ui/src/textbox.tsx index 8ac95a1235..46bac07138 100644 --- a/ui/src/textbox.tsx +++ b/ui/src/textbox.tsx @@ -72,7 +72,7 @@ export const XTextbox = ({ model: m }: { model: Textbox }) => { const [value, setValue] = React.useState(m.value ?? ''), - debounceRef = React.useRef(debounce(DEBOUNCE_TIMEOUT, () => wave.push())), + debounceRef = React.useRef(debounce(DEBOUNCE_TIMEOUT, wave.push)), onChange = ({ target }: React.FormEvent, v?: S) => { v = v || (target as HTMLInputElement).value From 4cc0ec59b08f19dd5af76c4c3feb7e46e84c03bf Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Mon, 5 Sep 2022 13:48:16 +0200 Subject: [PATCH 5/8] tests: remove unnecessary clean up code --- ui/src/textbox.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/ui/src/textbox.test.tsx b/ui/src/textbox.test.tsx index 24bf97e57b..054d23d8aa 100644 --- a/ui/src/textbox.test.tsx +++ b/ui/src/textbox.test.tsx @@ -28,7 +28,6 @@ describe('Textbox.tsx', () => { beforeEach(() => { jest.clearAllMocks() jest.useFakeTimers() - wave.args[name] = null // Because we mutate props to programatically change "value" from Wave app we need to reset it before every test textboxProps = { name } }) From 21bbf7cb23d1029c9cad77dd032f04c6b9b01c01 Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Mon, 5 Sep 2022 13:50:27 +0200 Subject: [PATCH 6/8] tests: Remove unnecessary 'act' calls --- ui/src/textbox.test.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ui/src/textbox.test.tsx b/ui/src/textbox.test.tsx index 054d23d8aa..f7ed139a1d 100644 --- a/ui/src/textbox.test.tsx +++ b/ui/src/textbox.test.tsx @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { fireEvent, render, act } from '@testing-library/react' +import { fireEvent, render } from '@testing-library/react' import userEvent from '@testing-library/user-event' import React from 'react' import { Textbox, XTextbox } from './textbox' @@ -56,7 +56,7 @@ describe('Textbox.tsx', () => { it('Sets args on input', () => { const { getByTestId } = render() fireEvent.change(getByTestId(name), { target: { value: 'text' } }) - act(() => { jest.runOnlyPendingTimers() }) + jest.runOnlyPendingTimers() expect(wave.args[name]).toBe('text') }) @@ -81,9 +81,9 @@ describe('Textbox.tsx', () => { fireEvent.change(getByTestId(name), { target: { value: 'aaa' } }) expect(pushMock).not.toBeCalled() // Not called immediately, but after specified timeout. - act(() => { jest.advanceTimersByTime(250) }) + jest.advanceTimersByTime(250) expect(pushMock).not.toBeCalled() - act(() => { jest.advanceTimersByTime(250) }) + jest.advanceTimersByTime(250) expect(pushMock).toBeCalled() }) @@ -94,7 +94,7 @@ describe('Textbox.tsx', () => { userEvent.type(getByTestId(name), 'a') userEvent.type(getByTestId(name), 'a') userEvent.type(getByTestId(name), 'a') - act(() => { jest.runOnlyPendingTimers() }) + jest.runOnlyPendingTimers() expect(pushMock).toBeCalledTimes(1) }) From cb1e2448d5374be27046ccfec6f73882403b5b03 Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Mon, 5 Sep 2022 13:52:30 +0200 Subject: [PATCH 7/8] tests: Revert test for 'Calls sync - trigger' --- ui/src/textbox.test.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ui/src/textbox.test.tsx b/ui/src/textbox.test.tsx index f7ed139a1d..f3e5ea913d 100644 --- a/ui/src/textbox.test.tsx +++ b/ui/src/textbox.test.tsx @@ -81,10 +81,8 @@ describe('Textbox.tsx', () => { fireEvent.change(getByTestId(name), { target: { value: 'aaa' } }) expect(pushMock).not.toBeCalled() // Not called immediately, but after specified timeout. - jest.advanceTimersByTime(250) - expect(pushMock).not.toBeCalled() - jest.advanceTimersByTime(250) - expect(pushMock).toBeCalled() + jest.runOnlyPendingTimers() + expect(pushMock).toBeCalledTimes(1) }) it('Debounces wave push', () => { From 6225bd77adb6054dab2f75ab7b98138ad0161f0e Mon Sep 17 00:00:00 2001 From: Alexandre Alencar Date: Mon, 5 Sep 2022 14:19:54 +0200 Subject: [PATCH 8/8] fix: Set wave args properly when "value" prop changes --- ui/src/textbox.test.tsx | 19 +++++++++++++++++++ ui/src/textbox.tsx | 7 ++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/ui/src/textbox.test.tsx b/ui/src/textbox.test.tsx index f3e5ea913d..26a3469c75 100644 --- a/ui/src/textbox.test.tsx +++ b/ui/src/textbox.test.tsx @@ -75,6 +75,25 @@ describe('Textbox.tsx', () => { expect(wave.args[name]).toBe('default') }) + it('Sets args when "value" prop changes', () => { + const { rerender } = render() + expect(wave.args[name]).toBe('a') + + rerender() + expect(wave.args[name]).toBe('b') + }) + + it('Sets args when typing new text and then "value" prop changes', () => { + const { getByTestId, rerender } = render() + expect(wave.args[name]).toBe('a') + + userEvent.type(getByTestId(name), '{backspace}b') + expect(wave.args[name]).toBe('b') + + rerender() + expect(wave.args[name]).toBe('c') + }) + it('Calls sync on change - trigger specified', () => { const { getByTestId } = render() diff --git a/ui/src/textbox.tsx b/ui/src/textbox.tsx index 46bac07138..b6c159f662 100644 --- a/ui/src/textbox.tsx +++ b/ui/src/textbox.tsx @@ -99,9 +99,10 @@ export const type: m.password ? 'password' : undefined, } - // eslint-disable-next-line react-hooks/exhaustive-deps - React.useEffect(() => { wave.args[m.name] = m.value || '' }, []) - React.useEffect(() => { setValue(m.value ?? '')}, [m.value]) + React.useEffect(() => { + wave.args[m.name] = m.value ?? '' + setValue(m.value ?? '') + }, [m.value, m.name]) return m.mask ?