208 add tests for a richeditor#308
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DOM-level testing infrastructure for the ChangesRichEditor Testing Setup and Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsx (1)
44-46: 💤 Low valueConsider simplifying the initial render.
React Testing Library's
renderfunction already wraps updates inactinternally, so the explicitawait act(async () => render(...))wrapper is redundant. Theasynccallback wrapper serves no purpose here sincerenderis synchronous.♻️ Simplified version
- // Use `act` to wait for all editor updates to complete before making assertions; - // otherwise, some asynchronous updates may not have been applied to the DOM yet. - const result = await act(async () => render(renderEditor(props))); + const result = render(renderEditor(props));Note: The custom
rerender,insert, andformatmethods correctly useactsince they trigger state updates outside of RTL's automatic wrapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsx` around lines 44 - 46, The initial render in renderRichEditor.tsx wraps render(renderEditor(props)) in an unnecessary await act(...) call; replace the explicit await act(async () => render(renderEditor(props))) with a direct call to render(renderEditor(props)) and assign its return to result (since RTL's render already handles act), while leaving the custom rerender, insert, and format helpers that explicitly use act unchanged; locate the call around renderEditor and act imports and update accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/scripts/vitest.setup.ts`:
- Around line 17-22: Element.prototype.getClientRects currently sets
Symbol.iterator to an empty function which returns undefined and breaks
iteration; update the mock returned by Element.prototype.getClientRects (the
DOMRectList stub) so its [Symbol.iterator] property is a function that returns
an iterator object with a next() method (e.g., returning { done: true }
immediately or iterating over an internal array), ensuring the returned object
conforms to the iterator protocol and matches DOMRectList behavior for for...of
and spread usage.
---
Nitpick comments:
In `@packages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsx`:
- Around line 44-46: The initial render in renderRichEditor.tsx wraps
render(renderEditor(props)) in an unnecessary await act(...) call; replace the
explicit await act(async () => render(renderEditor(props))) with a direct call
to render(renderEditor(props)) and assign its return to result (since RTL's
render already handles act), while leaving the custom rerender, insert, and
format helpers that explicitly use act unchanged; locate the call around
renderEditor and act imports and update accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7501d73-29c8-475e-96a6-79c0b93d9557
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/app/src/core/storage/interop/export/__snapshots__/NotesExporter.dom.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
package.jsonpackages/app/scripts/vitest.setup.tspackages/app/src/core/storage/interop/export/NotesExporter.dom.test.tspackages/app/src/features/NoteEditor/tests/richEditor.dom.test.tspackages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsxpackages/app/src/features/NoteEditor/tests/utils/utils.tsxpackages/app/vitest.config.mtswords.txt
💤 Files with no reviewable changes (1)
- packages/app/src/core/storage/interop/export/NotesExporter.dom.test.ts
| const onFormatting = createEvent<TextFormat>(); | ||
| const onInserting = createEvent<InsertingPayload>(); |
There was a problem hiding this comment.
Instead of simulating clicks on editor panel buttons, i suggests directly call the APIs that those buttons use under the hood. So the tests is not depends on DOM structure in editor panel ans we can change the behavior of the panel without changing the tests
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsx`:
- Around line 51-60: The test helper functions insert and format are declared
async but do not await anything, misleading callers who use await
editor.insert(...) / await editor.format(...); update the helper implementations
in renderRichEditor.tsx by removing the async keyword from the insert and format
functions (the ones calling onInserting(payload) and onFormatting(format)) so
they are plain synchronous helpers, ensuring tests do not falsely rely on
awaiting them for Lexical/React DOM flushes.
In `@packages/app/src/features/NoteEditor/tests/utils/utils.tsx`:
- Line 34: The error message in utils.tsx is using the wrong variable: when
endNode is null the thrown Error interpolates endNode (null) instead of the
search string endText; update the throw in the code that checks if (!endNode) to
reference endText (or the correct search variable) so the Error reads something
like "Text node not found for \"<searched text>\"" to show what was being looked
for (locate the check that throws when endNode is falsy and replace the
interpolated variable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b06610c8-eaf5-45e1-a4c2-29930f203051
📒 Files selected for processing (4)
packages/app/scripts/vitest.setup.tspackages/app/src/features/NoteEditor/tests/richEditor.dom.test.tspackages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsxpackages/app/src/features/NoteEditor/tests/utils/utils.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/app/scripts/vitest.setup.ts
- packages/app/src/features/NoteEditor/tests/richEditor.dom.test.ts
katsyuta
force-pushed
the
208-add-tests-for-a-richeditor
branch
from
e145c11 to
b31cb56
Compare
| "/_resources/[REDACTED-UUID]-test.txt", | ||
| "/[REDACTED-UUID].md", | ||
| "/[REDACTED-UUID].md", | ||
| "/_resources/[REDACTED-UUID]-test.txt", |
There was a problem hiding this comment.
Why snapshot is changed?
It must not be changed. Because it means something have been changed, but it would be out of scope of that PR.
We need in reproducible tests.
There was a problem hiding this comment.
I can’t understand why the snapshot changed. This happened when I added projects to the Vitest config. Only the order of the results changed.
Maybe Vitest changes the order of test runs, and this affected the snapshot.
There was a problem hiding this comment.
Look at the snapshot. There are difference in array items order, not in test runs order.
Explore that problem reasons and fix it.
In case it cannot be fixed, we have to at least manually order results before snapshot it, to make sure we will not have such unexpected changes for snapshots in future
| expect(screen.getByText(content)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('Toggles text formatting', async () => { |
There was a problem hiding this comment.
We need in test that would work with a multi line text and to format its parts, not whole line.
Like that
That **text** is *partially* formatted.
Only _that_ word is selected. **Not a whole line**
There was a problem hiding this comment.
Now the editor works incorrectly, we can’t format a part of a string or multiple strings.
So I added a test, but marked it as fails, and when we fix the implementation, it should be updated (just remove the .fails).
There was a problem hiding this comment.
- The tests is incomplete. We need to test a user interactions that includes clicks and keyboard inputs. Add such tests. Use only DOM events for that interactions like real user does.
- We must split tests into few files. We will have a hundreds tests for rich editor eventually. So we must invent the rules how to organize them. Each file must have not very much tests, but we have to support very large amount of test files. We could break the tests at least for "interactive" and non interactive. The interactive ones is where user actively interact with editor (via DOM events) and we check that editor behaves as expected. Also we could breakdown them by scope like formatting, editing, etc. Think about that, maybe consult with ChatGPT
- We have to support the tests with many editors. I don't see such tests that proves that 2 instances will work fine together in one document
- I don't see any cleanups before each test. How it does work? We must be able to control the DOM.
There was a problem hiding this comment.
Cleanup with unmount calls automatically. If a test needs to call unmount manually, we can do so like this:
const editor = await renderRichEditor({ value: `` });
editor.unmount();
katsyuta
force-pushed
the
208-add-tests-for-a-richeditor
branch
from
fa800ff to
b3d1a58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/features/NoteEditor/tests/utils/utils.ts`:
- Around line 34-37: The range.setEnd calls at lines 34 and 36 are using
endText.length and startText.length (the query string lengths) as offsets, which
can exceed the actual text node's length and cause IndexSizeError when content
spans multiple nodes. Replace endText.length with endNode.length in the
range.setEnd call within the if block, and replace startText.length with
startNode.length in the range.setEnd call within the else block to use the
actual text node bounds instead of the search query length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 280cf089-575c-4ae9-88d4-c4a1b9664429
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/app/src/core/storage/interop/export/__snapshots__/NotesExporter.dom.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
package.jsonpackages/app/scripts/vitest.setup.tspackages/app/src/core/storage/interop/export/NotesExporter.dom.test.tspackages/app/src/features/NoteEditor/tests/richEditor.dom.test.tspackages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsxpackages/app/src/features/NoteEditor/tests/utils/utils.tspackages/app/vitest.config.mtswords.txt
💤 Files with no reviewable changes (1)
- packages/app/src/core/storage/interop/export/NotesExporter.dom.test.ts
✅ Files skipped from review due to trivial changes (1)
- words.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- packages/app/src/features/NoteEditor/tests/utils/renderRichEditor.tsx
- packages/app/vitest.config.mts
- packages/app/scripts/vitest.setup.ts
- packages/app/src/features/NoteEditor/tests/richEditor.dom.test.ts
| "@testing-library/react": "^16.3.2", | ||
| "@testing-library/user-event": "^14.6.1", |
There was a problem hiding this comment.
All that deps must be installed in workspace app, not globally
| "/_resources/[REDACTED-UUID]-test.txt", | ||
| "/[REDACTED-UUID].md", | ||
| "/[REDACTED-UUID].md", | ||
| "/_resources/[REDACTED-UUID]-test.txt", |
There was a problem hiding this comment.
Look at the snapshot. There are difference in array items order, not in test runs order.
Explore that problem reasons and fix it.
In case it cannot be fixed, we have to at least manually order results before snapshot it, to make sure we will not have such unexpected changes for snapshots in future
| await user.click(screen.getByRole('heading')); | ||
| await user.keyboard('Some text'); | ||
|
|
||
| expect(screen.getByRole('textbox')).not.toHaveTextContent('Some text'); |
There was a problem hiding this comment.
What text content it must contain instead? Add more specific expectation
| const [editorBoxA, editorBoxB] = screen.getAllByRole('textbox'); | ||
| expect(screen.getAllByRole('textbox')).toHaveLength(2); |
There was a problem hiding this comment.
Technically it is possible that after call the getAllByRole on first line the second call will return different value.
So it is more correct to call that code once, assign array into some variable, check its size, and then assign constants from there
| const [editorBoxA, editorBoxB] = screen.getAllByRole('textbox'); | |
| expect(screen.getAllByRole('textbox')).toHaveLength(2); | |
| const textBoxes = screen.getAllByRole('textbox'); | |
| expect(textBoxes).toHaveLength(2); | |
| const [editorBoxA, editorBoxB] = textBoxes; |
But maybe even better to use values by its indexes like that textBoxes[1].
| // editorB must stay untouched | ||
| expect(within(editorBoxB).queryByRole('emphasis')).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Of course it will be untouched. Because you never tried to change its content. You call format only for editorA here. What this test prove at all then?
| expect(within(editor).getAllByRole('paragraph')).toHaveLength(1); | ||
| const [paragraph] = within(editor).getAllByRole('paragraph'); | ||
| expect(paragraph).toHaveTextContent('My favorite dish is cake'); |
There was a problem hiding this comment.
The same as https://github.com/DeepinkApp/deepink/pull/308/changes#r3481341356
Fix that problem everywhere. It is difficult to understand the code for now because of that.
Btw, is there any methods like get or first to select first matched element?
| const [firstParagraph, secondParagraph] = within(editor).getAllByRole('paragraph'); | ||
| expect(within(editor).getAllByRole('paragraph')).toHaveLength(2); | ||
| expect(firstParagraph).toHaveTextContent('My favorite dis'); | ||
| expect(secondParagraph).toHaveTextContent('h is cake'); |
There was a problem hiding this comment.
What if there will be 2 paragraph and 100 dividers <hr/>?
This test will not catch that problem.
Can we build our tests in a way to catch such unexpected changes?
My expectation is test would declare what is expected DOM structure and assertions would check that.
2 participants
Closes #208
I added Vitest projects to separate tests by environment. Projects allow us to use different setup files and test settings for different groups of tests. To distinguish tests that require a DOM environment, I suggest using the
.dom.test.tssuffix.Summary by CodeRabbit
Tests
Chores