-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat(ui): add create tag form #3672
Conversation
@@ -1,17 +0,0 @@ | |||
import { render, screen } from "@testing-library/react"; |
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.
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.
Just a few minor comments, but looks good. Appreciate the screenshot, it gives more context.
screen.getByRole("textbox", { name: Label.KernelOptions }), | ||
"options1" | ||
); | ||
fireEvent.submit(screen.getByRole("form")); |
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.
It would be better to write in this way that resembles real user interaction (either clicking a submit button, or userEvent.type(input, "{enter}");
.
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.
This is something I've been curious about. Doesn't testing for one particular interaction actually limit (or further dictate) the expected interactions with the form, when in reality the user can submit the form with pressing enter in the form or clicking the submit (I also don't know if there are some screen readers/mobile keyboards etc. can call onSubmit()
directly).
It seems like that (without testing every possible interaction) testing the form using .submit()
captures the broadest scope of interactions as opposed to having a test that would pass with a <button onClick=...
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.
This is something I've been curious about. Doesn't testing for one particular interaction actually limit (or further dictate) the expected interactions with the form, when in reality the user can submit the form with pressing enter in the form or clicking the submit (I also don't know if there are some screen readers/mobile keyboards etc. can call
onSubmit()
directly).
Very good point. It is a compromise, for sure. The way I think about it is: test from a real user's perspective as much as you can.
A user normally can never fire a submit event directly and it's better to focus on real interactions. In my experience, testing the enter button press is often the best choice (clicking the button is more likely to be caught with manual testing).
But you did spark my interest, I'll dig into this a bit more.
It seems like that (without testing every possible interaction) testing the form using
.submit()
captures the broadest scope of interactions as opposed to having a test that would pass with a<button onClick=...
It would be great if it worked that way in practice, but in reality (AFAIK) it's possible that fireEvent.submit
would trigger the form submit correctly when neither submit button click nor pressing Enter works.
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.
So maybe the approach here should be that the FormikForm component should have tests for all three interactions, while the components that consume FormikForm can use any of the methods. We don't need to test the internals of the component each time we consume it, but we also get the benefit of having tests for each way the user can interact with the form.
I've filed #3688 so we can continue the discussion there.
items: [tagFactory({ id: 8, name: "tag1" })], | ||
saved: true, | ||
}); | ||
fireEvent.submit(screen.getByRole("form")); |
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.
It would be better to write in this way that resembles real user interaction (either clicking a submit button, or userEvent.type(input, "{enter}");
.
jest | ||
.spyOn(baseHooks, "useCycled") | ||
.mockImplementation(() => [true, () => null]); |
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.
Would you mind adding a comment explaining why this is needed/what happens here. Even better if we could avoid doing this to avoid having to update tests when implementation details change.
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.
Yep, done, thanks. I spent quite a bit of time trying to get this to work with rerender
but it appears that while you can update the redux state to change before calling rerender
the component doesn't maintain its previous ref states so the values don't get cycled. I'm not sure if there's something I'm missing or if this is just the way rerender
works.
ui/src/app/tags/components/TagsHeader/AddTagForm/AddTagFormFields/AddTagFormFields.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/tags/components/TagsHeader/AddTagForm/AddTagFormFields/AddTagFormFields.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/tags/components/TagsHeader/TagHeaderForms/TagHeaderForms.test.tsx
Show resolved
Hide resolved
Add a form to create tags to the tag list header. Fixes: canonical/app-tribe#747.
Done
QA
MAAS deployment
To run this branch you will need access to one of the following MAAS deployments:
Running the branch
You can run this branch by:
QA steps
//node[@class="system"]/vendor = "QEMU" and,
and submit.Fixes
Fixes: canonical-web-and-design/app-tribe#747.
Screenshots