refactor(input-presentation, input): convert enzyme tests to RTL #6958
Conversation
tomdavies73
force-pushed
the
FE-6786
branch
from
1b01a73
to
cde5f52
Compare
| const inputPresentation = within(inputPresentationContainer).getByRole( | ||
| "presentation" | ||
| ); | ||
| expect(inputPresentationContainer).toBeVisible(); |
There was a problem hiding this comment.
suggestion (non-blocking): I'd feel more comfortable if this test actually checked that the Children content is visible - either by using getByTextContent or using a toHaveText assertion alongside the toBeVisible one(s)
| expect(inputPresentation).toBeVisible(); | ||
| }); | ||
|
|
||
| test("renders a passed node via the `positionedChildren` prop as a direct child of the presentation container", () => { |
There was a problem hiding this comment.
suggestion: I would prefer to reword this description to emphasise that the positionedChildren content is being rendered before the "normal" children, which seems to be the purpose of the prop and which you are definitely (and correctly) testing for here. Maybe something like renders a passed node via the positionedChildren prop as a child before the other children?
| "send", | ||
| ] as EnterKeyHintTypes[])( | ||
| "'enterKeyHint' is correctly passed to the input when prop value is %s", | ||
| (keyHints) => { |
There was a problem hiding this comment.
| (keyHints) => { | |
| (keyHint) => { |
nitpick (non-blocking): although it's from an array of different hintS, this variable at any one time only represents a single keyhint, so I'd prefer not to give it a name that implies a plural.
| const user = userEvent.setup(); | ||
| const input = screen.getByRole("textbox"); | ||
|
|
||
| await user.click(input); |
There was a problem hiding this comment.
suggestion (non-blocking): maybe there are problems that I've overlooked, but I'd prefer to just do input.focus() here, given what we're supposed to be testing. The danger with just testing the click is that, although this will focus the element, it does other things too, and we can't guarantee that other ways of focusing the element will also call the onFocus prop. Eg (unlikely I know) what if a bug was introduced where onFocus didn't get called on focus but was still called on click? That would be a bug but the test as you have it here would still pass.
| const user = userEvent.setup(); | ||
| const input = screen.getByRole("textbox"); | ||
|
|
||
| await user.click(input); |
There was a problem hiding this comment.
suggestion (non-blocking): same as above
| expect(input).toBeFocused(); | ||
| }); | ||
|
|
||
| test("triggers a passed function via the `onClick` prop from the input context provider, when the input is clicked", async () => { |
There was a problem hiding this comment.
comment: this seems to be the test that should have gone under the previous description. The onClick here is provided directly to the component rather than via context, so this description needs a slightly different test.
|
|
||
| input.selectionStart = 0; | ||
| input.selectionEnd = 0; | ||
| input.focus(); |
There was a problem hiding this comment.
comment: the description says "when the first character within the input is clicked", but there is no click here, just a focus. I'm not sure whether the important thing to test is the click or the focus, but either the description or this line of the test should be changed so that the 2 match.
|
|
||
| input.selectionStart = 5; | ||
| input.selectionEnd = 5; | ||
| input.focus(); |
There was a problem hiding this comment.
comment: as in the previous comment
|
|
||
| input.selectionStart = 2; | ||
| input.selectionEnd = 2; | ||
| input.focus(); |
| jest.useRealTimers(); | ||
| }); | ||
|
|
||
| test("when the input is rendered with a type other than 'text', setSelectionRange is not triggered on focus", () => { |
There was a problem hiding this comment.
comment (non-blocking): this one seems to be correct as the description says "on focus", which is what the test does. Maybe this means all the tests should have "on focus" (rather than "on click") in the description?
| await user.type(input, "test"); | ||
|
|
||
| jest.advanceTimersByTime(500); | ||
| expect(onChangeDeferredProp).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
praise: good idea to check the prop isn't called prior to the timeout finishing! 👍🏼
| const input = screen.getByRole("textbox"); | ||
|
|
||
| await user.click(input); | ||
| expect(input).toBeFocused(); |
There was a problem hiding this comment.
question: I noticed the title doesn't seem to match with assertions taking place. Just wanted to clarify if this was a potential oversight?
Proposed behaviour
Converts all enzyme tests to RTL.
Current behaviour
Currently, tests are written in enzyme.
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions