fix: Fix flaky test case for the iframe spec #38018
Conversation
WalkthroughThe pull request introduces extensive modifications to the test suite for the Iframe widget, including changes to the testing setup, assertions, and the addition of new test cases. Key updates involve the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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=10 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12197427047. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Iframe/IframeTest_spec.ts (2)
85-87: Prefer locator variables over tag selectorsReplace the tag selector
"input"with a locator variable or data-* attribute for better specificity and maintainability.Suggested change:
Define a locator variable in
CommonLocators.ts:_inputField = "input.t--widget-inputwidget";Update the test code:
-getIframeBody(0) - .find("input") +getIframeBody(0) + .find(locators._inputField)
90-90: Avoid using hardcoded strings in assertionsConsider using constants or variables instead of hardcoded strings in assertions to enhance test flexibility.
Suggested change:
Define a constant for the expected value:
const expectedValue = "submitclicked";Then update the assertion:
-expect(inputValue).to.equal("submitclicked"); +expect(inputValue).to.equal(expectedValue);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Iframe/IframeTest_spec.ts(2 hunks)app/client/cypress/fixtures/IframeWidgetPostMessage.json(1 hunks)app/client/cypress/fixtures/testdata.json(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)app/client/cypress/support/Objects/CommonLocators.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/cypress/limited-tests.txt
- app/client/cypress/fixtures/testdata.json
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/support/Objects/CommonLocators.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/fixtures/IframeWidgetPostMessage.json (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/Widgets/Iframe/IframeTest_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 (1)
app/client/cypress/fixtures/IframeWidgetPostMessage.json (1)
1-1: LGTM
The fixture file is correctly structured.
| .find(".navbar__logo > img") | ||
| .eq(0) | ||
| .should("have.attr", "src") | ||
| .and("include", "logo"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use locator variables instead of hardcoded CSS selectors
Replace the hardcoded CSS selector .navbar__logo > img with a locator variable or data-* attribute to adhere to the coding guidelines.
Suggested change:
Define a locator variable in CommonLocators.ts:
_pageHeaderLogoImage = ".navbar__logo > img";Update the test code:
-getIframeBody(1)
- .find(".navbar__logo > img")
+getIframeBody(1)
+ .find(locators._pageHeaderLogoImage)| propPane.UpdatePropertyFieldValue("URL", " "); | ||
| agHelper.ValidateToastMessage("url updated"); | ||
|
|
||
| agHelper.ClickButton("Submit"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded button text with locator variable
Use a locator variable instead of the hardcoded button text "Submit" to comply with the coding guidelines.
Suggested change:
Define a locator variable in CommonLocators.ts:
_submitButton = "button.t--widget-buttonwidget";Update the test code:
-agHelper.ClickButton("Submit");
+agHelper.GetNClick(locators._submitButton);| _pageHeaderToggle = ".navbar__items>button"; | ||
| _pageHeaderMenuList = ".navbar-sidebar__backdrop"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data- attributes instead of CSS selectors*
Replace the CSS selectors with data-* attributes to comply with the coding guidelines.
Suggested change:
If possible, assign data attributes to the elements and update the locators accordingly.
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12197427047. |
## Description Re-wrote new test case. Fixes # https://app.zenhub.com/workspaces/stability-pod-6690c4814e31602e25cab7fd/issues/gh/appsmithorg/appsmith/38017 ## Automation /ok-to-test tags="@tag.Iframe" ### 🔍 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/12197419550> > Commit: 5b0d7ad > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12197419550&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Iframe` > Spec: > <hr>Fri, 06 Dec 2024 11:07:20 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 ## Release Notes - **New Features** - Enhanced testing suite for the Iframe widget, allowing for more specific interactions with multiple iframe instances. - Added a new test case to verify iframe behavior during property updates. - **Bug Fixes** - Updated iframe URL configurations to ensure correct redirection to the new documentation site. - **Documentation** - Introduced a new JSON structure for the Iframe widget, detailing properties and settings for improved user interaction. - **Chores** - Updated test specification paths to focus on the Iframe widget tests. - Modified common locators for improved navigation element targeting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Re-wrote new test case.
Fixes # https://app.zenhub.com/workspaces/stability-pod-6690c4814e31602e25cab7fd/issues/gh/appsmithorg/appsmith/38017
Automation
/ok-to-test tags="@tag.Iframe"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12197419550
Commit: 5b0d7ad
Cypress dashboard.
Tags:
@tag.IframeSpec:
Fri, 06 Dec 2024 11:07:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores