test: Admin setting test cases #38172
Conversation
WalkthroughThe pull request introduces significant enhancements to the Cypress end-to-end tests for the Admin Settings page, focusing on improving the structure and readability of the tests through the use of helper functions from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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: 1
🔭 Outside diff range comments (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts (1)
Line range hint
156-158: Replace cy.waitUntil with agHelper methodUse agHelper's assertion methods instead of cy.waitUntil with contains for more reliable and consistent testing.
- cy.waitUntil(() => - cy.contains("General", { timeout: 180000 }).should("be.visible"), - ); + agHelper.WaitUntilEleAppear("General");
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts (2)
43-44: Use agHelper.VisitNAssert for consistencyReplace direct cy.visit with agHelper.VisitNAssert to maintain consistency across the test suite.
- cy.visit("/settings", { timeout: 60000 }); + agHelper.VisitNAssert("/settings", "getEnvVariables");
119-122: Replace direct Cypress commands with agHelper methodsReplace direct cy.get().should() assertions with corresponding agHelper methods for consistency and better maintainability.
- cy.get(adminsSettings.saveButton).should("be.disabled"); - cy.get(adminsSettings.resetButton).should("be.disabled"); + agHelper.AssertElementEnabledDisabled(adminsSettings.saveButton, 0); + agHelper.AssertElementEnabledDisabled(adminsSettings.resetButton, 0);Also applies to: 127-130
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_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.
📓 Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts (2)
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33
Timestamp: 2024-11-12T08:11:36.416Z
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-11-12T08:11:36.416Z
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.
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts (1)
Line range hint 1-196: Overall test structure looks good!
The test cases are well organized and cover the necessary scenarios. The migration to agHelper methods improves maintainability. Consider implementing the suggested changes for better consistency across the test suite.
| cy.get(".admin-settings-menu-option").click(); | ||
| cy.url().should("contain", adminSettingsHelper.routes.GENERAL); | ||
| agHelper.GetNClick(adminSettingsHelper._adminSettingsBtn); | ||
| agHelper.AssertURL(adminSettingsHelper.routes.GENERAL); | ||
| cy.wait("@getEnvVariables"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary cy.wait
Replace cy.wait("@getEnvVariables") with proper assertion using agHelper.ValidateNetworkStatus("@getEnvVariables") to follow best practices.
- cy.wait("@getEnvVariables");
+ agHelper.ValidateNetworkStatus("@getEnvVariables");📝 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.wait("@getEnvVariables"); | |
| agHelper.ValidateNetworkStatus("@getEnvVariables"); |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts(1 hunks)app/client/cypress/locators/AdminsSettings.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
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.
| @@ -46,4 +46,5 @@ export default { | |||
| singleSessionPerUserInput: "[data-testid='singleSessionPerUserEnabled']", | |||
| sessionTimeoutWrapper: ".t--admin-settings-userSessionTimeoutInMinutes", | |||
| sessionTimeoutInput: "[name='userSessionTimeoutInMinutes']", | |||
| adminEmailsData: ".t--admin-settings-APPSMITH_ADMIN_EMAILS span > span", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace CSS path selector with data- attribute.*
The selector uses CSS path with child selectors (span > span). According to our coding guidelines, we should avoid CSS paths and use data-* attributes instead.
Consider this change:
- adminEmailsData: ".t--admin-settings-APPSMITH_ADMIN_EMAILS span > span",
+ adminEmailsData: "[data-testid='admin-settings-emails-data']",Remember to update the corresponding element in your component with the new data-testid attribute.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts (2)
Line range hint
141-196: Add error handling for server restart scenarioWhile the test is well-structured, consider adding error handling for the server restart scenario:
cy.waitUntil(() => - cy.contains("General", { timeout: 180000 }).should("be.visible"), + cy.contains("General", { timeout: 180000 }) + .should("be.visible") + .catch((error) => { + cy.log('Server restart failed:', error.message); + throw error; + }), );
209-232: Refactor repetitive section navigation testsConsider reducing repetition by creating a helper function for section navigation testing:
const sections = [ { tab: adminsSettings.generalTab, route: adminSettingsHelper.routes.GENERAL }, { tab: adminsSettings.advancedTab, route: adminSettingsHelper.routes.ADVANCED }, // ... add other sections ]; sections.forEach(({ tab, route }) => { agHelper.GetNClick(tab); agHelper.AssertURL(route); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_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.
📓 Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts (1)
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33
Timestamp: 2024-11-12T08:11:36.416Z
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.
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts (2)
27-29: Remove unnecessary cy.wait
Replace cy.wait("@getEnvVariables") with proper assertion using agHelper.ValidateNetworkStatus("@getEnvVariables") to follow best practices.
35-38: LGTM! Well-structured access control test
Good use of element absence assertion and URL validation for non-admin access.
|
/ci-test-limit-count run_count=10 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/AdminSettings/Admin_settings_spec.ts |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12466919878. |
## Description Admin setting cases for CE. Fixes # https://app.zenhub.com/workspaces/stability-pod-6690c4814e31602e25cab7fd/issues/gh/appsmithorg/appsmith/38320 ## Automation /ok-to-test tags="@tag.Settings" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12466577616> > Commit: fabd248 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12466577616&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Settings` > Spec: > <hr>Mon, 23 Dec 2024 12:36:13 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced comprehensive end-to-end tests for the Admin Settings page, covering various user scenarios and settings functionality. - Added a new property `adminEmailsData` for improved element selection in tests. - **Chores** - Enhanced the readability and maintainability of end-to-end tests for the Admin Settings page by replacing direct commands with helper functions. - Streamlined element interactions and assertions for improved clarity. - Expanded test coverage with additional cases for default instance name and admin setting sections. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Admin setting cases for CE.
Fixes # https://app.zenhub.com/workspaces/stability-pod-6690c4814e31602e25cab7fd/issues/gh/appsmithorg/appsmith/38320
Automation
/ok-to-test tags="@tag.Settings"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12466577616
Commit: fabd248
Cypress dashboard.
Tags:
@tag.SettingsSpec:
Mon, 23 Dec 2024 12:36:13 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
adminEmailsDatafor improved element selection in tests.Chores