refactor(text-editor): covert tests to use react-testing-library #7016
+2,132
−1,779
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robinzigmond
force-pushed
the
FE-6805-texteditor-rtl-yay
branch
from
8333ffd
to
3eab34b
Compare
robinzigmond
force-pushed
the
FE-6805-texteditor-rtl-yay
branch
from
3eab34b
to
df84249
Compare
| ); | ||
| }); | ||
|
|
||
| test("files a console warning when the `rows` prop is passed as a number less than 2", () => { |
There was a problem hiding this comment.
suggestion/question/nitpick: Is this a spelling mistake?
Suggested change
| test("files a console warning when the `rows` prop is passed as a number less than 2", () => { | |
| test("fires a console warning when the `rows` prop is passed as a number less than 2", () => { |
| expect(screen.getByText("talic texti")).toBeVisible(); | ||
| }, 10000); // allow to run for up to 10 seconds due to timeout issues | ||
|
|
||
| // for coverage only - this behaviour doesn't work properly in jsdom (note incorrect order of text being entere). |
There was a problem hiding this comment.
nitpick:
Suggested change
| // for coverage only - this behaviour doesn't work properly in jsdom (note incorrect order of text being entere). | |
| // for coverage only - this behaviour doesn't work properly in jsdom (note incorrect order of text being entered). |
| expect(screen.getByRole("listitem")).toHaveTextContent("ist iteml"); | ||
| }, 10000); // allow to run for up to 10 seconds due to timeout issues | ||
|
|
||
| // for coverage only - this behaviour doesn't work properly in jsdom (note incorrect order of text being entere). |
There was a problem hiding this comment.
nitpick:
Suggested change
| // for coverage only - this behaviour doesn't work properly in jsdom (note incorrect order of text being entere). | |
| // for coverage only - this behaviour doesn't work properly in jsdom (note incorrect order of text being entered). |
| }); | ||
|
|
||
| // the next several tests are for coverage only - due to an issue with React itself, dispatching the (deprecated) `keypress` event | ||
| // with non-standard propertoes is the only way to trigger the `handleBeforeInput` handler in a test. See |
There was a problem hiding this comment.
nitpick:
Suggested change
| // with non-standard propertoes is the only way to trigger the `handleBeforeInput` handler in a test. See | |
| // with non-standard properties is the only way to trigger the `handleBeforeInput` handler in a test. See |
edleeks87
reviewed
Oct 21, 2024
There was a problem hiding this comment.
Really good work @robinzigmond, I really appreciate the detail you've gone to to explain why certain things have been added (has definitely limited the questions/comments from me) and the effort you've put into investigating the issues with JSDOM and DraftJS
|
|
||
| await user.keyboard("{ArrowRight}"); | ||
| expect(screen.getByRole("button", { name: "bold" })).toHaveFocus(); | ||
| }, 40000); // allow to run for up to 40 seconds(!) due to timeout issues |
There was a problem hiding this comment.
suggestion (non-blocking): it might be worth splitting these tests up a bit so we can avoid large timeouts here, I think we can live with the 10 second ones but exceeding that seems too much. If that doesn't work I think it'd be better to move them to playwright until we can rewrite the component.
Update: had a little play around locally, I think if we use a combination of async findBy querying for the tooltips and await waitFor we can remove the 10 second timeout tests (didn't work for the larger ones) so maybe we can break up the slower ones or move them to playwright
|
|
||
| screen.getByRole("button", { name: buttonName }).focus(); | ||
|
|
||
| expect(screen.getByTestId("tooltip")).toBeVisible(); |
There was a problem hiding this comment.
Suggested change
| expect(screen.getByTestId("tooltip")).toBeVisible(); | |
| expect(await screen.findByRole("tooltip")).toBeVisible(); |
There was a problem hiding this comment.
you can also probably combine them with:
expect(await screen.findByRole("tooltip", {name: tooltipText})).toBeVisible();
| expect(screen.getByTestId("tooltip")).toHaveTextContent(tooltipText); | ||
|
|
||
| screen.getByRole("button", { name: buttonName }).blur(); | ||
| expect(screen.queryByTestId("tooltip")).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Suggested change
| expect(screen.queryByTestId("tooltip")).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("tooltip")).not.toBeInTheDocument(); |
| } | ||
| ); | ||
|
|
||
| test("calls the `onClose` callback of an `LinkPreview` component if clicked", async () => { |
There was a problem hiding this comment.
Suggested change
| test("calls the `onClose` callback of an `LinkPreview` component if clicked", async () => { | |
| test("calls the `onClose` callback of a `LinkPreview` component if clicked", async () => { |
| }); | ||
|
|
||
| // the next several tests are for coverage only - due to an issue with React itself, dispatching the (deprecated) `keypress` event | ||
| // with non-standard propertoes is the only way to trigger the `handleBeforeInput` handler in a test. See |
| await expect(innerTextList(page, "ol", 3)).toHaveText("baz"); | ||
| }); | ||
|
|
||
| test("when a `list toolbar button is clicked, added text is rendered in an unordered list, with enter separating list items", async ({ |
There was a problem hiding this comment.
Suggested change
| test("when a `list toolbar button is clicked, added text is rendered in an unordered list, with enter separating list items", async ({ | |
| test("when a `list` toolbar button is clicked, added text is rendered in an unordered list, with enter separating list items", async ({ |
| await expect(innerTextList(page, "ul", 3)).toHaveText("baz"); | ||
| }); | ||
|
|
||
| test("when an list button is toggled on and off, new text is entered without the corresponding formatting, while the previously-entered text keeps the formatting", async ({ |
There was a problem hiding this comment.
Suggested change
| test("when an list button is toggled on and off, new text is entered without the corresponding formatting, while the previously-entered text keeps the formatting", async ({ | |
| test("when a `list` button is toggled on and off, new text is entered without the corresponding formatting, while the previously-entered text keeps the formatting", async ({ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed behaviour
TextEditor component should have unit tests written in React Testing Library
NOTE: due to many issues with the drafjs library and jsdom/RTL, a lot of functionality was not possible to sensibly test, so these tests have been added to Playwright instead. Many unit tests have been added just for coverage that do not otherwise provide much value. When we eventually move away from draftjs (which is no longer supported) we should write RTL tests for what is being covered by the new Playwright tests, and remove both those playwright tests and as many as possible of the current "coverage only" unit tests.
Current behaviour
Unit tests use Enzyme
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions