208 add tests for a richeditor#308
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds jsdom/test dependencies and setup, moves DOM setup into a jsdom Vitest project, installs browser-layout mocks, provides RichEditor test utilities, and adds extensive DOM tests for markdown rendering, formatting, images, and list behaviors. 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 |
| @@ -1,5 +1,3 @@ | |||
| // @vitest-environment jsdom | |||
There was a problem hiding this comment.
After added projects in the vitest configuration, this annotation is no needed.
vitest ignores environment annotations when projects are used, so the file was renamed to indicate the required test environment
|
@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
| require('@testing-library/jest-dom'); | ||
| require('blob-polyfill'); |
There was a problem hiding this comment.
Why this code runs with no conditions? How it will work in node environment?
| "/[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.
| notesApi={{} as any} | ||
| filesRegistry={{} as any} | ||
| filesController={{} as any} | ||
| attachmentsController={{} as any} | ||
| tagsRegistry={{} as any} | ||
| notesRegistry={{} as any} | ||
| notesHistory={{} as any} | ||
| notesIndex={{} as any} |
There was a problem hiding this comment.
Why we provide nothing?
We have to provide only minimal contexts that is actually used by rich editor component.
The WorkspaceProvider component is just an aggregate helper.
Everything that is actually used, must be mocked, not to be an empty object
There was a problem hiding this comment.
The directory with stuff for testing must be named as __tests__, that is a special name to distinguish the actual code and the tests code
| * Selects everything from the beginning of `startText` to the end of `endText`; | ||
| * If `endText` is omitted, selects the entire `startText`. | ||
| */ | ||
| export const selectContent = (startText: string, endText?: string) => { |
There was a problem hiding this comment.
Incorrect abstraction.
We may potentially have many instances of the editors. So it is technically not possible to select the text in specific editor with current api.
The util must receive the container node.
Also it is unclear what happens when i pass only startText
| isReadOnly: true, | ||
| }); | ||
|
|
||
| expect(screen.getByRole('textbox')).toHaveAttribute('contenteditable', 'false'); |
There was a problem hiding this comment.
That expectation proves nothing. What if user will click that editor and will enter text?
| value: `My favorite image\n\n I love cat`, | ||
| }); | ||
|
|
||
| selectContent('My favorite image'); |
There was a problem hiding this comment.
Why we select the text to insert image? It is not how actual user does act.
The real human being will click text to place cursor on some position. That is not a selection.
| const firstText = screen.getByText('My favorite image'); | ||
| const secondText = screen.getByText('I love cat'); |
There was a problem hiding this comment.
Why we define it here and don't use immediately? Move that
| 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.
- 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.
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