test: add editor editing BDD coverage#344
Merged
Merged
Conversation
Reviewer's GuideAdds Playwright BDD configuration and a comprehensive editor-editing feature suite, migrating existing editor e2e cases into Gherkin-based BDD tests and wiring them into the test runner, while ignoring generated artifacts. Sequence diagram for Playwright BDD editor editing scenariosequenceDiagram
participant PlaywrightTest as Playwright_test_runner
participant PlaywrightBdd as playwright_bdd
participant Steps as editor_editing_steps
participant Page as Page
PlaywrightTest->>PlaywrightBdd: defineBddConfig(playwright.bdd.config.ts)
PlaywrightBdd->>PlaywrightTest: generate testDir (playwright/.features-gen)
PlaywrightTest->>PlaywrightBdd: run feature Editor_editing
PlaywrightBdd->>Steps: Given a blank document page is open
Steps->>Page: signInAndWaitForApp
Steps->>Page: createDocumentPageAndNavigate
Steps->>Page: focusEditor
PlaywrightBdd->>Steps: When I type "text" in the editor
Steps->>Page: insertTextIntoExpandedSelection
Steps->>Page: keyboard.type
PlaywrightBdd->>Steps: Then the editor contains "text"
Steps->>Page: EditorSelectors.slateEditor
Steps->>Page: expect.toContainText
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
editor-editing.steps.tsfile is quite large and mixes step definitions with a lot of helper logic; consider extracting the generic Slate/TestEditor helpers (e.g., selection, block traversal, paste/undo/redo utilities) into separate reusable modules to keep the step file focused and easier to navigate. - There are many
waitForTimeoutcalls sprinkled through the steps; where possible, replace fixed delays with expectation-based waits (e.g.,expect(...).to...) to make the tests less flaky and more deterministic. - The repeated
window/__TEST_EDITOR__access and type definitions forTestSlate*structures show similar patterns across helpers; consider centralizing thegetTestEditor/getTestWindowlogic and shared types to avoid duplication and reduce the chance of subtle inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `editor-editing.steps.ts` file is quite large and mixes step definitions with a lot of helper logic; consider extracting the generic Slate/TestEditor helpers (e.g., selection, block traversal, paste/undo/redo utilities) into separate reusable modules to keep the step file focused and easier to navigate.
- There are many `waitForTimeout` calls sprinkled through the steps; where possible, replace fixed delays with expectation-based waits (e.g., `expect(...).to...`) to make the tests less flaky and more deterministic.
- The repeated `window`/`__TEST_EDITOR__` access and type definitions for `TestSlate*` structures show similar patterns across helpers; consider centralizing the `getTestEditor`/`getTestWindow` logic and shared types to avoid duplication and reduce the chance of subtle inconsistencies.
## Individual Comments
### Comment 1
<location path="playwright/bdd/steps/editor-editing.steps.ts" line_range="653-680" />
<code_context>
+ });
+}
+
+async function pasteContent(page: Page, html: string, plainText: string) {
+ await page.evaluate(
+ ({ html, plainText }) => {
+ const getTestEditor = () => {
+ const testWindow = window as Window & {
+ __TEST_EDITOR__?: TestSlateEditor;
+ __TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>;
+ };
+
+ return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0];
+ };
+ const editor = getTestEditor();
+
+ if (!editor?.insertData) {
+ throw new Error('No test editor with insertData() found');
+ }
+
+ const dataTransfer = new DataTransfer();
+
+ if (html) dataTransfer.setData('text/html', html);
+ dataTransfer.setData('text/plain', plainText);
+ editor.insertData(dataTransfer);
+ },
+ { html, plainText }
+ );
+
+ await page.waitForTimeout(1000);
+}
+
</code_context>
<issue_to_address>
**suggestion (performance):** `pasteContent` uses a hardcoded 1s sleep after inserting data, which may be longer than necessary and still flaky.
The helper always waits 1000ms after `editor.insertData`, which can unnecessarily slow tests and still be flaky on slower environments. If the editor provides a reliable signal for paste completion (e.g., change in content or a custom event), prefer polling for that condition instead of using a fixed timeout to make the tests faster and more reliable.
```suggestion
async function pasteContent(page: Page, html: string, plainText: string) {
// Capture the initial editor content so we can reliably detect when the paste has been applied.
const initialContent = await page.evaluate(() => {
const getTestEditor = () => {
const testWindow = window as Window & {
__TEST_EDITOR__?: TestSlateEditor;
__TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>;
};
return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0];
};
const editor = getTestEditor();
return editor ? editor.children : null;
});
await page.evaluate(
({ html, plainText }) => {
const getTestEditor = () => {
const testWindow = window as Window & {
__TEST_EDITOR__?: TestSlateEditor;
__TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>;
};
return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0];
};
const editor = getTestEditor();
if (!editor?.insertData) {
throw new Error('No test editor with insertData() found');
}
const dataTransfer = new DataTransfer();
if (html) dataTransfer.setData('text/html', html);
dataTransfer.setData('text/plain', plainText);
editor.insertData(dataTransfer);
},
{ html, plainText }
);
// Wait until the editor content has changed compared to the initial snapshot, or time out.
// This avoids a fixed sleep and makes the tests both faster and more robust.
await page.waitForFunction(
(before) => {
const getTestEditor = () => {
const testWindow = window as Window & {
__TEST_EDITOR__?: TestSlateEditor;
__TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>;
};
return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0];
};
const editor = getTestEditor();
if (!editor) return false;
const current = editor.children;
// If we didn't have an initial snapshot for some reason, treat any content as "changed".
if (before == null) return !!current;
try {
return JSON.stringify(current) !== JSON.stringify(before);
} catch {
// If serialization fails for any reason, fall back to assuming it's changed.
return true;
}
},
initialContent,
{ timeout: 2000 }
);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+653
to
+680
| async function pasteContent(page: Page, html: string, plainText: string) { | ||
| await page.evaluate( | ||
| ({ html, plainText }) => { | ||
| const getTestEditor = () => { | ||
| const testWindow = window as Window & { | ||
| __TEST_EDITOR__?: TestSlateEditor; | ||
| __TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>; | ||
| }; | ||
|
|
||
| return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0]; | ||
| }; | ||
| const editor = getTestEditor(); | ||
|
|
||
| if (!editor?.insertData) { | ||
| throw new Error('No test editor with insertData() found'); | ||
| } | ||
|
|
||
| const dataTransfer = new DataTransfer(); | ||
|
|
||
| if (html) dataTransfer.setData('text/html', html); | ||
| dataTransfer.setData('text/plain', plainText); | ||
| editor.insertData(dataTransfer); | ||
| }, | ||
| { html, plainText } | ||
| ); | ||
|
|
||
| await page.waitForTimeout(1000); | ||
| } |
There was a problem hiding this comment.
suggestion (performance): pasteContent uses a hardcoded 1s sleep after inserting data, which may be longer than necessary and still flaky.
The helper always waits 1000ms after editor.insertData, which can unnecessarily slow tests and still be flaky on slower environments. If the editor provides a reliable signal for paste completion (e.g., change in content or a custom event), prefer polling for that condition instead of using a fixed timeout to make the tests faster and more reliable.
Suggested change
| async function pasteContent(page: Page, html: string, plainText: string) { | |
| await page.evaluate( | |
| ({ html, plainText }) => { | |
| const getTestEditor = () => { | |
| const testWindow = window as Window & { | |
| __TEST_EDITOR__?: TestSlateEditor; | |
| __TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>; | |
| }; | |
| return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0]; | |
| }; | |
| const editor = getTestEditor(); | |
| if (!editor?.insertData) { | |
| throw new Error('No test editor with insertData() found'); | |
| } | |
| const dataTransfer = new DataTransfer(); | |
| if (html) dataTransfer.setData('text/html', html); | |
| dataTransfer.setData('text/plain', plainText); | |
| editor.insertData(dataTransfer); | |
| }, | |
| { html, plainText } | |
| ); | |
| await page.waitForTimeout(1000); | |
| } | |
| async function pasteContent(page: Page, html: string, plainText: string) { | |
| // Capture the initial editor content so we can reliably detect when the paste has been applied. | |
| const initialContent = await page.evaluate(() => { | |
| const getTestEditor = () => { | |
| const testWindow = window as Window & { | |
| __TEST_EDITOR__?: TestSlateEditor; | |
| __TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>; | |
| }; | |
| return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0]; | |
| }; | |
| const editor = getTestEditor(); | |
| return editor ? editor.children : null; | |
| }); | |
| await page.evaluate( | |
| ({ html, plainText }) => { | |
| const getTestEditor = () => { | |
| const testWindow = window as Window & { | |
| __TEST_EDITOR__?: TestSlateEditor; | |
| __TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>; | |
| }; | |
| return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0]; | |
| }; | |
| const editor = getTestEditor(); | |
| if (!editor?.insertData) { | |
| throw new Error('No test editor with insertData() found'); | |
| } | |
| const dataTransfer = new DataTransfer(); | |
| if (html) dataTransfer.setData('text/html', html); | |
| dataTransfer.setData('text/plain', plainText); | |
| editor.insertData(dataTransfer); | |
| }, | |
| { html, plainText } | |
| ); | |
| // Wait until the editor content has changed compared to the initial snapshot, or time out. | |
| // This avoids a fixed sleep and makes the tests both faster and more robust. | |
| await page.waitForFunction( | |
| (before) => { | |
| const getTestEditor = () => { | |
| const testWindow = window as Window & { | |
| __TEST_EDITOR__?: TestSlateEditor; | |
| __TEST_EDITORS__?: Record<string, TestSlateEditor | undefined>; | |
| }; | |
| return testWindow.__TEST_EDITOR__ ?? Object.values(testWindow.__TEST_EDITORS__ ?? {})[0]; | |
| }; | |
| const editor = getTestEditor(); | |
| if (!editor) return false; | |
| const current = editor.children; | |
| // If we didn't have an initial snapshot for some reason, treat any content as "changed". | |
| if (before == null) return !!current; | |
| try { | |
| return JSON.stringify(current) !== JSON.stringify(before); | |
| } catch { | |
| // If serialization fails for any reason, fall back to assuming it's changed. | |
| return true; | |
| } | |
| }, | |
| initialContent, | |
| { timeout: 2000 } | |
| ); | |
| } |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Tests
Result: 56 passed (15.5m)
Summary by Sourcery
Add Playwright BDD configuration and comprehensive editor editing feature coverage using Gherkin-based workflows.
New Features:
Enhancements:
Build: