Refactor Cypress Auth Test to POM and Improve Test Stability #9024
Conversation
WalkthroughThe changes involve the removal of two Cypress test files, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
cypress/e2e/homepage_spec/UserLogin.cy.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (7)
cypress/e2e/homepage_spec/UserLogin.cy.ts (2)
1-2: Add TypeScript type annotations for better maintainability.While the code follows POM best practices, adding type annotations would improve maintainability and IDE support.
-import LoginPage from "pageobject/Login/LoginPage"; -const loginPage = new LoginPage(); +import { LoginPage } from "pageobject/Login/LoginPage"; +const loginPage: LoginPage = new LoginPage();
1-45: Consider adding test cases for edge scenarios.The test suite has good basic coverage, but consider adding these scenarios:
- Session timeout handling
- Network error scenarios
- Concurrent login attempts
- Browser refresh during login process
- Password complexity validation
Also, consider creating custom commands for common assertions to improve code reuse.
Would you like me to help create these additional test cases or custom commands?
cypress/pageobject/Login/LoginPage.ts (4)
16-23: LGTM! Consider adding type safety for credentials.Good implementation of parameterized login for both successful and failed scenarios. The boolean parameter effectively controls the test flow.
Consider creating an interface for credentials to make the code more maintainable:
interface LoginCredentials { username: string; password: string; } const VALID_CREDENTIALS: LoginCredentials = { username: "devdistrictadmin", password: "Coronasafe@123" }; const INVALID_CREDENTIALS: LoginCredentials = { username: "devdistrictadmin", password: "Corona" };
42-44: Consider adding API verification for sign out.Good implementation using the helper function. However, consider adding API interception to verify the sign-out request completes successfully.
Add API verification similar to other methods:
interceptSignOutReq(): void { cy.intercept("POST", "**/api/v1/auth/logout").as("signOut"); } verifySignOutReq(): void { cy.wait("@signOut").its("response.statusCode").should("eq", 200); }
62-84: Consider parameterizing expected status codes.Good implementation of API interception and verification. However, the expected status codes could be parameterized for more flexible testing.
Consider this approach:
interface ApiVerification { alias: string; expectedStatus: number; } verifyApiResponse({ alias, expectedStatus }: ApiVerification): void { cy.wait(`@${alias}`).its("response.statusCode").should("eq", expectedStatus); }This would allow more flexible status code verification while reducing code duplication.
Stability improvements need attention - hardcoded waits found
Multiple files still contain hardcoded waits that should be replaced with proper API interceptions and assertions:
PatientLogupdate.ts: Contains 5 instances of hardcoded waits (2000-3000ms)PatientFileupload.ts: Contains 3 instances including a 5000ms waitUsersManage.cy.ts: Contains 3 instances of 5000ms waitsPatientConsultation.ts: Contains waits of 5000ms and 3000msPatientRegistration.cy.ts: Contains 6 instances of hardcoded waitsWhile API interceptions are well implemented in the LoginPage and other files, the presence of numerous hardcoded waits indicates that the stability improvements are incomplete. These should be replaced with proper API interceptions and assertions for better test reliability.
🔗 Analysis chain
Line range hint
1-84: Verify test stability improvements.The changes align well with the PR objectives, implementing POM pattern and improving stability through API verification. Let's verify the stability improvements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining hardcoded waits in test files echo "Checking for hardcoded waits..." rg "cy\.wait\(\d+" cypress/ # Check for consistent use of API interception echo "Checking API interception usage..." rg "cy\.intercept" cypress/Length of output: 9813
src/components/Auth/Login.tsx (1)
Line range hint
26-427: Consider splitting the component for better maintainability.The component currently handles both login and forgot password flows. Consider splitting it into separate components:
LoginForm.tsx: Handle login flowForgotPasswordForm.tsx: Handle password recovery flowAuthLayout.tsx: Handle common layout elementsThis would improve:
- Code maintainability
- Component testing
- Reusability
- Separation of concerns
Additionally, consider extracting form validation logic into a custom hook for reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cypress/e2e/auth_spec/ForgotPassword.cy.ts(0 hunks)cypress/e2e/auth_spec/auth.cy.ts(0 hunks)cypress/e2e/homepage_spec/UserLogin.cy.ts(1 hunks)cypress/pageobject/Login/LoginPage.ts(2 hunks)src/components/Auth/Login.tsx(3 hunks)
💤 Files with no reviewable changes (2)
- cypress/e2e/auth_spec/ForgotPassword.cy.ts
- cypress/e2e/auth_spec/auth.cy.ts
🔇 Additional comments (4)
cypress/pageobject/Login/LoginPage.ts (1)
29-29: LGTM! Consistent use of submitButton command.
The change aligns with other login methods and improves test stability.
src/components/Auth/Login.tsx (3)
339-339: LGTM: Semantic ID for forgot password button.
The added ID follows clear naming conventions and supports the test automation objectives.
375-375: LGTM: Clear ID for navigation button.
The ID addition maintains consistency with the naming pattern and clearly indicates the button's purpose.
409-409: LGTM: Descriptive ID for form submission button.
The ID addition completes the set of button identifiers needed for test automation while maintaining naming consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
cypress/pageobject/Login/LoginPage.ts (2)
16-23: Consider externalizing test credentials.While the implementation is good, consider moving the hardcoded credentials to a configuration file or environment variables for better maintainability and security.
Example structure:
// cypress/fixtures/credentials.json { "districtAdmin": { "username": "devdistrictadmin", "password": "Coronasafe@123", "invalidPassword": "Corona" } }
42-60: Enable method chaining for better test flow.Consider returning
thisfrom void methods to enable method chaining, which can make tests more readable and fluent.Example refactor:
- clickSignOutBtn(): void { + clickSignOutBtn(): this { cy.verifyAndClickElement("#sign-out-button", "Sign Out"); + return this; } - fillUserNameInForgotPasswordForm(userName: string): void { + fillUserNameInForgotPasswordForm(userName: string): this { cy.get("#forgot_username").type(userName); + return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/pageobject/Login/LoginPage.ts(2 hunks)src/components/Auth/Login.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Auth/Login.tsx
|
Hey @nihal467 can you please review this PR |
|
👋 Hi, @JavidSumra, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…avidSumra/care_fe into issues/9015/refactor-cypress-auth-test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.env (1)
10-11: Consider using separate environment files for different environments.Instead of commenting out the production URL, consider:
- Using
.env.developmentfor local development- Using
.env.testfor test environment- Using
.env.productionfor productionThis helps prevent accidental commits of development configurations to production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.env(1 hunks)cypress/e2e/homepage_spec/UserLogin.cy.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/homepage_spec/UserLogin.cy.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
cypress/e2e/homepage_spec/UserLogin.cy.ts (1)
21-27: Enhance error case validation.Add more comprehensive validation for the error scenario:
- Verify error message visibility
- Validate form state after error
- it("Display an error when logging in as admin with incorrect password", () => { + it("should display appropriate error message for invalid credentials", { retries: 2 }, () => { loginPage.interceptLoginReq(); loginPage.loginManuallyAsDistrictAdmin(false); loginPage.verifyLoginReq(); cy.verifyNotification("No active account found with the given credentials"); + // Verify form state after error + cy.get('input[name="password"]').should('have.class', 'error'); + cy.get('button[type="submit"]').should('be.enabled'); cy.closeNotification(); });cypress/pageobject/Login/LoginPage.ts (5)
16-23: Consider externalizing test credentialsWhile the conditional logic for handling correct/incorrect credentials is good, having hardcoded credentials in the test code could make maintenance difficult. Consider moving these to environment variables or a test configuration file.
// config/test-credentials.ts export const TEST_CREDENTIALS = { districtAdmin: { username: 'devdistrictadmin', validPassword: 'Coronasafe@123', invalidPassword: 'Corona' } }; // LoginPage.ts import { TEST_CREDENTIALS } from '../config/test-credentials'; loginManuallyAsDistrictAdmin(isCorrectCredentials: boolean = true): void { const { username, validPassword, invalidPassword } = TEST_CREDENTIALS.districtAdmin; cy.get("input[id='username']").type(username); cy.get("input[id='password']").type(isCorrectCredentials ? validPassword : invalidPassword); cy.submitButton("Login"); }
46-52: Consider centralizing selectorsThe methods are well-structured, but consider centralizing the selector strings to make maintenance easier and prevent duplication.
// Constants at the top of the file private readonly SELECTORS = { forgotUsername: '#forgot_username', resetLinkBtn: '#send-reset-link-btn' } as const; // Updated methods fillUserNameInForgotPasswordForm(userName: string): void { cy.get(this.SELECTORS.forgotUsername).type(userName); } clickSendResetLinkBtn(): void { cy.verifyAndClickElement(this.SELECTORS.resetLinkBtn, "Send Reset Link"); }
62-64: Add type safety for button textConsider using an enum or union type for the button text parameter to prevent errors from typos or invalid text.
type ForgotPasswordText = 'Forgot Password?' | 'Reset Password'; clickForgotPasswordButton(text: ForgotPasswordText): void { cy.verifyAndClickElement("#forgot-pass-btn", text); }
90-96: Enhance UI verification methods flexibilityConsider these improvements for better type safety and flexibility:
- Add type safety for the text array
- Allow custom selectors for different scenarios
type HeadingText = 'Forgot Password' | 'Reset Password' | 'Password Reset Instructions'; interface VerificationSelectors { loginButton?: string; forgotPasswordHeading?: string; } private readonly DEFAULT_SELECTORS: VerificationSelectors = { loginButton: '#login-button', forgotPasswordHeading: '#forgot-password-heading' }; verifyLoginButtonPresence(selector?: string): void { cy.verifyContentPresence(selector ?? this.DEFAULT_SELECTORS.loginButton, ["Login"]); } verifyForgotPasswordHeading(text: HeadingText[], selector?: string): void { cy.verifyContentPresence(selector ?? this.DEFAULT_SELECTORS.forgotPasswordHeading, text); }
Line range hint
1-99: Consider splitting the class for better separation of concernsThe
LoginPageclass currently handles multiple responsibilities:
- Login functionality
- Password reset functionality
- API interceptions
- UI verifications
Consider splitting this into smaller, more focused classes following the Single Responsibility Principle:
LoginPage- core login functionalityPasswordResetPage- password reset functionalityApiInterceptor- API interception utilitiesUIVerifier- UI verification utilitiesExample structure:
// ApiInterceptor.ts class ApiInterceptor { interceptLoginReq(): void { /* ... */ } verifyLoginReq(): void { /* ... */ } // ... other API methods } // PasswordResetPage.ts class PasswordResetPage { fillUserNameInForgotPasswordForm(): void { /* ... */ } clickSendResetLinkBtn(): void { /* ... */ } // ... other password reset methods } // LoginPage.ts class LoginPage { private api: ApiInterceptor; private passwordReset: PasswordResetPage; constructor() { this.api = new ApiInterceptor(); this.passwordReset = new PasswordResetPage(); } // ... core login methods }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cypress/e2e/homepage_spec/UserLogin.cy.ts(1 hunks)cypress/pageobject/Login/LoginPage.ts(2 hunks)src/components/Auth/Login.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Auth/Login.tsx
🔇 Additional comments (1)
cypress/pageobject/Login/LoginPage.ts (1)
29-29: LGTM!
The change to use cy.submitButton is consistent with other login methods and improves maintainability.
| import LoginPage from "pageobject/Login/LoginPage"; | ||
|
|
||
| const loginPage = new LoginPage(); | ||
| const userName = "dummy_user_1"; | ||
| const forgotPasswordHeading = "Forgot password?"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move test data to fixtures and improve constant scoping.
Consider these improvements:
- Move test data to a fixture file for better maintainability
- Move constants inside the relevant describe blocks where they are used
import LoginPage from "pageobject/Login/LoginPage";
const loginPage = new LoginPage();
-const userName = "dummy_user_1";
-const forgotPasswordHeading = "Forgot password?";
+// Move to cypress/fixtures/auth.json
+{
+ "testUsers": {
+ "dummy_user_1": {
+ "username": "dummy_user_1"
+ }
+ },
+ "ui": {
+ "forgotPasswordHeading": "Forgot password?"
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| loginPage.interceptResetLinkReq(); | ||
| loginPage.clickSendResetLinkBtn(); | ||
| loginPage.verifyResetLinkReq(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add API response verification.
Enhance API interception to verify response status and body:
loginPage.interceptResetLinkReq();
loginPage.clickSendResetLinkBtn();
- loginPage.verifyResetLinkReq();
+ // Verify API response
+ cy.wait('@resetLinkReq').then((interception) => {
+ expect(interception.response.statusCode).to.equal(200);
+ expect(interception.response.body).to.have.property('message', 'Reset link sent successfully');
+ });Committable suggestion skipped: line range outside the PR's diff.
| it("Log in as admin with correct password", () => { | ||
| loginPage.loginManuallyAsDistrictAdmin(); | ||
| loginPage.interceptFacilityReq(); | ||
| loginPage.verifyFacilityReq(); | ||
| loginPage.ensureLoggedIn(); | ||
| loginPage.clickSignOutBtn(); | ||
| loginPage.verifyLoginPageUrl(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test stability and coverage for successful login.
The test needs the following improvements:
- Move API interception before the login action
- Add retry mechanism for flaky scenarios
- Add validation for UI elements
- it("Log in as admin with correct password", () => {
+ it("should successfully log in as admin and verify facility access", { retries: 2 }, () => {
+ loginPage.interceptFacilityReq();
loginPage.loginManuallyAsDistrictAdmin();
- loginPage.interceptFacilityReq();
loginPage.verifyFacilityReq();
loginPage.ensureLoggedIn();
+ // Verify UI elements after successful login
+ cy.get('#user-profile-name').should('be.visible');
loginPage.clickSignOutBtn();
loginPage.verifyLoginPageUrl();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("Log in as admin with correct password", () => { | |
| loginPage.loginManuallyAsDistrictAdmin(); | |
| loginPage.interceptFacilityReq(); | |
| loginPage.verifyFacilityReq(); | |
| loginPage.ensureLoggedIn(); | |
| loginPage.clickSignOutBtn(); | |
| loginPage.verifyLoginPageUrl(); | |
| }); | |
| it("should successfully log in as admin and verify facility access", { retries: 2 }, () => { | |
| loginPage.interceptFacilityReq(); | |
| loginPage.loginManuallyAsDistrictAdmin(); | |
| loginPage.verifyFacilityReq(); | |
| loginPage.ensureLoggedIn(); | |
| // Verify UI elements after successful login | |
| cy.get('#user-profile-name').should('be.visible'); | |
| loginPage.clickSignOutBtn(); | |
| loginPage.verifyLoginPageUrl(); | |
| }); |
| describe("Reset user's password using email", () => { | ||
| beforeEach(() => { | ||
| cy.awaitUrl("/", true); | ||
| }); | ||
|
|
||
| it("Send a password reset link and navigate back to the login page", () => { | ||
| loginPage.clickForgotPasswordButton(forgotPasswordHeading); | ||
| loginPage.verifyForgotPasswordHeading([forgotPasswordHeading]); | ||
| loginPage.fillUserNameInForgotPasswordForm(userName); | ||
| loginPage.interceptResetLinkReq(); | ||
| loginPage.clickSendResetLinkBtn(); | ||
| loginPage.verifyResetLinkReq(); | ||
| cy.verifyNotification("Password Reset Email Sent"); | ||
| cy.closeNotification(); | ||
| loginPage.clickBackButton(); | ||
| loginPage.verifyLoginPageUrl(); | ||
| loginPage.verifyLoginButtonPresence(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Combine password reset tests and add negative scenarios.
As per PR objectives and previous review comments:
- Combine all three password reset test cases into one comprehensive test
- Add negative test cases for invalid scenarios
- Add validation for edge cases
describe("Reset user's password using email", () => {
+ const forgotPasswordHeading = "Forgot password?";
+
beforeEach(() => {
cy.awaitUrl("/", true);
+ cy.fixture('auth.json').as('authData');
});
- it("Send a password reset link and navigate back to the login page", () => {
+ it("should handle password reset workflow with various scenarios", () => {
+ // Test Case 1: Invalid email format
+ loginPage.clickForgotPasswordButton(forgotPasswordHeading);
+ loginPage.fillUserNameInForgotPasswordForm("invalid@email");
+ cy.get('button[type="submit"]').should('be.disabled');
+ cy.get('input[name="username"]').should('have.class', 'error');
+
+ // Test Case 2: Non-existent user
+ loginPage.fillUserNameInForgotPasswordForm("[email protected]");
+ loginPage.interceptResetLinkReq();
+ loginPage.clickSendResetLinkBtn();
+ loginPage.verifyResetLinkReq();
+ cy.verifyNotification("User not found");
+ cy.closeNotification();
+
+ // Test Case 3: Successful password reset
loginPage.clickForgotPasswordButton(forgotPasswordHeading);
loginPage.verifyForgotPasswordHeading([forgotPasswordHeading]);
loginPage.fillUserNameInForgotPasswordForm(userName);
loginPage.interceptResetLinkReq();
loginPage.clickSendResetLinkBtn();
loginPage.verifyResetLinkReq();
cy.verifyNotification("Password Reset Email Sent");
cy.closeNotification();
loginPage.clickBackButton();
loginPage.verifyLoginPageUrl();
loginPage.verifyLoginButtonPresence();
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("Reset user's password using email", () => { | |
| beforeEach(() => { | |
| cy.awaitUrl("/", true); | |
| }); | |
| it("Send a password reset link and navigate back to the login page", () => { | |
| loginPage.clickForgotPasswordButton(forgotPasswordHeading); | |
| loginPage.verifyForgotPasswordHeading([forgotPasswordHeading]); | |
| loginPage.fillUserNameInForgotPasswordForm(userName); | |
| loginPage.interceptResetLinkReq(); | |
| loginPage.clickSendResetLinkBtn(); | |
| loginPage.verifyResetLinkReq(); | |
| cy.verifyNotification("Password Reset Email Sent"); | |
| cy.closeNotification(); | |
| loginPage.clickBackButton(); | |
| loginPage.verifyLoginPageUrl(); | |
| loginPage.verifyLoginButtonPresence(); | |
| }); | |
| }); | |
| describe("Reset user's password using email", () => { | |
| const forgotPasswordHeading = "Forgot password?"; | |
| beforeEach(() => { | |
| cy.awaitUrl("/", true); | |
| cy.fixture('auth.json').as('authData'); | |
| }); | |
| it("should handle password reset workflow with various scenarios", () => { | |
| // Test Case 1: Invalid email format | |
| loginPage.clickForgotPasswordButton(forgotPasswordHeading); | |
| loginPage.fillUserNameInForgotPasswordForm("invalid@email"); | |
| cy.get('button[type="submit"]').should('be.disabled'); | |
| cy.get('input[name="username"]').should('have.class', 'error'); | |
| // Test Case 2: Non-existent user | |
| loginPage.fillUserNameInForgotPasswordForm("[email protected]"); | |
| loginPage.interceptResetLinkReq(); | |
| loginPage.clickSendResetLinkBtn(); | |
| loginPage.verifyResetLinkReq(); | |
| cy.verifyNotification("User not found"); | |
| cy.closeNotification(); | |
| // Test Case 3: Successful password reset | |
| loginPage.clickForgotPasswordButton(forgotPasswordHeading); | |
| loginPage.verifyForgotPasswordHeading([forgotPasswordHeading]); | |
| loginPage.fillUserNameInForgotPasswordForm(userName); | |
| loginPage.interceptResetLinkReq(); | |
| loginPage.clickSendResetLinkBtn(); | |
| loginPage.verifyResetLinkReq(); | |
| cy.verifyNotification("Password Reset Email Sent"); | |
| cy.closeNotification(); | |
| loginPage.clickBackButton(); | |
| loginPage.verifyLoginPageUrl(); | |
| loginPage.verifyLoginButtonPresence(); | |
| }); | |
| }); |
|
@JavidSumra Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
auth_spectohomepage_specandauth.cy.tstoUserLogin.cy.ts.auth.cy.tsandforgotpassword.cy.tsintoUserLogin.cy.ts, organizing them under separatedescribeblocks.LoginPage.tsfor login credentials.cy.interceptbefore accessing#user-profile-nameto ensure page load.UserProfilePage.tsandcommands.ts.cy.interceptfor stability.Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores