fix: Property control spec #38004
fix: Property control spec #38004
Conversation
|
/ci-test-limit-count run_count=5 |
WalkthroughThe changes in this pull request involve updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 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
|
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12194954818. |
![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 (1)
app/client/cypress/limited-tests.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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.
🔇 Additional comments (1)
app/client/cypress/limited-tests.txt (1)
2-2: Verify test coverage migration from the old test file.
Need to ensure the test coverage from the removed Fork_Template_spec.js is maintained.
✅ Verification successful
Let me check the content of both files to compare the test coverage.
Let me try one more search to find any traces of the old test file or its content.
Let me try one more search to find any test coverage related to fork templates.
Test coverage for fork template functionality is maintained in ForkTemplateToGitConnectedApp.js
The test coverage has been migrated and enhanced in ForkTemplateToGitConnectedApp.js, which includes testing for:
- Forking templates into git-connected apps
- Forking templates to child branches
- Cloning of JS Objects during template forking
- Error handling and success states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if fork template testing is covered elsewhere
# Search for other files that might contain fork template testing
rg -l "fork.*template" app/client/cypress/e2e
# Check if the old test file still exists (should be removed if replaced)
fd "Fork_Template_spec.js" app/client/cypress
Length of output: 181
Script:
#!/bin/bash
# Check content of both files to compare test coverage
echo "=== Content of ForkTemplateToGitConnectedApp.js ==="
cat app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js
# Search for the old test file in the entire repository to ensure we get its content
echo -e "\n=== Searching for Fork_Template_spec.js content ==="
rg -l "Fork_Template_spec" app/client/cypress
Length of output: 4740
Script:
#!/bin/bash
# Search for any references to Fork_Template in the entire codebase
rg -i "fork.*template.*spec" app/client/cypress
# Search in limited-tests.txt for any history
cat app/client/cypress/limited-tests.txt
Length of output: 130759
Script:
#!/bin/bash
# Search for fork template testing in the entire codebase
rg -A 5 "fork.*template" app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js
# Check if there are any other test files containing fork template tests
rg -l "fork.*template" app/client/cypress/e2e/Regression/ClientSide/Templates/
Length of output: 1264
app/client/cypress/limited-tests.txt
Outdated
| @@ -1,5 +1,5 @@ | |||
| # To run only limited tests - give the spec names in below format: | |||
| cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js | |||
| cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts | |||
There was a problem hiding this comment.
💡 Codebase verification
Test file exists but contains some test anti-patterns that need attention
- Found usage of
cy.wait(500)which is a flaky test practice. Consider using Cypress's built-in automatic retry and wait mechanisms - Test follows proper structure with describe/it blocks and meaningful test descriptions
- No discouraged selectors found
- The old Fork_Template_spec file is not present in the codebase
🔗 Analysis chain
Verify the new test specification exists and follows Cypress best practices.
Let's ensure the new test file exists and follows the required guidelines.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the new test file
# Check if the new test file exists
fd "PropertyControl_spec.ts" app/client/cypress
# Check for potential test anti-patterns
rg -A 3 "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts" || echo "No anti-patterns found"
# Check selector usage
rg "\.btn\.submit|button\[type=submit\]|\$\('" "app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts" || echo "No discouraged selectors found"
# Verify login/logout method usage
rg "LoginFromAPI|LogOutviaAPI|SignupFromAPI" "app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts"
Length of output: 802
Script:
#!/bin/bash
# Let's check the content of the test file to verify best practices
cat "app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts"
# Also check if the old test file exists somewhere else
fd "Fork_Template_spec" app/client/cypress
Length of output: 7983
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12194954818. |
|
/ci-test-limit-count run_count=10 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12197827729. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts (1)
Line range hint
41-41: Replace cy.wait with proper assertionsUsing
cy.waitis against our coding guidelines. Replace it with proper assertions or commands that wait for specific conditions.- cy.wait(500); + cy.get('[data-testid="t--selected-widget-Table1"]').should('exist');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_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 (2)
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts (2)
Line range hint 15-252: Test structure and assertions look good
The test cases are well-structured with:
- Clear test descriptions
- Multiple assertions for validation
- Proper use of Cypress commands
- Good separation of concerns between test cases
100-100: Verify email column existence in datasource schemas
The searchableColumn has been changed to "email" for both Users and Movies tables. Please verify that:
- The "email" column exists in both datasource schemas
- The column is appropriate for search functionality
Also applies to: 108-108
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12197827729. |
|
/ci-test-limit-count run_count=10 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12233135663. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12233135663. |
|
/ci-test-limit-count run_count=10 runId=12233135663 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12233750098. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12233750098. |
|
/ci-test-limit-count run_count=20 runId=12233135663 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12236178499. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12236178499. |
## Description Fix test case as DB data updated. Fixes # https://app.zenhub.com/workspaces/stability-pod-6690c4814e31602e25cab7fd/issues/gh/appsmithorg/appsmith/38022 ## Automation /ok-to-test tags="@tag.Widget" ### 🔍 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/12236809570> > Commit: 046fc7a > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12236809570&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget` > Spec: > <hr>Mon, 09 Dec 2024 14:51:56 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 ## Summary by CodeRabbit - **Tests** - Updated test specifications to focus on property control functionality instead of fork template functionality. - Modified searchable column parameters in tests to enhance data relevance for property control interactions. - Ensured continued validation of datasource dropdowns and search functionality within the tests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fix test case as DB data updated.
Fixes # https://app.zenhub.com/workspaces/stability-pod-6690c4814e31602e25cab7fd/issues/gh/appsmithorg/appsmith/38022
Automation
/ok-to-test tags="@tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12236809570
Commit: 046fc7a
Cypress dashboard.
Tags:
@tag.WidgetSpec:
Mon, 09 Dec 2024 14:51:56 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit