test: updated DS test and common method #36204
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request implements changes to the Changes
Possibly related PRs
Suggested labels
Poem
Tip New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts (3 hunks)
- app/client/cypress/support/Pages/DataSources.ts (1 hunks)
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_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/DataSources.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.
Additional comments not posted (4)
app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts (4)
6-6: Use of locator variables is approved.The addition of
locatorsto the import list aligns with best practices for Cypress tests, ensuring that selectors are managed centrally and not hardcoded throughout the test cases.
75-75: Good use of data attributes for selectors.Using
data-testidattributes for selectors is a best practice in testing because it decouples the test selectors from CSS classes and IDs that may change with CSS changes. This practice enhances the maintainability of the tests.Also applies to: 93-93
80-81: Ensure multiple assertions per test case.It's good to see multiple assertions being used in the test cases. This practice helps ensure that the tests are robust and can catch multiple issues in one run. However, make sure that the assertions are not overly dependent on each other, which could lead to cascading test failures if one assertion fails.
Also applies to: 98-98
74-74: Avoid using explicit waits likeagHelper.Sleep(1500).Using explicit waits can lead to brittle tests that may fail unpredictably depending on external factors like network latency or processing speed. Instead, consider using Cypress's built-in wait mechanisms to wait for specific conditions or elements to be ready.
Here's a suggested refactor to use a more reliable waiting mechanism:
- agHelper.Sleep(1500); + cy.wait('@getDatasourceStructure');Also applies to: 92-92
Skipped due to learnings
Learnt from: sagar-qa007 PR: appsmithorg/appsmith#35412 File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99 Timestamp: 2024-08-06T05:59:19.000Z 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: sagar-qa007 PR: appsmithorg/appsmith#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33 Timestamp: 2024-07-16T08:25:23.799Z Learning: Avoid using sleep functions like `agHelper.Sleep` and `this.Sleep` 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#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56 Timestamp: 2024-07-16T06:44:55.263Z Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep` 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#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56 Timestamp: 2024-07-16T06:48:43.110Z 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.
| @@ -1373,7 +1373,7 @@ export class DataSources { | |||
| expectedTableName = search, | |||
| ) { | |||
| this.agHelper.Sleep(2500); //for query editor to load | |||
| this.agHelper.TypeText(this._datasourceStructureSearchInput, search); | |||
| this.agHelper.ClearNType(this._datasourceStructureSearchInput, search); | |||
There was a problem hiding this comment.
Refactor suggestion to replace explicit waits with conditional waits.
Using explicit waits like agHelper.Sleep(1000) can lead to non-deterministic test behaviors and longer than necessary test times. Consider using Cypress's conditional waiting features to wait for specific conditions or elements to be ready.
Here's a suggested refactor:
- agHelper.Sleep(1000); //for search result to load
+ cy.wait('@searchResults'); // Assuming an alias for the search network callCommittable suggestion was skipped due to low confidence.
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10775575341. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts (3 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
- app/client/cypress/support/Pages/DataSources.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/limited-tests.txt
Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts
- app/client/cypress/support/Pages/DataSources.ts
|
/ci-test-limit runId=10775575341 |
4 similar comments
|
/ci-test-limit runId=10775575341 |
|
/ci-test-limit runId=10775575341 |
|
/ci-test-limit runId=10775575341 |
|
/ci-test-limit runId=10775575341 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10776809936. |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10776809238. |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10776809510. |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10776810764. |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10776814824. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10776814824. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Summary by CodeRabbit
Bug Fixes
New Features
Warning
Tests have not run on the HEAD 7e15730 yet
Mon, 09 Sep 2024 14:43:48 UTC