fix: Fixed Bug29566 spec with list widget #36260
Conversation
WalkthroughThe changes involve significant modifications to a Cypress test file, specifically Changes
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
|
/ci-test-limit-count run_count=20 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10814867598. |
![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
Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/Bug29566_Spec.ts (1)
21-54: Excellent work on the test case!The test case follows a clear and logical flow, covering the essential aspects of the bug being tested. Here's a breakdown of the test case:
- Set up the data source and query.
- Navigate through the editor and configure the widget.
- Deploy the app and assert the widget's behavior.
- Reload the page and re-assert the widget's behavior.
The test case is well-structured and effectively tests the list widget's behavior on page load.
A few suggestions for improvement:
- Consider extracting the repeated assertions into a separate function to improve readability and maintainability.
- Add comments to explain the purpose of each section of the test case, making it easier for other developers to understand the test's flow.
Here's an example of how you can extract the repeated assertions into a separate function:
function assertWidgetText(locator: string, expectedTexts: string[]) { agHelper.AssertElementLength(locator, expectedTexts.length); agHelper.GetText(locator).then((texts: string[]) => { expect(texts).to.deep.equal(expectedTexts); }); }Then, you can use this function in the test case:
agHelper .GetText(locators._widgetInCanvas("textwidget") + " span") .then((initialTexts: string[]) => { deployMode.DeployApp(); assertWidgetText(locators._textWidgetInDeployed, initialTexts); agHelper.CypressReload(); assertWidgetText(locators._textWidgetInDeployed, initialTexts); });
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/BugTests/Bug29566_Spec.ts (1 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
Additional context used
Path-based instructions (2)
app/client/cypress/limited-tests.txt (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/BugTests/Bug29566_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.
Additional comments not posted (5)
app/client/cypress/limited-tests.txt (1)
2-2: Good choice for limited tests! 👍Focusing the limited tests on
Bug29566_Spec.tsaligns well with the PR objective of fixing the flaky test in that file. This change ensures that the problematic test gets adequate attention during the CI/CD process.app/client/cypress/e2e/Regression/ClientSide/BugTests/Bug29566_Spec.ts (4)
1-14: Great job organizing the imports!The new imports for
dataSources,deployMode,locators,propPane,EditorNavigation,AppSidebar,AppSidebarButton,EntityType,PageLeftPane, andPagePaneSegmentare well-structured and indicate a clear understanding of the test's requirements.Importing these modules from the appropriate locations will help keep the test code clean and maintainable.
16-19: Excellent setup in thebeforehook!Adding the DSL using
agHelper.AddDsl("Listv2/simpleList")in thebeforehook is a great way to ensure that the necessary setup is performed before running the test case.This approach helps keep the test case focused on the specific scenario being tested and improves the overall organization of the test file.
37-38: Great job using locator variables!Using locator variables like
locators._widgetInCanvas("textwidget")andlocators._textWidgetInDeployedinstead of plain strings is a best practice in Cypress testing.This approach helps maintain consistency and makes the test code more readable and maintainable. It also makes it easier to update the locators if the application's structure changes in the future.
Keep up the good work!
Also applies to: 40-40, 42-43, 47-47
44-45: Excellent use of multiple assertions!Using multiple assertions for the
expectstatements, such as comparing the deployed texts with the initial texts, is a best practice in Cypress testing.This approach ensures that the test case covers all the necessary conditions and provides more informative error messages if an assertion fails. It also makes the test case more robust and less likely to produce false positives.
Great job implementing this best practice!
Also applies to: 51-52
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10814867598. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/ListWidgetOnPageLoad_Spec.ts (1)
1-55: Great test case! Here are a few suggestions to make it even better:
Use the actual data source name instead of relying on the
@dsNamealias, which is not defined in this file. This will make the test case more self-contained and easier to understand.Use
cy.reload()instead ofagHelper.CypressReload()for reloading the page.cy.reload()is a standard Cypress command and is more idiomatic.Consider moving the data source creation and query configuration to a
beforehook. This way, it will be executed only once for all test cases in this file, making the tests faster and more efficient.For example:
before(() => { agHelper.AddDsl("Listv2/simpleList"); dataSources.CreateDataSource("Postgres"); dataSources.CreateQueryAfterDSSaved( "SELECT id,name FROM public.users LIMIT 10;", ); dataSources.ToggleUsePreparedStatement(false); dataSources.RunQuery(); }); it("1. List widget gets populated on page load. Bug: 29566", function () { // Rest of the test case... cy.reload(); // ... });Other than that, the test case follows many of the best practices for Cypress tests, such as using locator variables, data-* attributes for selectors, performing multiple assertions, and avoiding duplication and unnecessary delays. Well done!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/BugTests/ListWidgetOnPageLoad_Spec.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/ListWidgetOnPageLoad_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.
## Description **Root Cause Analysis (RCA):** The test case in `BugTests/Bug29566_Spec.ts` has become flaky due to its dependency on a third-party API call `(https://api.jsonbin.io/v3/b/)`. As experienced automation engineers, we understand that relying on third-party services can introduce instability and lead to frequent test failures. In this particular case, the dependency was tightly coupled with the test logic, and updating the data was not a viable solution. **Solution:** Upon assessing the situation, I determined that refactoring the existing test would take significantly longer than rewriting it from scratch. Therefore, I chose the rewriting approach to prioritize time efficiency. The test flow has been updated while maintaining the same assertions for data validation. Additionally, I have incorporated an extra assertion to verify the data after a page reload to further enhance the robustness of the test. Fixes #`36259` ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 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/10816309843> > Commit: 2be906c > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10816309843&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 11 Sep 2024 17:34:50 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** - Enhanced testing for the list widget to ensure it populates correctly on page load, addressing Bug 29566. - Introduced dynamic data sourcing for more accurate test scenarios. - **Bug Fixes** - Updated test cases to reflect changes in functionality and validate correct data display. - **Documentation** - Renamed test case for clarity regarding the specific bug being addressed. - **Chores** - Updated the limited tests configuration to focus on the new bug test instead of a template test. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Root Cause Analysis (RCA):
The test case in
BugTests/Bug29566_Spec.tshas become flaky due to its dependency on a third-party API call(https://api.jsonbin.io/v3/b/). As experienced automation engineers, we understand that relying on third-party services can introduce instability and lead to frequent test failures. In this particular case, the dependency was tightly coupled with the test logic, and updating the data was not a viable solution.Solution:
Upon assessing the situation, I determined that refactoring the existing test would take significantly longer than rewriting it from scratch. Therefore, I chose the rewriting approach to prioritize time efficiency. The test flow has been updated while maintaining the same assertions for data validation. Additionally, I have incorporated an extra assertion to verify the data after a page reload to further enhance the robustness of the test.
Fixes #
36259Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10816309843
Commit: 2be906c
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 11 Sep 2024 17:34:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores