Improved admin 2FA browser tests #23370
Conversation
WalkthroughThe changes remove the existing two-factor authentication (2FA) end-to-end test file and replace it with a new test suite that uses a Page Object Model design pattern. Three new classes are introduced: Suggested labels
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between bc924ef4a11a06bdaaaacee0cdb821cb544c950f and f13713e. 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ 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: 0
🧹 Nitpick comments (1)
ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (1)
26-29: Consider adding await to the visit methodThe
visit()method should useawaitwhen navigating to ensure the page load completes before subsequent actions.visit() { - this.page.goto('/ghost'); + return this.page.goto('/ghost'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 30b2cb2 and 3e003c61657a07fe4da52b559f964a97d53b8f63.
📒 Files selected for processing (5)
ghost/core/test/e2e-browser/admin/2fa.spec.js(0 hunks)ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-login.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-page.js(1 hunks)ghost/core/test/e2e-browser/admin/two-factor-auth.spec.js(1 hunks)
💤 Files with no reviewable changes (1)
- ghost/core/test/e2e-browser/admin/2fa.spec.js
🧰 Additional context used
🧬 Code Graph Analysis (3)
ghost/core/test/e2e-browser/admin/pages/admin-page.js (2)
ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js (1)
AdminPage(1-1)ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (1)
AdminPage(1-1)
ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (2)
ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js (1)
AdminPage(1-1)ghost/core/test/e2e-browser/admin/two-factor-auth.spec.js (2)
require(2-2)AdminLoginPage(4-4)
ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js (1)
ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (1)
AdminPage(1-1)
🔇 Additional comments (7)
ghost/core/test/e2e-browser/admin/pages/admin-page.js (1)
1-10: Good base class implementation for page objectsThe
AdminPageclass provides a clean base for all admin page objects with a usefullogoutByCookieClearmethod. This follows good object-oriented design principles by abstracting common functionality that will be shared across page objects.ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js (1)
1-11: Good use of page object inheritance and element encapsulationThe implementation follows the Page Object Model pattern correctly by extending the base
AdminPageclass and encapsulating the site title element. The locator strategy using CSS selector.gh-nav-menufor the site title appears to be a resilient way to verify successful login.ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (2)
3-24: Excellent use of accessibility-focused locatorsThe page object properly initializes elements using Playwright's recommended accessibility-focused locators such as
getByLabelandgetByRole. This is a significant improvement over hard-coded CSS/XPath selectors as they're more resilient to UI changes and better represent how users interact with the page.
30-43: Well-structured action methods following AAA patternThe methods for signing in, verifying tokens, and resending tokens are clear, focused, and follow a consistent pattern. Each method encapsulates a specific user action, making the test code more readable and maintainable.
ghost/core/test/e2e-browser/admin/two-factor-auth.spec.js (3)
7-15: Good test setup with clear separation of concernsThe test file correctly uses the
beforeEachhook to ensure a clean state before each test by clearing cookies. The comment explaining why a shared page isn't used provides valuable context for future maintenance.
17-27: Well-structured 2FA authentication testThis test follows the AAA pattern effectively:
- Arrange: Fetch user data and set up login page
- Act: Perform login and 2FA verification
- Assert: Verify successful navigation to dashboard
The use of page objects keeps the test concise and focused on behavior rather than implementation details.
29-42: Comprehensive test for 2FA resend functionalityThis test thoroughly verifies the 2FA resend workflow with appropriate assertions at each step:
- Checks if the "Sent" button appears after resending
- Verifies successful login after using the resent token
The structure follows the same clean AAA pattern as the previous test.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/test/e2e-browser/admin/pages/admin-page.js (3)
1-7: Good implementation of the Page Object base class.This is a solid implementation of the base Page Object class that follows good OOP principles. The constructor properly allows customization of the page URL while providing a sensible default.
However, consider adding JSDoc comments to document the class and its parameters, especially since this will be extended by other page objects.
+/** + * Base admin page object that provides common functionality for all admin pages. + */ class AdminPage { pageUrl = '/'; + /** + * @param {import('@playwright/test').Page} page - Playwright page object + * @param {string} pageUrl - URL path to the admin page + */ constructor(page, pageUrl = '/') { this.page = page; this.pageUrl = pageUrl; }
9-12: Logout implementation is clean and effective.The method clears cookies from the browser context, which is a reliable way to ensure the user is logged out between tests. This approach is better than navigating through UI elements to perform logout, making tests more robust.
Consider a more general method name if this is intended for broader session cleanup beyond just logout.
- async logoutByCookieClear() { + /** + * Clears cookies to log out and reset the session state + */ + async clearSession() { const context = await this.page.context(); await context.clearCookies(); }
14-16: Simple and effective page navigation method.This method handles the basic page navigation well. For additional flexibility, consider allowing the method to accept an optional URL parameter to override the instance's
pageUrl.- async visit() { - await this.page.goto(this.pageUrl); + /** + * Navigate to the page URL + * @param {string} [url] - Optional URL to override the default page URL + */ + async visit(url) { + await this.page.goto(url || this.pageUrl); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 3e003c61657a07fe4da52b559f964a97d53b8f63 and 5fd0b3bd52d85a589f110e4bf696d16745839f81.
📒 Files selected for processing (2)
ghost/core/test/e2e-browser/admin/pages/admin-login.page.js(1 hunks)ghost/core/test/e2e-browser/admin/pages/admin-page.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/e2e-browser/admin/pages/admin-login.page.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/core/test/e2e-browser/admin/pages/admin-page.js (2)
ghost/core/test/e2e-browser/admin/pages/admin-login.page.js (1)
AdminPage(1-1)ghost/core/test/e2e-browser/admin/pages/admin-dashboard.page.js (1)
AdminPage(1-1)
🔇 Additional comments (1)
ghost/core/test/e2e-browser/admin/pages/admin-page.js (1)
19-20: Appropriate module export.The class is properly exported for use in other test files.
ibalosh
changed the title
no-ref Browser tests like admin 2fa tests can be improved. They should be easier to read, with less hard coded values, like repeatable locators. To make them easier to maintain, more readable and reusable, 2fa admin test was refactored with usage of page objects, less britle locators, better variable names, closer match to AAA (arrange act assert) way of writing test cases. If the style of this test is acceptable, it can be used as blueprint for other tests to follow it.
cmraible
force-pushed
the
improve-e2e-tests
branch
from
bc924ef to
f13713e
Compare
|
Hi @ibalosh thanks for this, looks really good! I'm just rebasing and kicking off CI, if everything checks out I'm aiming to merge this today |
cmraible
changed the title
|
thanks @cmraible ! |
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.
ref #23370 * 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 `createPostDraft` in 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.js` and `e2e-browser-utils.js` , which makes it hard to figure out in this and other tests that `user 1` from data generator is used. This should be more flexible, and something to consider to decouple a bit in future.
ref #23370 * 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 `createPostDraft` in 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.js` and `e2e-browser-utils.js` , which makes it hard to figure out in this and other tests that `user 1` from data generator is used. This should be more flexible, and something to consider to decouple a bit in future.
3 participants
I have decided to refactor one of the admin browser tests - two factor authentication.
The reason for this is based on an observation that the browser tests can be improved in my opinion.
I see couple of reasons for improvement
If tests are more resilient on refactoring, developers will be more motivated on updating them if they fail.
Less false positives, more motivation for the updates.
The updates made to the test
This should allow better readability of the code, allow you to reuse pages with their methods, without the need of hard coding locators, or needing to update them in multiple places.
In my opinion, this should make it easier to update, and add new tests. I have only refactored one file, to see if you like this direction, and see the value too in the update.