Refactored admin i18n tests #23513
Refactored admin i18n tests #23513
Conversation
ref TryGhost#23370 Related to improvement to 2fa admin auth tests, decided to add more improvements, through cleaning up i18n and more usage of page objects.
WalkthroughThe pull request refactors the end-to-end browser tests for the Ghost admin interface by adopting a page object model. It introduces new page object classes for posts, email previews, and publication language settings, and updates existing page classes to include URL path parameters. The main i18n test is rewritten to utilize these page objects instead of direct Playwright commands and helper functions. Additionally, an index module is created to export all admin page objects for easier imports, and related test files are updated to use this centralized module. Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
ghost/core/test/e2e-browser/admin/pages/admin-posts.page.js (2)
22-25: Modernize the wait pattern and improve consistency.Consider using Playwright's modern locator-based waiting patterns instead of
waitForSelector, and improve method naming consistency.async emailPreviewForPost() { - this.page.waitForSelector(this.#emailPreviewPostButtonsSelector); - this.#emailPreviewPostButtons.first().click(); + await this.#emailPreviewPostButtons.first().waitFor(); + await this.#emailPreviewPostButtons.first().click(); }Alternative suggestion for method naming consistency:
-async emailPreviewForPost() { +async previewPostEmail() {
4-6: Consider adding JSDoc for better documentation.The private fields could benefit from documentation to clarify their purpose, especially for the selector string.
class AdminPostsPage extends AdminPage { + /** @private {Locator} Locators for publish preview buttons */ #previewPostButtons = null; + /** @private {Locator} Locators for email preview buttons */ #emailPreviewPostButtons = null; + /** @private {string} CSS selector for email preview buttons */ #emailPreviewPostButtonsSelector = '[data-test-button="email-preview"]';ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js (1)
20-23: Consider adding validation for language parameter.The
setLanguagemethod doesn't validate the input parameter, which could lead to unexpected behavior.async setLanguage(language) { + if (!language || typeof language !== 'string') { + throw new Error('Language must be a non-empty string'); + } await this.languageField.fill(language); await this.#saveButton.click(); }ghost/core/test/e2e-browser/admin/pages/admin-posts-email-preview.page.js (1)
1-24: Well-structured page object with good practices!The class follows the page object pattern excellently with proper encapsulation using private fields and clear method responsibilities.
Consider these enhancements for improved robustness:
class AdminPostsEmailPreviewPage extends AdminPage { #tableBody = null; /** * @param {import('@playwright/test').Page} page - playwright page object */ constructor(page) { super(page); this.#tableBody = page.frameLocator('iframe.gh-pe-iframe').locator('tbody').first(); } + /** + * Gets the text content of the email preview table body + * @returns {Promise<string>} The text content + */ async content() { + await this.#tableBody.waitFor({ state: 'visible' }); return await this.#tableBody.textContent(); } + /** + * Closes the email preview modal using the Escape key + * @returns {Promise<void>} + */ async closeEmailPreviewForPost() { await this.page.keyboard.press('Escape'); + // Wait for the modal to close + await this.page.waitForSelector('iframe.gh-pe-iframe', { state: 'detached' }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ghost/core/test/e2e-browser/admin/i18n.spec.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-posts-email-preview.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-posts.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/index.js(1 hunks)ghost/core/test/e2e-browser/admin/two-factor-auth.spec.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ghost/core/test/e2e-browser/admin/two-factor-auth.spec.js (1)
ghost/core/test/e2e-browser/admin/i18n.spec.js (3)
require(1-1)require(3-3)require(5-10)
ghost/core/test/e2e-browser/admin/pages/index.js (2)
ghost/core/test/e2e-browser/admin/i18n.spec.js (3)
require(1-1)require(3-3)require(5-10)ghost/core/test/e2e-browser/admin/two-factor-auth.spec.js (2)
require(2-2)require(5-8)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 22.13.1)
🔇 Additional comments (5)
ghost/core/test/e2e-browser/admin/pages/index.js (1)
1-6: LGTM! Good centralization pattern.The centralized export pattern improves maintainability and provides a clean single import point for all admin page objects. This aligns well with the Page Object Model adoption mentioned in the PR objectives.
ghost/core/test/e2e-browser/admin/two-factor-auth.spec.js (1)
5-8: LGTM! Clean import refactoring.The refactoring to use centralized imports from
./pages/indexis well-executed and consistent with the Page Object Model adoption. This improves maintainability by providing a single import source for page objects.ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js (1)
9-10:✅ Verification successful
Good refactor to standardize path handling!
The addition of the path parameter to the superclass constructor aligns well with the page object pattern and improves consistency.
Please verify that all other AdminPage subclasses have been updated consistently to pass their respective paths:
🏁 Script executed:
#!/bin/bash # Description: Check all AdminPage subclasses for consistent constructor patterns # Expected: All should pass a path parameter to super() ast-grep --pattern $'class $_ extends AdminPage { $$$ constructor($_) { super($_, $_); $$$ } $$$ }'Length of output: 10170
All AdminPage subclasses updated consistently
I’ve verified that every
AdminPagesubclass now callssuper(page, <path>)with the correct route:
ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js:super(page, '/ghost/#/settings/publication-language')ghost/core/test/e2e-browser/admin/pages/admin-posts.page.js:super(page, '/ghost/#/posts')ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js:super(page, '/ghost')ghost/core/test/e2e-browser/admin/pages/admin-login.page.js:super(page, '/ghost')All paths are applied consistently—approving the change.
ghost/core/test/e2e-browser/admin/i18n.spec.js (2)
5-10: Excellent adoption of centralized imports!The centralized import from the index file improves maintainability and makes dependencies clear.
14-38: Outstanding refactor that significantly improves test maintainability!This refactor successfully transforms the test from using direct Playwright commands to a clean page object model approach. Key improvements:
- Better abstraction: Page-specific actions are now encapsulated in appropriate page objects
- Improved readability: The test flow is more declarative and easier to understand
- Enhanced maintainability: Changes to page structure will only require updates to the relevant page objects
- Consistent patterns: All page interactions follow the same object-oriented approach
The test logic remains intact while being much more maintainable. The assertions are clear and the flow from language setting → post creation → preview → verification → cleanup is well-structured.
ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js (1)
3-8: LGTM! Encapsulation properly implemented.The private field declarations follow good encapsulation practices with the
#prefix. This addresses the previous concern about field visibility and maintains proper object-oriented design principles.
🧹 Nitpick comments (3)
ghost/core/test/e2e-browser/admin/pages/admin-posts-email-preview.page.js (1)
7-14: Well-implemented constructor with good iframe handling.The constructor properly initializes the base class and uses Playwright's
frameLocatorAPI correctly for iframe content access. The JSDoc documentation is comprehensive and helpful.Consider the robustness of using
.first()on the tbody locator - ensure this targets the correct element consistently across different email preview scenarios.ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (1)
29-33: Consider renaming parameters for clarity.The parameter names
usernameFieldandpasswordFieldare misleading as they suggest field elements rather than values. Consider renaming them to better reflect their purpose.-async signIn(usernameField, passwordField) { - await this.#usernameField.fill(usernameField); - await this.#passwordField.fill(passwordField); +async signIn(email, password) { + await this.#usernameField.fill(email); + await this.#passwordField.fill(password); await this.#signInButton.click(); }ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js (1)
4-4: Consider making the default language configurable.The hardcoded default language
'en'might not be suitable for all test environments or deployment configurations. Consider making this configurable through a constructor parameter or environment variable.class AdminPublicationPage extends AdminPage { - #defaultLanguage = 'en'; + #defaultLanguage;And update the constructor:
- constructor(page) { - super(page, '/ghost/#/settings/publication-language'); + constructor(page, defaultLanguage = 'en') { + super(page, '/ghost/#/settings/publication-language'); + this.#defaultLanguage = defaultLanguage;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/core/test/e2e-browser/admin/pages/admin-login.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-posts-email-preview.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-posts.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/test/e2e-browser/admin/pages/index.js
- ghost/core/test/e2e-browser/admin/pages/admin-posts.page.js
🔇 Additional comments (10)
ghost/core/test/e2e-browser/admin/pages/admin-posts-email-preview.page.js (4)
1-5: Excellent POM implementation structure!The class correctly extends
AdminPageand uses modern JavaScript private field syntax for encapsulation. The import path and class declaration follow proper patterns for the Page Object Model.
16-19: Excellent wait strategy and content retrieval.The method correctly waits for the element to be visible before retrieving content, which prevents common timing issues in E2E tests. The implementation is clean and follows the single responsibility principle.
21-24: Great accessibility-focused implementation with proper state verification.Using
keyboard.press('Escape')is excellent for accessibility testing, and waiting for the element to bedetachedensures the preview is fully closed before proceeding. This prevents race conditions in subsequent test steps.
25-27: Standard and correct module export.The CommonJS export pattern is appropriate and consistent with the project's module system.
ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (3)
4-11: Good use of private fields for encapsulation!The refactoring to use private class fields enhances encapsulation and prevents external access to internal page element references. This aligns well with the Page Object Model pattern.
16-27: Well-structured constructor with semantic selectors!The initialization of private fields with semantic locators (getByLabel, getByRole) improves both accessibility and test maintainability.
44-46:✅ Verification successful
Verify the getter pattern usage.
Only
sentTwoFactorCodeButtonhas a getter while other private fields don't. Ensure this selective exposure is intentional and aligns with test requirements.Run the following script to check how this getter is used in tests:
🏁 Script executed:
#!/bin/bash # Description: Check usage of sentTwoFactorCodeButton getter in test files # Search for usage of sentTwoFactorCodeButton in test files rg -A 3 "sentTwoFactorCodeButton" --glob "*.spec.js" --glob "*.test.js"Length of output: 468
No changes required for getter pattern. The
sentTwoFactorCodeButtongetter is explicitly used inghost/core/test/e2e-browser/admin/two-factor-auth.spec.jsfor visibility assertions. No other private fields are referenced externally, so this selective exposure is intentional and aligns with test requirements.ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js (3)
20-22: LGTM! Good encapsulation with controlled access.The public getter provides controlled access to the private field while maintaining encapsulation. This is a clean implementation of the accessor pattern.
12-18: LGTM! Constructor follows Playwright best practices.The constructor properly initializes the page object with appropriate selectors using
getByTestId,getByRole, andgetByLabel. These are robust selectors that align with Playwright testing best practices.
33-36: LGTM! Clean reset functionality.The reset method provides a straightforward way to return to default settings, which is useful for test cleanup and initialization.
ghost/core/test/e2e-browser/admin/pages/admin-publication.page.js
Outdated
Show resolved
Hide resolved
|
hey @cmraible I hope you don't mind, I had some spare time last weekend, and I refactored bit more code - to one more existing e2e test. If nothing else, it can be use later in new e2e suite you are working on. |
Related to 2fa e2e tests, I have decided to refactor one more e2e test.
This is using more reusable page objects, and cleans up a bit this test to be more readable.
As more tests are added, POM objects will be improved, as it will help figure out which parts of the pages can be reused.
I did not touch things like
createPostDraftin this refactor, since that function is reused a lot, and change should be done gradually.Note:
I see that fixtures are setup very deeply down in
ghost-test.jsande2e-browser-utils.js, which makes it hard to figure out in this and other tests thatuser 1from data generator is used. This should be more flexible, and something to consider to decouple a bit in future.