-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(multi-select): rewrite all enzyme tests with React Testing Library (FE-6693) #7037
base: master
Are you sure you want to change the base?
Conversation
…onent is unmounted
… only update state
480c2e5
to
a6a812c
Compare
a6a812c
to
3585172
Compare
const isClickTriggeredBySelect = useRef(false); | ||
const isMouseDownReported = useRef(false); | ||
const isMouseDownOnInput = useRef(false); | ||
const isOpenedByFocus = useRef(false); | ||
const [isOpenedByFocus, setIsOpenedByFocus] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question(non-blocking): What has been the thinking in moving this from ref
to state
? Did you discover an issue or something else when writing the new tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes have improved the code, but I'm not convinced we needed to do this at this time as we are just trying to get this refactored to RTL and all the Selects are being rewritten anyway
const isClickTriggeredBySelect = useRef(false); | ||
const isMouseDownReported = useRef(false); | ||
const isMouseDownOnInput = useRef(false); | ||
const isOpenedByFocus = useRef(false); | ||
const [isOpenedByFocus, setIsOpenedByFocus] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes have improved the code, but I'm not convinced we needed to do this at this time as we are just trying to get this refactored to RTL and all the Selects are being rewritten anyway
jest.restoreAllMocks(); | ||
}); | ||
|
||
test("displays input field", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: I'm not sure we get much value from this test as we can imply it's presence/visibility from the other tests that query/assert against it
); | ||
}); | ||
|
||
it("closes when input is clicked twice", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): might be worth doing the same test for the input icon
expect(screen.getByRole("combobox")).toHaveValue(""); | ||
}); | ||
|
||
test("clears the input after an option is selected", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: won't the input always have an empty value as nothing has been typed here?
Proposed behaviour
Current behaviour
MultiSelect
are written with Enzyme, which is a legacy packageChecklist
d.ts
file added or updated if requiredQA
Additional context
Testing instructions