Skip to content

fix: enable form login spec improvement #40693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

jacquesikot
Copy link
Contributor

@jacquesikot jacquesikot commented May 19, 2025

Description

  • Convert entire test case to typescript
  • Abstract repetitive code to functions
  • Remove all waits and use UI assertions
  • Better comments

Automation

/ok-to-test tags="@tag.Sanity"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15269749230
Commit: e0f22f9
Cypress dashboard.
Tags: @tag.Sanity
Spec:


Tue, 27 May 2025 08:44:00 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Tests
    • Replaced existing Cypress tests with a new suite verifying toggling of form login, signup, and GitHub authentication, including UI validations and server restart handling.
  • New Features
    • Enhanced admin settings automation with navigation and controls for authentication options, including GitHub OAuth configuration and form login/signup toggles.
  • Chores
    • Added a new locator for the login form to support UI testing.

@jacquesikot jacquesikot requested a review from ankitakinger May 19, 2025 14:38
@jacquesikot jacquesikot self-assigned this May 19, 2025
Copy link
Contributor

coderabbitai bot commented May 19, 2025

Walkthrough

This change replaces the deleted JavaScript Cypress test suite with a new TypeScript version that tests toggling form login/signup and GitHub OAuth settings. It also extends the AdminSettings page object with new methods for navigation and form automation.

Changes

File(s) Change Summary
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js Deleted the JavaScript Cypress test suite for form login/signup enable-disable functionality.
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts Added a new TypeScript Cypress test suite with helper functions and two main tests for enabling/disabling form login/signup.
app/client/cypress/support/Pages/AdminSettings.ts Added new locator and methods for navigating to authentication settings and automating GitHub OAuth form fill/save actions.
app/client/cypress/locators/LoginPage.json Added new locator "loginForm": "form" for login form element identification.

Sequence Diagram(s)

sequenceDiagram
    participant Tester
    participant AdminSettingsPage
    participant App

    Tester->>AdminSettingsPage: Navigate to Authentication Settings
    AdminSettingsPage->>App: Open Authentication Tab

    Tester->>AdminSettingsPage: Toggle Form Signup/Login (enable/disable)
    AdminSettingsPage->>App: Update Auth Settings
    App-->>Tester: Show Restart Notice

    Tester->>App: Attempt Signup/Login
    App-->>Tester: Show Success/Error or Hide UI Elements

    Tester->>AdminSettingsPage: Fill and Save GitHub Auth Form
    AdminSettingsPage->>App: Save GitHub OAuth Settings
    App-->>Tester: Update Login Options
Loading

Suggested reviewers

  • albinAppsmith

Poem

Out with the old, in with the new,
TypeScript tests now guide us through.
AdminSettings gains some flair,
GitHub forms handled with care.
Login toggles, signup too,
Cypress scripts—robust and true!
🚀


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d51772b and e0f22f9.

📒 Files selected for processing (2)
  • app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/AdminSettings.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/cypress/support/Pages/AdminSettings.ts
🧰 Additional context used
📓 Path-based instructions (1)
`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. ...

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/FormLogin/EnableFormLogin_spec.ts
🧠 Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (8)
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34781
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_DuplicateLabel_spec.ts:14-16
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure that login is performed via API using `LoginFromAPI` to avoid UI flakiness.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34781
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_DuplicateLabel_spec.ts:14-16
Timestamp: 2024-07-12T07:45:03.955Z
Learning: In Cypress tests for the Appsmith project, ensure that login is performed via API using `LoginFromAPI` to avoid UI flakiness.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33
Timestamp: 2024-10-08T15:32:34.114Z
Learning: 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.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-07-16T06:44:55.118Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep`, and other related sleep functions in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#35412
File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99
Timestamp: 2024-10-08T15:32:39.374Z
Learning: In Cypress tests within the `app/client/cypress` directory, avoid using `agHelper.Sleep`, `this.Sleep`, and other related sleep functions to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable. Instead, use Cypress methods like `cy.get()`, `cy.contains()`, and `cy.intercept()` to wait for specific conditions.
Learnt from: ApekshaBhosale
PR: appsmithorg/appsmith#35412
File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99
Timestamp: 2024-08-06T05:39:47.929Z
Learning: In Cypress tests, avoid using `agHelper.Sleep` as it can lead to flaky tests and unnecessary delays. Instead, use Cypress methods like `cy.get()`, `cy.contains()`, and `cy.intercept()` to wait for specific conditions.
Learnt from: ApekshaBhosale
PR: appsmithorg/appsmith#35412
File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests, avoid using `agHelper.Sleep` as it can lead to flaky tests and unnecessary delays. Instead, use Cypress methods like `cy.get()`, `cy.contains()`, and `cy.intercept()` to wait for specific conditions.
Learnt from: brayn003
PR: appsmithorg/appsmith#36622
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts:41-45
Timestamp: 2024-10-11T09:34:24.680Z
Learning: In `app/client/cypress/**/**.*` Cypress test files, the cleanup guideline is to use `after` hooks for cleanup tasks.
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: perform-test / rts-build / build
  • GitHub Check: perform-test / client-build / client-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-prettier / prettier-check
  • GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (2)

