Skip to content
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

Added browser tests covering the editor's basic saving behavior #22577

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cmraible
Copy link
Collaborator

@cmraible cmraible commented Mar 19, 2025

ref https://linear.app/ghost/issue/ONC-833/support-escalation-re-issue-with-saving-articles-urgent

  • We recently had a bug that caused the editor to fail to save if only the body was filled, and the title was left blank. With a blank title, the slug wasn't generating, which caused the initial POST request to fail with a validation error.
  • We've had similar issues in the past and added some admin acceptance tests, but they didn't and wouldn't catch a case like this, where the bug is caused by an interaction between the editor, admin, and the Admin API.
  • This commit adds a new browser test file for the editor and two basic test cases as a starting point. It uses Playwright's "Page Object Model" concept to make the tests easier to read and write, and to keep the test's as DRY as we can.
  • It also adds a missing test in the Admin Acceptance tests, although this test wouldn't have caught this bug

Copy link
Contributor

coderabbitai bot commented Mar 19, 2025

Walkthrough

The changes introduce new end-to-end and acceptance tests for the editor’s saving functionality. An asynchronous test case for saving content in the lexical editor has been added, which simulates user interaction by clicking, pasting text using the new pasteInEditor helper, and verifying that the content is saved with the correct URL. In addition, a new test suite in the E2E admin tests verifies the saving behavior when either the title or the body of a post is changed, using the EditorPage class. The EditorPage class, which follows a Page Object Model pattern, provides methods for navigating to the editor, interacting with title and body inputs, triggering autosave, and checking post status responses. These changes ensure that both the lexical editor and the general editor maintain proper saving functionalities, as verified through comprehensive test flows.

Possibly related PRs

Suggested reviewers

  • 9larsons

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ghost/admin/tests/acceptance/editor/lexical-test.js

Oops! Something went wrong! :(

ESLint: 8.44.0

Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
Require stack:

  • /ghost/admin/.eslintrc.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1248:15)
    at Function.resolve (node:internal/modules/helpers:145:19)
    at Object.resolve (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2346:46)
    at ConfigArrayFactory._loadParser (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3325:39)
    at ConfigArrayFactory._normalizeObjectConfigDataBody (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3099:43)
    at _normalizeObjectConfigDataBody.next ()
    at ConfigArrayFactory._normalizeObjectConfigData (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3040:20)
    at _normalizeObjectConfigData.next ()
    at ConfigArrayFactory.loadInDirectory (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2886:28)
    at CascadingConfigArrayFactory._loadConfigInAncestors (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3871:46)

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f3c4f3c and cd90d5e.

📒 Files selected for processing (3)
  • ghost/admin/tests/acceptance/editor/lexical-test.js (2 hunks)
  • ghost/core/test/e2e-browser/admin/editor.spec.js (1 hunks)
  • ghost/core/test/e2e-browser/utils/pages/EditorPage.js (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Coverage
  • GitHub Check: Browser tests
  • GitHub Check: Ghost-CLI tests
🔇 Additional comments (9)
ghost/admin/tests/acceptance/editor/lexical-test.js (2)

6-6: Good addition of the pasteInEditor helper

The import of the pasteInEditor helper function enhances test capabilities by providing a way to simulate pasting content into the editor.


78-91: Well-structured test case for content saving

This test case properly addresses the bug mentioned in ONC-833 where the editor should save content even when only the body is filled. The test follows a clear flow:

  1. Visit the editor
  2. Wait for the lexical editor to load
  3. Focus the editor and paste content
  4. Verify the content is saved
  5. Check the resulting URL

The test complements the existing suite and ensures that body-only changes trigger proper saving behavior.

ghost/core/test/e2e-browser/admin/editor.spec.js (1)

1-22: Well-structured test suite using Page Object Model

This new test file properly implements end-to-end tests for the editor's saving behavior using Playwright. The tests cover two critical scenarios:

  1. Saving when only title is changed and blurred
  2. Saving when only content is changed for the first time

These tests directly address the bug where a post should be properly saved when content is added without a title. The use of the EditorPage class follows the Page Object Model pattern, improving test maintainability and readability.

Both test cases have clear assertions checking that the post status becomes "Draft" after the respective actions, which verifies the saving functionality is working correctly.

ghost/core/test/e2e-browser/utils/pages/EditorPage.js (6)

3-18: Well-designed Page Object Model with comprehensive JSDoc comments

The EditorPage class has a well-structured constructor that initializes all necessary page locators. The JSDoc comments provide clear documentation for the class and its options. The use of Playwright's Page Object Model pattern will help maintain clean and DRY test code.


20-34: Good implementation of navigation with type validation

The goto method properly handles navigation to both post and page editors with appropriate error handling for invalid types. This will allow test flexibility while maintaining robustness.


36-63: Well-implemented title manipulation methods

The methods for manipulating the title (blurTitle, fillTitle, and setTitle) are well designed with appropriate options for flexibility. The setTitle method properly handles clearing existing text before setting new content, and the optional blur parameter enables testing the blurring behavior that triggers saves.


65-85: Solid implementation of body content manipulation

The methods for manipulating the body content (fillBody and setBody) follow the same pattern as the title methods, providing consistency. The setBody method includes the option to trigger autosave, which is useful for testing the saving behavior addressed in the PR.


87-94: Clever autosave triggering implementation

The triggerAutosave method uses clock manipulation to expedite testing, saving approximately 3 seconds per test run by fast-forwarding the clock instead of waiting for the actual timeout. This is an excellent optimization for test speed.


96-124: Robust API response handling

The methods for waiting for API responses (waitForCreatePostResponse and getCreatePostPromise) are well-implemented with proper filtering conditions to capture specifically the POST requests with status 201. This ensures accurate verification of the saving functionality.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@cmraible cmraible force-pushed the add-editor-autosave-tests branch from 34fd926 to 28af5df Compare March 19, 2025 21:59
@cmraible cmraible changed the title Add editor autosave tests Added browser tests covering the editor's saving behavior Mar 20, 2025
@cmraible cmraible changed the title Added browser tests covering the editor's saving behavior Added browser tests covering the editor's basic saving behavior Mar 20, 2025
@cmraible cmraible marked this pull request as ready for review March 20, 2025 01:59
@ErisDS
Copy link
Member

ErisDS commented Mar 22, 2025

Curious about this one - did the editor fail, or did the API fail to do what was expected?

Would you mind taking a moment to outline the exact failure mechanism we're trying to defend against?

Copy link
Collaborator Author

cmraible commented Mar 24, 2025

The mechanism of the bug was:

  • The ember controller (not the lexical editor itself) failed to generate a slug if the title was empty. Normally it would generate untitled-{x} as the slug if the title was missing
  • When the body of the post (or anything in the sidebar) was changed, it would trigger a POST /posts request to save the draft
  • The backend would (correctly) respond with a validation error, complaining that there is no slug, and the save would fail.

There's definitely a missing unit test or 2 for the slug-generating behavior, which I'll add to this PR as well, and should be sufficient to protect against this exact failure mode in the future.

This browser/e2e test is intended as more of a smoke test / safety blanket to protect against any other bugs of this general breed, where a frontend bug can have unintended cascading failures ultimately resulting in posts not saving.

I have a todo to look into the admin test more. The way it's currently written, it doesn't catch this bug because (I think) it's using a mock API that bypasses the validation, so it just assumes the backend saves successfully — hence the browser test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants