fix: Fixed flaky fork template test case #36200
Conversation
WalkthroughThe pull request introduces modifications to the Cypress end-to-end testing framework, specifically enhancing tests related to forking templates. Key updates include changing element selectors, refining navigation steps, and adding functionality to generate a page with data. Additionally, the specification file for limited tests has been renamed to reflect this broader focus. Overall, these changes streamline test execution and improve the accuracy of user interaction simulations. Changes
Possibly related PRs
Suggested labels
Poem
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/10773057685. |
![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/Templates/Fork_Template_To_App_spec.ts (2 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/limited-tests.txt
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_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 (2)
Common learnings
Learnt from: dipyamanbiswas07 PR: appsmithorg/appsmith#29282 File: app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js:11-17 Timestamp: 2024-01-09T08:39:59.723Z Learning: The user has clarified that intercepts are called in end-to-end tests, and therefore resetting the feature flag in an `afterEach` hook is not required in their test setup.app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_spec.ts (1)
Learnt from: dipyamanbiswas07 PR: appsmithorg/appsmith#29282 File: app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js:11-17 Timestamp: 2024-01-09T08:39:59.723Z Learning: The user has clarified that intercepts are called in end-to-end tests, and therefore resetting the feature flag in an `afterEach` hook is not required in their test setup.
Additional comments not posted (3)
app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_spec.ts (3)
18-18: Use of dynamic locators is commendable.The use of
template.templateCardwith an index suggests a dynamic approach to selecting templates, which aligns with the PR's objective to reduce test flakiness. This is a good practice as it avoids hardcoding and makes the tests more robust.
25-25: Direct interaction with deployed elements simplifies the test flow.This change to directly interact with the deployed page element, as opposed to navigating through multiple UI elements, simplifies the test and reduces potential points of failure. This is a good practice in test automation, making the tests faster and more reliable.
35-35: Dynamic selection of templates enhances test reliability.Repeating the dynamic selection of templates in another test scenario is a good practice. It ensures consistency and reliability across different test cases. This approach helps in maintaining the robustness of the test suite.
app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_spec.ts
Show resolved
Hide resolved
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10773057685. |
## Description Root Cause Analysis (RCA): The issue stemmed from the use of hardcoded templates in our tests, which introduced inherent flakiness. This approach caused intermittent failures, particularly when the hardcoded template was not available or had changed. Solution: I have resolved this by implementing dynamic locators that select the first available template during the test execution. This ensures greater stability and reduces flakiness. The only scenario where this approach could fail is if no templates are available, which would be a product-level blocker rather than a test issue. Fixes #`36199` ## Automation /ok-to-test tags="@tag.Templates" ### 🔍 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/10775279036> > Commit: be8fc87 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10775279036&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Templates` > Spec: > <hr>Mon, 09 Sep 2024 14:51:58 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 - **Bug Fixes** - Improved reliability of the template forking test by updating element selectors and streamlining navigation steps. - Enhanced test coverage by adding a step for generating a page with data. - **Chores** - Renamed test specification file to better reflect its focus on application-level testing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Root Cause Analysis (RCA):
The issue stemmed from the use of hardcoded templates in our tests, which introduced inherent flakiness. This approach caused intermittent failures, particularly when the hardcoded template was not available or had changed.
Solution:
I have resolved this by implementing dynamic locators that select the first available template during the test execution. This ensures greater stability and reduces flakiness. The only scenario where this approach could fail is if no templates are available, which would be a product-level blocker rather than a test issue.
Fixes #
36199Automation
/ok-to-test tags="@tag.Templates"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10775279036
Commit: be8fc87
Cypress dashboard.
Tags:
@tag.TemplatesSpec:
Mon, 09 Sep 2024 14:51:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Chores