38-40: Good use of API login method.

Proper implementation using cy.LoginFromAPI instead of UI-based login, which aligns with our coding guidelines for improved test reliability.


70-70: Consistent API login usage.

Good use of cy.LoginFromAPI for authentication in the cleanup section, maintaining consistency with best practices.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

Support

Need 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)

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

Other keywords and placeholders

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

Documentation and Community

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

@jacquesikot jacquesikot changed the title refactor: remove EnableFormLogin_spec.js and enhance AdminSettings wi… fix: enable form login spec improvement May 19, 2025
@github-actions github-actions bot added the Bug Something isn't working label May 19, 2025
@jacquesikot
Copy link
Contributor Author

/ci-test-limit-count run_count=1 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts

Copy link

@github-actions github-actions bot added Bug Something isn't working and removed Bug Something isn't working labels May 19, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
app/client/cypress/support/Pages/AdminSettings.ts (1)

12-12: Consider using data attribute selector instead of class selector.

While the .t--settings-category-authentication selector follows the project pattern, our guidelines recommend using data-* attributes for selectors rather than classes.

- public _authenticationTab = ".t--settings-category-authentication";
+ public _authenticationTab = "[data-cy=settings-category-authentication]";
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (2)

167-168: Consider more specific selectors for absence checks.

Instead of generic element type selectors, use more specific data-* attribute selectors.

- agHelper.AssertElementAbsence("form");
- agHelper.AssertElementAbsence(loginPage.signupLink);
+ agHelper.AssertElementAbsence(loginPage.loginForm);
+ agHelper.AssertElementAbsence(loginPage.signupLink);

Ensure loginPage.loginForm is defined with a data-* attribute in the LoginPage.json file.


187-189: Use data- attributes for element assertions.*

Replace generic element type selectors with specific data-* attribute selectors.

- agHelper.AssertElementAbsence(adminSettings.loginWithGithub);
- agHelper.AssertElementExist("form");
- agHelper.AssertElementExist(loginPage.signupLink);
+ agHelper.AssertElementAbsence(adminSettings.loginWithGithub);
+ agHelper.AssertElementExist(loginPage.loginForm);
+ agHelper.AssertElementExist(loginPage.signupLink);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c84ac3 and 8923eee.

📒 Files selected for processing (3)
  • app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js (0 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/AdminSettings.ts (3 hunks)
💤 Files with no reviewable changes (1)
  • app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js
🧰 Additional context used
📓 Path-based instructions (1)
`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. ...

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/FormLogin/EnableFormLogin_spec.ts
  • app/client/cypress/support/Pages/AdminSettings.ts
🧠 Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (2)
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34781
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_DuplicateLabel_spec.ts:14-16
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure that login is performed via API using `LoginFromAPI` to avoid UI flakiness.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34781
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_DuplicateLabel_spec.ts:14-16
Timestamp: 2024-07-12T07:45:03.955Z
Learning: In Cypress tests for the Appsmith project, ensure that login is performed via API using `LoginFromAPI` to avoid UI flakiness.
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • 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
  • GitHub Check: build
🔇 Additional comments (9)
app/client/cypress/support/Pages/AdminSettings.ts (3)

3-3: Good practice importing locators from a JSON file.

This keeps selectors centralized and consistent across tests.


52-55: Well-structured navigation method.

This navigation helper properly encapsulates the steps needed to get to the authentication settings tab, which improves code reusability.


57-71: Good implementation of GitHub form interaction.

This method efficiently handles GitHub OAuth configuration with proper environment variable usage and clear action steps.

app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (6)

10-35: Well-structured helper function with proper assertions.

The toggleFormSignupLoginAndSave function efficiently manages form login/signup settings with good conditional logic. It properly waits for toast notifications to disappear before proceeding.


47-51: Good helper function for GitHub form interaction.

This function properly leverages the AdminSettings page object method and includes appropriate waits for UI updates.


76-105: Well-structured setup in before hook.

The setup code properly ensures the required preconditions are met before tests run, with good conditional logic to check and toggle settings only when needed.


134-134: Good usage of LoginFromAPI.

Using the API login method aligns with our best practices and reduces test flakiness.


146-146: Good usage of LoginFromAPI in the second test.

Consistent use of API login methods throughout the test suite.


171-171: Consistent use of LoginFromAPI for authentication.

Good practice to use API methods for login operations.

Comment on lines +128 to +132
agHelper.AssertContains(
"Signup is restricted on this instance of Appsmith",
"exist",
".ads-v2-callout__children",
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid using class selectors for assertions.

Replace the class selector with a data-* attribute selector for the error message.

- agHelper.AssertContains(
-   "Signup is restricted on this instance of Appsmith",
-   "exist",
-   ".ads-v2-callout__children",
- );
+ agHelper.AssertContains(
+   "Signup is restricted on this instance of Appsmith",
+   "exist",
+   "[data-cy=signup-restricted-message]",
+ );
🤖 Prompt for AI Agents
In
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts
between lines 128 and 132, the assertion uses a class selector
".ads-v2-callout__children" which is brittle. Replace this class selector with a
data-* attribute selector that uniquely identifies the error message element to
make the test more stable and maintainable.

Comment on lines +124 to +126
agHelper.TypeText("[type='email']", email);
agHelper.TypeText("[type='password']", password);
agHelper.GetNClick("[type='submit']");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid attribute selectors for form elements.

Use data-* attributes instead of type attributes for form elements as per our coding guidelines.

- agHelper.TypeText("[type='email']", email);
- agHelper.TypeText("[type='password']", password);
- agHelper.GetNClick("[type='submit']");
+ agHelper.TypeText(loginPage.emailInput, email);
+ agHelper.TypeText(loginPage.passwordInput, password);
+ agHelper.GetNClick(loginPage.submitBtn);

Ensure these locators are defined in the LoginPage.json file with data-* attributes.

📝 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.

Suggested change
agHelper.TypeText("[type='email']", email);
agHelper.TypeText("[type='password']", password);
agHelper.GetNClick("[type='submit']");
agHelper.TypeText(loginPage.emailInput, email);
agHelper.TypeText(loginPage.passwordInput, password);
agHelper.GetNClick(loginPage.submitBtn);
🤖 Prompt for AI Agents
In
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts
around lines 124 to 126, replace the attribute selectors using type='email',
type='password', and type='submit' with selectors that use data-* attributes
according to our coding guidelines. Update the locators to use data attributes
defined in the LoginPage.json file, and modify the test code to reference these
new data-* attribute selectors instead of the type attributes.

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15115952402.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:
To know the list of identified flaky tests - Refer here

***** Repeat Run Summary ***** Total Tests with repeat: 2 Total Passed: 1 Total Failed: 1 Total Skipped: 0 *****************************

@jacquesikot
Copy link
Contributor Author

/ci-test-limit-count run_count=1 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts

Copy link

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15129654808.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:
To know the list of identified flaky tests - Refer here

***** Repeat Run Summary ***** Total Tests with repeat: 2 Total Passed: 1 Total Failed: 1 Total Skipped: 0 *****************************


agHelper.AssertElementVisibility(adminSettings.saveButton);
agHelper.GetNClick(adminSettings.saveButton);
agHelper.WaitUntilToastDisappear("Successfully saved");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move all these functions to AdminSettings helper please? Lets keep only the tests here.

@jacquesikot
Copy link
Contributor Author

/ci-test-limit-count run_count=1 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts

Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

♻️ Duplicate comments (3)
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (3)

10-13: Use LogOutviaAPI instead of UI logout.

Per our coding guidelines, prefer API methods for logout operations to improve test reliability.

- const logoutFromApp = () => {
-   agHelper.GetNClick(homePage.profileMenu);
-   agHelper.GetNClick(homePage.signOutIcon);
- };
+ const logoutFromApp = () => {
+   cy.LogOutviaAPI();
+ };

69-71: Avoid attribute selectors for form elements.

Use data-* attributes instead of type attributes for form elements as per our coding guidelines.

- agHelper.TypeText("[type='email']", email);
- agHelper.TypeText("[type='password']", password);
- agHelper.GetNClick("[type='submit']");
+ agHelper.TypeText(loginPage.emailInput, email);
+ agHelper.TypeText(loginPage.passwordInput, password);
+ agHelper.GetNClick(loginPage.submitBtn);

Ensure these locators are defined in the LoginPage.json file with data-* attributes.


73-77: Avoid using class selectors for assertions.

Replace the class selector with a data-* attribute selector for the error message.

- agHelper.AssertContains(
-   "Signup is restricted on this instance of Appsmith",
-   "exist",
-   ".ads-v2-callout__children",
- );
+ agHelper.AssertContains(
+   "Signup is restricted on this instance of Appsmith",
+   "exist",
+   "[data-cy=signup-restricted-message]",
+ );
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (1)

10-18: Move helper functions to AdminSettings helper.

As suggested in previous comments, consider moving these helper functions to the AdminSettings helper class to keep only the tests in this file and improve reusability.

app/client/cypress/support/Pages/AdminSettings.ts (1)

75-79: Consider reducing the timeout value.

The 200-second timeout (200000ms) seems excessive. Consider using a more reasonable timeout like 30-60 seconds to prevent tests from hanging too long.

-    this.agHelper.AssertElementAbsence(AdminsSettings.restartNotice, 200000);
+    this.agHelper.AssertElementAbsence(AdminsSettings.restartNotice, 60000);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8923eee and eb88b21.

📒 Files selected for processing (2)
  • app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/AdminSettings.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`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. ...

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/AdminSettings.ts
  • app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts
🧠 Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts (3)
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34781
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_DuplicateLabel_spec.ts:14-16
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure that login is performed via API using `LoginFromAPI` to avoid UI flakiness.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34781
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_DuplicateLabel_spec.ts:14-16
Timestamp: 2024-07-12T07:45:03.955Z
Learning: In Cypress tests for the Appsmith project, ensure that login is performed via API using `LoginFromAPI` to avoid UI flakiness.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33
Timestamp: 2024-10-08T15:32:34.114Z
Learning: 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: client-lint / client-lint
  • GitHub Check: client-unit-tests / client-unit-tests
  • GitHub Check: client-build / client-build
  • GitHub Check: client-prettier / prettier-check
🔇 Additional comments (4)
app/client/cypress/support/Pages/AdminSettings.ts (4)

3-5: Imports look good.

The new imports follow the established pattern for locator files.


14-14: Property follows best practices.

Uses appropriate data-* attribute selector.


54-57: Navigation method is well-implemented.

Properly reuses existing navigation logic and follows the established pattern.


59-73: Form filling logic is correct.

Properly uses environment variables for OAuth credentials and follows the established pattern.

cy.LoginFromAPI(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));

