test: Admin setting new test cases with email #38522
Conversation
WalkthroughThis pull request enhances Cypress end-to-end testing for email settings in the application. It introduces a suite of tests for admin email configurations, including SMTP setup, user invitations, and password reset functionality. The modifications span multiple files, adding new locators for SMTP inputs, implementing asynchronous email verification methods, and updating header validation to improve test robustness and flexibility. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
🪧 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
Documentation and Community
|
github-actions
bot
added
skip-changelog
![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 (2)
app/client/cypress/locators/SignupPage.json (2)
8-10: LGTM on JSON structure, but locator needs improvementThe new locator should use data-* attributes instead of href for better maintainability.
- "forgetPasswordLink": "[href='/user/forgotPassword']" + "forgetPasswordLink": "[data-testid='t--forgot-password-link']"
Line range hint
1-10: Consider refactoring all selectors to use data- attributes*Several selectors in this file use attribute selectors or CSS classes which are fragile and against best practices.
{ - "username": "input[name='email']", - "password": "input[name='password']", - "submitBtn": "button[type='submit']", + "username": "[data-testid='t--email-input']", + "password": "[data-testid='t--password-input']", + "submitBtn": "[data-testid='t--submit-button']", "proficiencyGroupButton": "[data-testid='t--user-proficiency'] button", "useCaseGroupButton": "[data-testid='t--user-use-case'] button", - "dropdownOption": ".rc-select-item-option:first", - "getStartedSubmit": ".t--get-started-button", - "forgetPasswordLink": "[href='/user/forgotPassword']" + "dropdownOption": "[data-testid='t--dropdown-option']", + "getStartedSubmit": "[data-testid='t--get-started-button']", + "forgetPasswordLink": "[data-testid='t--forgot-password-link']" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts(1 hunks)app/client/cypress/locators/AdminsSettings.js(1 hunks)app/client/cypress/locators/SignupPage.json(1 hunks)app/client/cypress/support/Pages/AggregateHelper.ts(1 hunks)app/client/cypress/support/Pages/HomePage.ts(4 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- app/client/cypress/locators/AdminsSettings.js
- app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
- app/client/cypress/support/Pages/HomePage.ts
- app/client/cypress/support/Pages/AggregateHelper.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/client/cypress/locators/SignupPage.json (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/HomePage.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/AggregateHelper.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/locators/AdminsSettings.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
|
/ci-test-limit-count run_count=10 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12742947193. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
266-412: Extract duplicated email parsing logic.The email HTML parsing logic is duplicated. Consider extracting it into a helper function.
function extractInviteLink(emailHtml: string): string { const bodyMatch = emailHtml.match(/<body[^>]*>([\s\S]*?)<\/body>/i); if (!bodyMatch?.[1]) throw new Error("Email body not found"); const inviteLinkMatch = bodyMatch[1].match(/href="https?:\/\/[^"]*"/); if (!inviteLinkMatch) throw new Error("Invite link not found"); return inviteLinkMatch[0] .replace(/([^:]\/)\/+/g, "$1") .replace(/href=|=|"|"/g, ""); }🧰 Tools
🪛 Biome (1.9.4)
[error] 308-309: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 382-383: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/cypress/support/Pages/AggregateHelper.ts (1)
2002-2146: LGTM! Well-implemented email polling mechanism.Strong implementation with:
- Proper TypeScript types
- Comprehensive error handling
- Helpful debug logging
Minor suggestion: Consider extracting email matching logic into a separate function for better maintainability.
function findMatchingEmail( emails: Array<{ headers: { subject: string; date: string; to: string[] }; text: string }>, targetSubject: string, targetEmail?: string ) { return emails.filter((email) => { const subjectMatches = email.headers.subject .trim() .toLowerCase() .includes(targetSubject.trim().toLowerCase()); if (!targetEmail) return subjectMatches; const toAddresses = email.headers.to.split(" "); return subjectMatches && toAddresses.some( (to) => to.toLowerCase() === targetEmail.toLowerCase() ); }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts(1 hunks)app/client/cypress/support/Pages/AggregateHelper.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/AggregateHelper.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
[error] 308-309: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 382-383: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
🔇 Additional comments (4)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (4)
1-41: LGTM! Test setup follows best practices.Good use of:
- API calls for user setup instead of UI interactions
- Random email generation for test isolation
- Proper separation of concerns with helper imports
63-136: LGTM! Well-structured SMTP test with proper validations.Good implementation of:
- Input value assertions using invoke and expect
- Email verification using the new waitForEmail helper
- Proper test cleanup
138-210: LGTM! Comprehensive password reset test.Good implementation of:
- API interception with proper response validation
- Email content verification
- Reset link validation
212-264: LGTM! Well-implemented workspace invitation test.Good implementation of:
- Email verification for workspace invites
- Permission validation for invited users
- Proper cleanup after test
| it("1. Verify adding new admin user and sign up process", () => { | ||
| cy.LoginFromAPI(Cypress.env("USERNAME"), Cypress.env("PASSWORD")); | ||
| adminSettings.NavigateToAdminSettings(); | ||
| agHelper.AssertElementVisibility(AdminsSettings.LeftPaneBrandingLink); | ||
| cy.get(AdminsSettings.addEmailGhostInput) | ||
| .click({ force: true }) | ||
| .type(fromEmail); | ||
| agHelper.ClickOutside(); | ||
|
|
||
| agHelper.GetNClick(AdminsSettings.saveButton, 0, true); | ||
| cy.waitForServerRestart(); | ||
| agHelper.AssertContains( | ||
| fromEmail, | ||
| "exist", | ||
| formWidgetsPage.dropdownInput, | ||
| ); | ||
| cy.SignupFromAPI(fromEmail, "testPassword"); | ||
| cy.LoginFromAPI(fromEmail, "testPassword"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace cy.wait with proper assertions.
The test uses cy.wait for server restart which violates the coding guidelines. Consider using cy.waitUntil with proper server status checks.
-cy.waitForServerRestart();
+cy.waitUntil(() => {
+ return cy.request({
+ url: '/api/v1/health',
+ failOnStatusCode: false,
+ }).then((response) => response.status === 200);
+});📝 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("1. Verify adding new admin user and sign up process", () => { | |
| cy.LoginFromAPI(Cypress.env("USERNAME"), Cypress.env("PASSWORD")); | |
| adminSettings.NavigateToAdminSettings(); | |
| agHelper.AssertElementVisibility(AdminsSettings.LeftPaneBrandingLink); | |
| cy.get(AdminsSettings.addEmailGhostInput) | |
| .click({ force: true }) | |
| .type(fromEmail); | |
| agHelper.ClickOutside(); | |
| agHelper.GetNClick(AdminsSettings.saveButton, 0, true); | |
| cy.waitForServerRestart(); | |
| agHelper.AssertContains( | |
| fromEmail, | |
| "exist", | |
| formWidgetsPage.dropdownInput, | |
| ); | |
| cy.SignupFromAPI(fromEmail, "testPassword"); | |
| cy.LoginFromAPI(fromEmail, "testPassword"); | |
| }); | |
| it("1. Verify adding new admin user and sign up process", () => { | |
| cy.LoginFromAPI(Cypress.env("USERNAME"), Cypress.env("PASSWORD")); | |
| adminSettings.NavigateToAdminSettings(); | |
| agHelper.AssertElementVisibility(AdminsSettings.LeftPaneBrandingLink); | |
| cy.get(AdminsSettings.addEmailGhostInput) | |
| .click({ force: true }) | |
| .type(fromEmail); | |
| agHelper.ClickOutside(); | |
| agHelper.GetNClick(AdminsSettings.saveButton, 0, true); | |
| cy.waitUntil(() => { | |
| return cy.request({ | |
| url: '/api/v1/health', | |
| failOnStatusCode: false, | |
| }).then((response) => response.status === 200); | |
| }); | |
| agHelper.AssertContains( | |
| fromEmail, | |
| "exist", | |
| formWidgetsPage.dropdownInput, | |
| ); | |
| cy.SignupFromAPI(fromEmail, "testPassword"); | |
| cy.LoginFromAPI(fromEmail, "testPassword"); | |
| }); |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12742947193. |
sagar-qa007
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (6)
200-200: Remove console.log statements.Remove debug console.log statements from the test file as they add noise to the test output.
-console.log("Reset Password Link:", resetPasswordLink); -console.log("Email:", email); -console.log("bodyMatch: ", bodyMatch); -console.log("inviteLinkMatch: ", inviteLinkMatch); -console.log("Invite workspace Link:", inviteLink);Also applies to: 302-302, 309-309, 316-316, 323-323, 375-375, 383-383, 390-390, 397-397
201-201: Replace hardcoded timeout with constant.Use the existing
TIMEOUTconstant instead of hardcoded timeout values for consistency.-cy.visit(resetPasswordLink, { timeout: 60000 }); +cy.visit(resetPasswordLink, { timeout: TIMEOUT });Also applies to: 254-254, 324-324, 399-399
310-311: Use optional chaining for safer property access.Replace the conditional check with optional chaining for cleaner code.
-if (bodyMatch && bodyMatch[1]) { - const bodyContent = bodyMatch[1]; +const bodyContent = bodyMatch?.[1]; +if (bodyContent) {Also applies to: 384-385
🧰 Tools
🪛 Biome (1.9.4)
[error] 310-311: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
141-144: Simplify redundant conditional.The conditional statement assigns the same value in both cases, making it redundant.
-const resetPassSubject: string = - CURRENT_REPO === REPO.EE - ? "Reset your Appsmith password" - : "Reset your Appsmith password"; +const resetPassSubject: string = "Reset your Appsmith password";
19-19: Use a helper function for generating test emails.Extract email generation logic into a helper function to reduce code duplication.
function generateTestEmail(prefix: string): string { return `${prefix}.${Math.random().toString(36).substring(2, 10)}@appsmith.com`; }Then use it like:
-const fromEmail = `sagar.${Math.random().toString(36).substring(2, 10)}@appsmith.com`; -const emailOne = `sagarspec1.${Math.random().toString(36).substring(2, 10)}@appsmith.com`; +const fromEmail = generateTestEmail('sagar'); +const emailOne = generateTestEmail('sagarspec1');Also applies to: 25-28
31-41: Use beforeAll instead of before for test setup.Since the setup is creating test users that are used across all test cases, use
beforeAllto ensure the setup runs only once for the entire test suite.-before(() => { +beforeAll(() => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
[error] 310-311: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 384-385: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (2)
52-54: Replace cy.waitForServerRestart with proper assertions.The test uses
cy.waitForServerRestartandcy.waitwhich violates the coding guidelines. Consider usingcy.waitUntilwith proper server status checks.-agHelper.GetNClick(AdminsSettings.saveButton, 0, true); -cy.waitForServerRestart(); -cy.waitUntil(() => cy.get(homePage._profileMenu).should("be.visible")); +agHelper.GetNClick(AdminsSettings.saveButton, 0, true); +cy.waitUntil(() => { + return cy.request({ + url: '/api/v1/health', + failOnStatusCode: false, + }).then((response) => response.status === 200); +}); +cy.get(homePage._profileMenu).should("be.visible");
135-137: Replace cy.waitForServerRestart with proper assertions.Similar to the previous occurrence, replace the server restart wait with proper health check.
-agHelper.GetNClick(AdminsSettings.saveButton, 0, true); -cy.waitForServerRestart(); -cy.waitUntil(() => cy.get(homePage._profileMenu).should("be.visible")); +agHelper.GetNClick(AdminsSettings.saveButton, 0, true); +cy.waitUntil(() => { + return cy.request({ + url: '/api/v1/health', + failOnStatusCode: false, + }).then((response) => response.status === 200); +}); +cy.get(homePage._profileMenu).should("be.visible");
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (3)
31-41: Consider optimizing the setup by using Promise.all for parallel user creation.The sequential signup operations can be optimized for faster test execution.
before(() => { - cy.LogOut(); - cy.SignupFromAPI(emailOne, tempPassword); - cy.LogOut(); - cy.SignupFromAPI(emailTwo, tempPassword); - cy.LogOut(); - cy.SignupFromAPI(emailThree, tempPassword); - cy.LogOut(); - cy.SignupFromAPI(emailFour, tempPassword); - cy.LogOut(); + const users = [emailOne, emailTwo, emailThree, emailFour]; + return Promise.all( + users.map(email => cy.SignupFromAPI(email, tempPassword)) + ).then(() => cy.LogOut()); });
73-119: Refactor repetitive SMTP input validations into a helper function.The current implementation has duplicate code for input validation. Consider creating a helper function.
+const validateSmtpInput = (selector: string, value: string) => { + cy.get(selector) + .clear() + .type(value) + .invoke('val') + .then((text) => { + expect(text).to.equal(value); + }); +}; -cy.get(AdminsSettings.smtpAppsmithMailHostInput) - .clear() - .type("host.docker.internal") - .invoke("val") - .then((text) => { - expect(text).to.equal("host.docker.internal"); - }); +validateSmtpInput(AdminsSettings.smtpAppsmithMailHostInput, "host.docker.internal");
197-197: Remove console.log statements from test cases.Debug statements should not be committed to the codebase.
-console.log("Reset Password data:", resetPasswordLinkMatch); -console.log("Reset Password Link:", resetPasswordLink); -console.log("Invite workspace Link:", inviteLink); -console.log("Email:", email); -console.log("bodyMatch: ", bodyMatch); -console.log("inviteLinkMatch: ", inviteLinkMatch); -console.log("Invite workspace Link:", inviteLink); -console.log("bodyMatch: ", bodyMatch);Also applies to: 202-202, 256-256, 305-305, 313-313, 320-320, 327-327, 387-387
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
[error] 140-140: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 314-315: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 388-389: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
53-53: Replace cy.waitForServerRestart with proper assertions.
| const emailHtml = email.html; // Store the email HTML content | ||
| const inviteLinkMatch = emailHtml.match( | ||
| /href="([^"]*applications[^"]*)"/, | ||
| ); // Extract the link using regex | ||
|
|
||
| if (inviteLinkMatch) { | ||
| const inviteLink = inviteLinkMatch[1].replace(/([^:]\/)\/+/g, "$1"); | ||
| console.log("Invite workspace Link:", inviteLink); | ||
| cy.visit(inviteLink, { timeout: 60000 }); | ||
| homePage.SelectWorkspace(workspaceName); | ||
| agHelper.AssertContains(workspaceName); | ||
| cy.get(homePageLocators.applicationCard) | ||
| .first() | ||
| .trigger("mouseover"); | ||
| agHelper.AssertElementExist(homePageLocators.appEditIcon); | ||
| } else { | ||
| throw new Error( | ||
| "Invite workspace app link not found in the email HTML", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicate email verification logic into a helper function.
The HTML parsing and link extraction logic is repeated across multiple test cases.
+const extractInviteLink = (emailHtml: string): string => {
+ const bodyMatch = emailHtml.match(/<body[^>]*>([\s\S]*?)<\/body>/i);
+ if (!bodyMatch?.[1]) throw new Error("Email body not found");
+
+ const inviteLinkMatch = bodyMatch[1].match(/href="https?:\/\/[^"]*"/);
+ if (!inviteLinkMatch) throw new Error("Invite link not found in the email HTML");
+
+ return inviteLinkMatch[0]
+ .replace(/([^:]\/)\/+/g, "$1")
+ .replace(/href=|=|"|"/g, "");
+};
-const emailHtml = email.html;
-const bodyMatch = emailHtml.match(/<body[^>]*>([\s\S]*?)<\/body>/i);
-if (bodyMatch && bodyMatch[1]) {
- const bodyContent = bodyMatch[1];
- const inviteLinkMatch = bodyContent.match(/href="https?:\/\/[^"]*"/);
- if (inviteLinkMatch) {
- const inviteLink = inviteLinkMatch[0]
- .replace(/([^:]\/)\/+/g, "$1")
- .replace(/href=|=|"|"/g, "");
+const inviteLink = extractInviteLink(email.html);Also applies to: 309-340, 383-414
| cy.waitUntil(() => cy.get(homePage._profileMenu).should("be.visible")); | ||
| }); | ||
|
|
||
| it.only("3. To verify forget password email", () => { |
There was a problem hiding this comment.
Remove .only to ensure all tests are executed.
Using it.only prevents other tests from running and should not be committed.
-it.only("3. To verify forget password email", () => {
+it("3. To verify forget password email", () => {📝 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.only("3. To verify forget password email", () => { | |
| it("3. To verify forget password email", () => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (3)
20-21: Consider moving timeout values to constants file.These values could be reused across different test files. Consider moving them to a shared constants file.
- const POLL_INTERVAL = 5000; - const TIMEOUT = 80000; + import { EMAIL_POLL_INTERVAL, EMAIL_TIMEOUT } from "../../../../constants";
73-119: Refactor duplicate expect statements into a helper function.Multiple input validations follow the same pattern. Consider extracting this into a helper function.
+const validateInputValue = (selector: string, value: string) => { + cy.get(selector) + .clear() + .type(value) + .invoke("val") + .then((text) => { + expect(text).to.equal(value); + }); +}; -cy.get(AdminsSettings.smtpAppsmithMailHostInput) - .clear() - .type("host.docker.internal") - .invoke("val") - .then((text) => { - expect(text).to.equal("host.docker.internal"); - }); +validateInputValue(AdminsSettings.smtpAppsmithMailHostInput, "host.docker.internal");
323-324: Use optional chaining for safer property access.Replace the conditional checks with optional chaining for better readability and safety.
-if (bodyMatch && bodyMatch[1]) { +if (bodyMatch?.at(1)) {Also applies to: 397-398
🧰 Tools
🪛 Biome (1.9.4)
[error] 323-324: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
[error] 323-324: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 397-398: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (2)
53-53: Replace cy.wait with proper assertions.The test uses
cy.waitForServerRestartwhich violates the coding guidelines.
193-212: Extract duplicate email verification logic into a helper function.The HTML parsing and link extraction logic is repeated across multiple test cases.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (2)
19-21: Consider extracting magic numbers into named constants.The poll interval and timeout values should be moved to a configuration file or constants file for better maintainability.
-const POLL_INTERVAL = 5000; -const TIMEOUT = 100000; +import { EMAIL_CONFIG } from "../../../../constants"; +const { POLL_INTERVAL, TIMEOUT } = EMAIL_CONFIG;
264-264: Extract timeout value into a constant.The timeout value of 60000 is hardcoded in multiple places. Consider extracting it into a named constant.
+const PAGE_LOAD_TIMEOUT = 60000; -cy.visit(inviteLink, { timeout: 60000 }); +cy.visit(inviteLink, { timeout: PAGE_LOAD_TIMEOUT });Also applies to: 429-429
app/client/cypress/support/Pages/AggregateHelper.ts (1)
2003-2146: Simplify email matching logic and improve type safety.The email matching logic can be simplified and made more type-safe. Consider these improvements:
- Add TypeScript interfaces for email structure:
interface EmailHeaders { subject: string; date: string; to: string[]; } interface Email { headers: EmailHeaders; text: string; html: string; }
- Extract email matching logic into a separate function:
function isMatchingEmail(email: Email, targetSubject: string, targetEmail?: string): boolean { const subjectMatches = email.headers.subject .trim() .toLowerCase() .includes(targetSubject.trim().toLowerCase()); if (!targetEmail) return subjectMatches; const toAddresses = email.headers.to.split(" "); return subjectMatches && toAddresses.some( (to) => to.toLowerCase() === targetEmail.toLowerCase() ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts(1 hunks)app/client/cypress/support/Pages/AggregateHelper.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/support/Pages/AggregateHelper.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
[error] 329-330: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 414-415: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (4)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts (4)
53-55: Replace cy.waitForServerRestart with proper assertions.Using cy.waitForServerRestart violates the coding guidelines. Consider using cy.waitUntil with proper server status checks.
-cy.waitForServerRestart().then(() => { - agHelper.WaitUntilEleAppear(homePage._profileMenu); -}); +cy.waitUntil(() => { + return cy.request({ + url: '/api/v1/health', + failOnStatusCode: false, + }).then((response) => response.status === 200); +}).then(() => { + agHelper.WaitUntilEleAppear(homePage._profileMenu); +});
138-139: Replace cy.waitForServerRestart with proper assertions.Using cy.waitForServerRestart violates the coding guidelines. Consider using cy.waitUntil with proper server status checks.
193-212: Extract email parsing logic into a helper function.The email HTML parsing and link extraction logic is repeated across test cases. Consider extracting it into a reusable helper function.
409-440: Extract email parsing logic into a helper function.The email HTML parsing and link extraction logic is repeated. Consider extracting it into a reusable helper function.
🧰 Tools
🪛 Biome (1.9.4)
[error] 414-415: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| cy.SignupFromAPI(fromEmail, "testPassword"); | ||
| cy.LoginFromAPI(fromEmail, "testPassword"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions for successful sign-up.
The test should verify that the sign-up process completed successfully by adding appropriate assertions.
cy.SignupFromAPI(fromEmail, "testPassword");
cy.LoginFromAPI(fromEmail, "testPassword");
+agHelper.AssertElementVisibility(homePage._profileMenu);
+agHelper.AssertElementVisibility(homePage._displayUsername);📝 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.
| cy.SignupFromAPI(fromEmail, "testPassword"); | |
| cy.LoginFromAPI(fromEmail, "testPassword"); | |
| cy.SignupFromAPI(fromEmail, "testPassword"); | |
| cy.LoginFromAPI(fromEmail, "testPassword"); | |
| agHelper.AssertElementVisibility(homePage._profileMenu); | |
| agHelper.AssertElementVisibility(homePage._displayUsername); |
| cy.get(AdminsSettings.smtpAppsmithMailHostInput) | ||
| .clear() | ||
| .type("host.docker.internal") | ||
| .invoke("val") | ||
| .then((text) => { | ||
| expect(text).to.equal("host.docker.internal"); | ||
| }); | ||
|
|
||
| cy.get(AdminsSettings.smtpAppsmithMailPortInput) | ||
| .clear() | ||
| .type("25") | ||
| .invoke("val") | ||
| .then((text) => { | ||
| expect(text).to.equal("25"); | ||
| }); | ||
|
|
||
| cy.get(AdminsSettings.smtpAppsmithMailFromInput) | ||
| .clear() | ||
| .type(fromEmail) | ||
| .invoke("val") | ||
| .then((text) => { | ||
| expect(text).to.equal(fromEmail); | ||
| }); | ||
|
|
||
| cy.get(AdminsSettings.smtpAppsmithMailReplyToInput) | ||
| .clear() | ||
| .type(fromEmail) | ||
| .invoke("val") | ||
| .then((text) => { | ||
| expect(text).to.equal(fromEmail); | ||
| }); | ||
|
|
||
| cy.get(AdminsSettings.smtpAppsmithMailUserNameInput) | ||
| .clear() | ||
| .type("root") | ||
| .invoke("val") | ||
| .then((text) => { | ||
| expect(text).to.equal("root"); | ||
| }); | ||
|
|
||
| cy.get(AdminsSettings.smtpAppsmithMailPasswordInput) | ||
| .clear() | ||
| .type("root") | ||
| .invoke("val") | ||
| .then((text) => { | ||
| expect(text).to.equal("root"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicate input validation logic into a helper function.
The input validation logic is repeated for multiple SMTP fields. Consider extracting it into a reusable helper function.
+const validateSMTPInput = (selector: string, value: string) => {
+ cy.get(selector)
+ .clear()
+ .type(value)
+ .invoke("val")
+ .then((text) => {
+ expect(text).to.equal(value);
+ });
+};
-cy.get(AdminsSettings.smtpAppsmithMailHostInput)
- .clear()
- .type("host.docker.internal")
- .invoke("val")
- .then((text) => {
- expect(text).to.equal("host.docker.internal");
- });
+validateSMTPInput(AdminsSettings.smtpAppsmithMailHostInput, "host.docker.internal");📝 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.
| cy.get(AdminsSettings.smtpAppsmithMailHostInput) | |
| .clear() | |
| .type("host.docker.internal") | |
| .invoke("val") | |
| .then((text) => { | |
| expect(text).to.equal("host.docker.internal"); | |
| }); | |
| cy.get(AdminsSettings.smtpAppsmithMailPortInput) | |
| .clear() | |
| .type("25") | |
| .invoke("val") | |
| .then((text) => { | |
| expect(text).to.equal("25"); | |
| }); | |
| cy.get(AdminsSettings.smtpAppsmithMailFromInput) | |
| .clear() | |
| .type(fromEmail) | |
| .invoke("val") | |
| .then((text) => { | |
| expect(text).to.equal(fromEmail); | |
| }); | |
| cy.get(AdminsSettings.smtpAppsmithMailReplyToInput) | |
| .clear() | |
| .type(fromEmail) | |
| .invoke("val") | |
| .then((text) => { | |
| expect(text).to.equal(fromEmail); | |
| }); | |
| cy.get(AdminsSettings.smtpAppsmithMailUserNameInput) | |
| .clear() | |
| .type("root") | |
| .invoke("val") | |
| .then((text) => { | |
| expect(text).to.equal("root"); | |
| }); | |
| cy.get(AdminsSettings.smtpAppsmithMailPasswordInput) | |
| .clear() | |
| .type("root") | |
| .invoke("val") | |
| .then((text) => { | |
| expect(text).to.equal("root"); | |
| }); | |
| const validateSMTPInput = (selector: string, value: string) => { | |
| cy.get(selector) | |
| .clear() | |
| .type(value) | |
| .invoke("val") | |
| .then((text) => { | |
| expect(text).to.equal(value); | |
| }); | |
| }; | |
| validateSMTPInput(AdminsSettings.smtpAppsmithMailHostInput, "host.docker.internal"); | |
| validateSMTPInput(AdminsSettings.smtpAppsmithMailPortInput, "25"); | |
| validateSMTPInput(AdminsSettings.smtpAppsmithMailFromInput, fromEmail); | |
| validateSMTPInput(AdminsSettings.smtpAppsmithMailReplyToInput, fromEmail); | |
| validateSMTPInput(AdminsSettings.smtpAppsmithMailUserNameInput, "root"); | |
| validateSMTPInput(AdminsSettings.smtpAppsmithMailPasswordInput, "root"); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
app/client/cypress/support/Pages/AggregateHelper.ts (5)
2014-2017: Consider using more descriptive variable names.Variables like
latestEmailDateandlatestEmailare used for tracking state during polling, but their purpose could be clearer.-let latestEmailDate: Date | null = null; -let latestEmail: any = null; +let mostRecentEmailDate: Date | null = null; +let mostRecentMatchingEmail: EmailResponse | null = null;
2018-2020: Add error handling for invalid date strings.The
parseDatefunction should handle invalid date strings gracefully.function parseDate(dateString: string): Date { + const parsedDate = new Date(dateString.replace(/ \([A-Za-z\s]*\)$/, "")); + if (isNaN(parsedDate.getTime())) { + throw new Error(`Invalid date string: ${dateString}`); + } - return new Date(dateString.replace(/ \([A-Za-z\s]*\)$/, "")); + return parsedDate; }
2035-2039: Remove or use proper logging mechanism.Console.log statements should be replaced with proper logging or removed in production code.
- console.log( - "Fetched email subjects:", - emails.map((email) => email.headers.subject), - ); + cy.log(`Fetched ${emails.length} emails: ${emails.map(email => email.headers.subject).join(', ')}`);
2064-2075: Simplify email address matching logic.The email address comparison can be simplified and made more robust.
- const toAddresses = email.headers.to.split(" "); - return toAddresses.some( - (to) => to.toLowerCase() === targetEmail.toLowerCase(), - ); + const normalizedTargetEmail = targetEmail.toLowerCase().trim(); + const toAddresses = email.headers.to.toLowerCase().split(/[\s,]+/); + return toAddresses.some(to => to.trim() === normalizedTargetEmail);
2126-2130: Improve timeout error message.The error message could provide more context about the polling attempt.
- console.error("No matching email found within the timeout period."); + const timeoutSeconds = Math.round((Date.now() - startTime) / 1000); + console.error( + `No matching email found after polling for ${timeoutSeconds} seconds. ` + + `Found ${emails.length} other emails during this period.` + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/support/Pages/AggregateHelper.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/AggregateHelper.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/cypress/support/Pages/AggregateHelper.ts (1)
2003-2013: Method signature follows TypeScript best practices.The parameter interface is well-structured with clear types and optional parameters.
| .request("http://localhost:5001/api/v1/maildev-emails") | ||
| .then((res) => { | ||
| if (res.status !== 200) { | ||
| throw new Error(`Request failed with status ${res.status}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add retry mechanism for failed HTTP requests.
The email fetching could fail due to temporary network issues. Consider adding retry logic.
- return cy
- .request("http://localhost:5001/api/v1/maildev-emails")
+ return cy
+ .request({
+ url: "http://localhost:5001/api/v1/maildev-emails",
+ retryOnStatusCodeFailure: true,
+ retryOnNetworkFailure: true,
+ })📝 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.
| .request("http://localhost:5001/api/v1/maildev-emails") | |
| .then((res) => { | |
| if (res.status !== 200) { | |
| throw new Error(`Request failed with status ${res.status}`); | |
| } | |
| .request({ | |
| url: "http://localhost:5001/api/v1/maildev-emails", | |
| retryOnStatusCodeFailure: true, | |
| retryOnNetworkFailure: true, | |
| }) | |
| .then((res) => { | |
| if (res.status !== 200) { | |
| throw new Error(`Request failed with status ${res.status}`); | |
| } |
| return fetchEmail().then((email) => { | ||
| if (!email) { | ||
| throw new Error( | ||
| `Timeout: No email with subject "${targetSubject}" found${ | ||
| targetEmail ? ` for recipient "${targetEmail}"` : "" | ||
| }.`, | ||
| ); | ||
| } | ||
| return email; | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add content validation to match existing patterns
The codebase shows a consistent pattern of validating both email metadata and content. Consider adding content validation to match this pattern, as seen in Email_settings_Spec.ts:
- Validate email text/html content
- Add content assertions similar to
expect(email.text.trim()).to.equal(expectedContent)
🔗 Analysis chain
Consider adding email content validation.
The method returns the email without validating its content structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for email content validation in other test files
rg -A 5 "waitForEmail.*then.*email\." app/client/cypress/
Length of output: 59
Script:
#!/bin/bash
# Find email-related files
fd -e ts -e js "email" app/client/cypress/
# Look for email object structure and validation patterns
rg -i "interface.*email|type.*email|email.*=.*{" app/client/cypress/
# Search for email assertions in tests
rg -i "expect.*email|assert.*email" app/client/cypress/
Length of output: 65978
Script:
#!/bin/bash
# Find email object structure and validation
ast-grep --pattern 'interface $_ {
$$$
email
$$$
}'
# Look for email validation patterns
rg -A 3 "validateEmail|checkEmail|isValidEmail" app/client/cypress/
# Find test assertions for email
rg -A 3 "expect.*email|assert.*email" "app/client/cypress/e2e"
Length of output: 12564
sagar-qa007
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/client/cypress/support/Pages/AggregateHelper.ts (3)
2017-2019: Improve date parsing robustness.The current date parsing implementation might fail for different date formats. Consider using a date parsing library like
date-fnsfor more robust parsing.function parseDate(dateString: string): Date { - return new Date(dateString.replace(/ \([A-Za-z\s]*\)$/, "")); + try { + return new Date(dateString.replace(/ \([A-Za-z\s]*\)$/, "")); + } catch (error) { + throw new Error(`Invalid date format: ${dateString}`); + } }
2029-2033: Define a proper email interface.Extract the email type definition into a separate interface for better code organization and reusability.
+interface EmailResponse { + headers: { + subject: string; + date: string; + to: string; + }; + text: string; +} - const emails: Array<{ - headers: { subject: string; date: string; to: string }; - text: string; - }> = res.body; + const emails: Array<EmailResponse> = res.body;
2034-2048: Optimize email filtering logic.The current implementation creates unnecessary string allocations. Consider optimizing the string comparisons.
const matchingEmails = emails.filter((email) => { + const normalizedSubject = targetSubject.trim().toLowerCase(); + const normalizedEmailTo = targetEmail?.trim().toLowerCase(); const subjectMatch = email.headers.subject .trim() .toLowerCase() - .includes(targetSubject.trim().toLowerCase()); + .includes(normalizedSubject); if (targetEmail) { const emailTo = email.headers.to.trim().toLowerCase(); - return subjectMatch && emailTo === targetEmail.trim().toLowerCase(); + return subjectMatch && emailTo === normalizedEmailTo; } return subjectMatch; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/support/Pages/AggregateHelper.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/AggregateHelper.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
app/client/cypress/support/Pages/AggregateHelper.ts (3)
2003-2013: Method signature follows TypeScript best practices.The parameter destructuring and type definitions are well-structured, making the API clear and type-safe.
2021-2070: Add retry mechanism for failed HTTP requests.The email fetching could fail due to temporary network issues. Consider adding retry logic.
2072-2082: Add content validation to match existing patterns.The codebase shows a consistent pattern of validating both email metadata and content. Consider adding content validation to match this pattern.
| const endTime = Date.now() + timeout; // Calculate the end time based on timeout duration | ||
| let latestEmail: any = null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation.
Add validation for the input parameters to ensure timeout and pollInterval are positive numbers, and targetSubject is not empty.
+ if (timeout <= 0 || pollInterval <= 0) {
+ throw new Error('timeout and pollInterval must be positive numbers');
+ }
+ if (!targetSubject.trim()) {
+ throw new Error('targetSubject cannot be empty');
+ }
const endTime = Date.now() + timeout;
let latestEmail: any = null;Committable suggestion skipped: line range outside the PR's diff.
Description
New cypress test case for admin setting
Fixes # https://app.zenhub.com/workspaces/stability-pod-6690c4814e31602e25cab7fd/issues/gh/appsmithorg/appsmith/38459
Automation
/ok-to-test tags="@tag.Settings"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12891872722
Commit: 9626ef8
Cypress dashboard.
Tags: @tag.Settings
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/AdminSettings/Email_settings_Spec.ts
List of identified flaky tests.Tue, 21 Jan 2025 17:24:59 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Tests
Locators
Improvements