agHelper.AssertElementVisibility(homePage.homeIcon);
agHelper.GetNClick(homePage.homeIcon, 0, true, 2500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove explicit timeout parameters that mimic cy.wait.

The timeout parameters in GetNClick calls effectively function as wait statements, which should be avoided.

- agHelper.GetNClick(homePage.homeIcon, 0, true, 2500);
+ agHelper.GetNClick(homePage.homeIcon, 0, true);
- agHelper.GetNClick(homePage.homeIcon, 0, true, 2500);
+ agHelper.GetNClick(homePage.homeIcon, 0, true);

Also applies to: 118-118

🤖 Prompt for AI Agents
In
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts
at line 114 (and also at line 118), the explicit timeout parameters in the
GetNClick function are used to create wait-like behavior, which is discouraged.
Remove these timeout arguments from the GetNClick calls to prevent unnecessary
delays and rely on Cypress's default command timeout handling. Ensure the
function calls are updated accordingly without the timeout parameters.

.should("contain.text", "Are you sure?");
agHelper.GetNClick(adminSettings.disconnectBtn);
agHelper.WaitUntilEleAppear(adminSettings.restartNotice);
agHelper.AssertElementAbsence(adminSettings.restartNotice, 200000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Replace long timeout with proper assertions.

The 200000ms timeout in AssertElementAbsence is effectively a wait statement. Use proper UI assertions or wait conditions instead.

- agHelper.AssertElementAbsence(adminSettings.restartNotice, 200000);
+ agHelper.WaitUntilEleDisappear(adminSettings.restartNotice);
📝 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.

Suggested change
agHelper.AssertElementAbsence(adminSettings.restartNotice, 200000);
agHelper.WaitUntilEleDisappear(adminSettings.restartNotice);
🤖 Prompt for AI Agents
In
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts
at line 127, the use of a 200000ms timeout in AssertElementAbsence acts as a
long wait, which is inefficient and unreliable. Replace this with Cypress's
built-in assertions that automatically wait for the element to be absent,
removing the explicit timeout and ensuring the test is more robust and faster.

Comment on lines 81 to 105
public checkAndDisableGithub() {
// Check if GitHub is connected and disconnect if needed
this.agHelper.GetNClick(AdminsSettings.githubButton);
cy.get("body").then(($body) => {
if ($body.find(AdminsSettings.disconnectBtn).length > 0) {
// GitHub is connected, need to disconnect
this.agHelper.GetNClick(AdminsSettings.disconnectBtn);
this.agHelper
.GetElement(AdminsSettings.disconnectBtn)
.should("contain.text", "Are you sure?");
this.agHelper.GetNClick(AdminsSettings.disconnectBtn);
this.agHelper.WaitUntilEleAppear(AdminsSettings.restartNotice);
this.agHelper.AssertElementAbsence(
AdminsSettings.restartNotice,
200000,
);
this.agHelper.WaitForCondition(() =>
this.agHelper.AssertContains("GitHub authentication", "exist"),
);
} else {
// GitHub is already disconnected, go back to auth settings
this.agHelper.GetNClick(AdminsSettings.authenticationTab);
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor to use consistent helper methods.

This method uses cy.get() directly and jQuery methods, which violates the established pattern. Use agHelper methods for consistency.

-    cy.get("body").then(($body) => {
-      if ($body.find(AdminsSettings.disconnectBtn).length > 0) {
+    this.agHelper.GetElement(AdminsSettings.disconnectBtn).then(($element) => {
+      if ($element.length > 0) {

Also reduce the timeout:

-        this.agHelper.AssertElementAbsence(
-          AdminsSettings.restartNotice,
-          200000,
-        );
+        this.agHelper.AssertElementAbsence(
+          AdminsSettings.restartNotice,
+          60000,
+        );
📝 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.

Suggested change
public checkAndDisableGithub() {
// Check if GitHub is connected and disconnect if needed
this.agHelper.GetNClick(AdminsSettings.githubButton);
cy.get("body").then(($body) => {
if ($body.find(AdminsSettings.disconnectBtn).length > 0) {
// GitHub is connected, need to disconnect
this.agHelper.GetNClick(AdminsSettings.disconnectBtn);
this.agHelper
.GetElement(AdminsSettings.disconnectBtn)
.should("contain.text", "Are you sure?");
this.agHelper.GetNClick(AdminsSettings.disconnectBtn);
this.agHelper.WaitUntilEleAppear(AdminsSettings.restartNotice);
this.agHelper.AssertElementAbsence(
AdminsSettings.restartNotice,
200000,
);
this.agHelper.WaitForCondition(() =>
this.agHelper.AssertContains("GitHub authentication", "exist"),
);
} else {
// GitHub is already disconnected, go back to auth settings
this.agHelper.GetNClick(AdminsSettings.authenticationTab);
}
});
}
public checkAndDisableGithub() {
// Check if GitHub is connected and disconnect if needed
this.agHelper.GetNClick(AdminsSettings.githubButton);
this.agHelper.GetElement(AdminsSettings.disconnectBtn).then(($element) => {
if ($element.length > 0) {
// GitHub is connected, need to disconnect
this.agHelper.GetNClick(AdminsSettings.disconnectBtn);
this.agHelper
.GetElement(AdminsSettings.disconnectBtn)
.should("contain.text", "Are you sure?");
this.agHelper.GetNClick(AdminsSettings.disconnectBtn);
this.agHelper.WaitUntilEleAppear(AdminsSettings.restartNotice);
this.agHelper.AssertElementAbsence(
AdminsSettings.restartNotice,
60000,
);
this.agHelper.WaitForCondition(() =>
this.agHelper.AssertContains("GitHub authentication", "exist"),
);
} else {
// GitHub is already disconnected, go back to auth settings
this.agHelper.GetNClick(AdminsSettings.authenticationTab);
}
});
}
🤖 Prompt for AI Agents
In app/client/cypress/support/Pages/AdminSettings.ts lines 81 to 105, the method
uses cy.get() and direct jQuery methods instead of the existing agHelper helper
methods, which should be used for consistency. Replace cy.get() and jQuery
find() with appropriate agHelper methods, and reduce the timeout for assertions
as suggested. Ensure all element interactions and assertions use agHelper
methods for uniformity and update the timeout parameter accordingly.

@jacquesikot
Copy link
Contributor Author

/ci-test-limit-count run_count=1 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts

Copy link

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15259373241.
Cypress dashboard url: Click here!
All Cypress tests have passed 🎉🎉🎉

***** Repeat Run Summary ***** Total Tests with repeat: 2 Total Passed: 2 Total Failed: 0 Total Skipped: 0 *****************************

@jacquesikot
Copy link
Contributor Author

/ci-test-limit-count run_count=10 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts

Copy link

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15259677069.
Cypress dashboard url: Click here!
All Cypress tests have passed 🎉🎉🎉

***** Repeat Run Summary ***** Total Tests with repeat: 20 Total Passed: 20 Total Failed: 0 Total Skipped: 0 *****************************

@jacquesikot
Copy link
Contributor Author

/ci-test-limit-count run_count=2 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts

Copy link

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15262930304.
Cypress dashboard url: Click here!
All Cypress tests have passed 🎉🎉🎉

***** Repeat Run Summary ***** Total Tests with repeat: 4 Total Passed: 4 Total Failed: 0 Total Skipped: 0 *****************************

@jacquesikot jacquesikot requested a review from ankitakinger May 26, 2025 23:06
@jacquesikot jacquesikot added the ok-to-test Required label for CI label May 26, 2025
@jacquesikot
Copy link
Contributor Author

/ci-test-limit-count run_count=1 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.ts

Copy link

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15269760508.
Cypress dashboard url: Click here!
All Cypress tests have passed 🎉🎉🎉

***** Repeat Run Summary ***** Total Tests with repeat: 2 Total Passed: 2 Total Failed: 0 Total Skipped: 0 *****************************

objectsCoreAdminSettings.NavigateToAuthenticationSettings();
objectsCoreAdminSettings.toggleFormSignupLoginAndSave(true, "signup");
});
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the section under comment // validating form signup is disabled from the original file. That cross checks this setting change to true now allows signups without any callout.

cy.LoginFromAPI(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));

agHelper.AssertElementVisibility(homePage.homeIcon);
agHelper.GetNClick(homePage.homeIcon, 0, true, 2500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this?

objectsCoreAdminSettings.toggleFormSignupLoginAndSave(true, "login");
agHelper.AssertElementVisibility(homePage.homeIcon);
agHelper.GetNClick(homePage.homeIcon, 0, true, 2500);
objectsCoreAdminSettings.NavigateToAuthenticationSettings(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steps 76 and 77 not needed. Might as well update 78 to:
objectsCoreAdminSettings.NavigateToAuthenticationSettings(true)

.should("contain.text", "Are you sure?");
agHelper.GetNClick(adminSettings.disconnectBtn);
agHelper.WaitUntilEleAppear(adminSettings.restartNotice);
agHelper.AssertElementAbsence(adminSettings.restartNotice, 200000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets remove line 85 and 86 and instead add:
agHelper.WaitUntilEleAppear(homePage.profileMenu)

Copy link

github-actions bot commented Jun 4, 2025

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

@github-actions github-actions bot added the Stale label Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working ok-to-test Required label for CI Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants