test: Skipping one test in API Edit Spec for functional bugs #38115
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThis pull request involves updates to Cypress testing configurations across multiple files. The changes primarily focus on modifying locator definitions and adjusting test case execution statuses. Specifically, the modifications include updating a selector in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)app/client/cypress/limited-tests.txt (1)Pattern
🔇 Additional comments (1)app/client/cypress/limited-tests.txt (1)
The change aligns with the PR objectives regarding API Edit Spec tests. Let's verify the existence of the specified test file: ✅ Verification successfulTest file path verified successfully The specified test file exists at the expected location: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the existence of the specified test file
# Expected: File should exist and be accessible
fd "API_Edit_spec.js" --type f
Length of output: 101 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
|
|
/ci-test-limit-count run_count=50 |
github-actions
bot
added
skip-changelog
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12283203248. |
![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/limited-tests.txt (1)
Line range hint
1-6: Consider documenting the test coverage impactWhile the change from template tests to API tests is clear, it would be helpful to document why this specific test was chosen for the limited test run.
Consider adding a comment explaining the test selection criteria, for example:
# To run only limited tests - give the spec names in below format: cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js +# Selected for critical API editing functionality coverage # For running all specs - uncomment below: #cypress/e2e/**/**/*
📜 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 the existence of the API Edit spec file
The change to use the API Edit spec looks correct, but let's verify the file exists and follows the Cypress best practices.
✅ Verification successful
Test spec file path is correct and valid
The API_Edit_spec.js file exists at the correct location: app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js, which matches the path specified in limited-tests.txt. The configuration is properly set up for CI test limits.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new test spec exists and verify its content
if fd "API_Edit_spec.js" | grep -q "ApiTests"; then
echo "File exists. Checking content..."
# Check for common anti-patterns
rg -i "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js" || echo "No common anti-patterns found"
else
echo "Warning: Test spec file not found"
fi
Length of output: 418
Script:
#!/bin/bash
# Let's verify if the file exists in a different location or with a different case
fd -i "api.*edit.*spec\.js" --type f
# Also check the content of limited-tests.txt to understand the context better
cat app/client/cypress/limited-tests.txt
Length of output: 442
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12283203248. |
|
/ci-test-limit-count run_count=10 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/locators/apiWidgetslocator.json (1)
Line range hint
1-62: Consider refactoring other locators to use data-testid.Many locators in this file use XPath, text content, or CSS attribute selectors which violate the coding guidelines. Consider refactoring them to use data-testid attributes for better maintainability and reliability.
Examples of locators that should be updated:
searchInputPlaceholder: Uses XPathcreateapi: Uses CSS classmoveTo: Uses text contentpopover: Uses XPath with classSuggested pattern:
{ - "searchInputPlaceholder": "//div[contains(@class, 't--dataSourceField')]//div//input", + "searchInputPlaceholder": "[data-testid='t--datasource-field-input']", - "createapi": ".t--createBlankApiCard", + "createapi": "[data-testid='t--create-blank-api']", - "moveTo": ".single-select >div:contains('Move to')", + "moveTo": "[data-testid='t--move-to-option']" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/locators/apiWidgetslocator.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/locators/apiWidgetslocator.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.
🔇 Additional comments (1)
app/client/cypress/locators/apiWidgetslocator.json (1)
50-50: LGTM! Good use of data-testid attribute.
The change from text-based selector to data-testid is aligned with the coding guidelines and best practices for Cypress tests.
|
/ci-test-limit-count run_count=10 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12299787751. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12299787751. |
|
/ci-test-limit-count run_count= 100 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12301193327. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12301193327. |
|
/ci-test-limit-count run_count= 100 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12309363236. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12309363236. |
|
/ci-test-limit-count run_count= 50 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12319609183. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js (3)
Line range hint
1-25: Consider using beforeEach for test isolationWhile the current setup is functional, consider moving the
agHelper.AddDsl("uiBindDsl")to abeforeEachblock to ensure each test starts with a clean state.- before(() => { + beforeEach(() => { agHelper.AddDsl("uiBindDsl"); });
Line range hint
51-63: Test implementation needs improvement before re-enablingWhile skipping the test is appropriate due to the bug, there are several improvements needed:
- The test uses force clicks which should be avoided
- Uses cy.wait implicitly through click operations
- Selector usage could be improved
Consider refactoring the test when fixing the bug:
it("2. Should update loading state after cancellation of confirmation for run query", function () { cy.CreateAPI("FirstAPI"); - cy.get(".CodeMirror-placeholder") + cy.get("[data-testid='codemirror-placeholder']") .first() .should("have.text", "https://mock-api.appsmith.com/users"); cy.log("Creation of FirstAPI Action successful"); cy.enterDatasourceAndPath(testdata.baseUrl, testdata.methods); - cy.get(apiwidget.settings).click({ force: true }); - cy.get(apiwidget.confirmBeforeExecute).click({ force: true }); + cy.get(apiwidget.settings).should('be.visible').click(); + cy.get(apiwidget.confirmBeforeExecute).should('be.visible').click(); cy.get(apiwidget.runQueryButton).click(); cy.get(".ads-v2-modal__content").find("button").contains("No").click(); cy.get(apiwidget.runQueryButton).children().should("have.length", 1); });
Test implementation needs improvements for stability and maintainability
The verification confirms multiple test stability issues:
- Found 8 instances of
force: truein API_Edit_spec.js- Multiple direct CSS selectors (.CodeMirror-placeholder, .ads-v2-modal__content)
- No data-testid attributes in use
- Several instances of
.first()selector usage- Explicit waits present in related API test files
Required changes:
- Replace CSS selectors with data-testid attributes (e.g.,
.CodeMirror-placeholder,.ads-v2-modal__content)- Remove force clicks and implement proper element waiting strategies
- Use specific element targeting instead of
.first()- Replace explicit waits with Cypress's built-in retry-ability and assertions
🔗 Analysis chain
Line range hint
1-140: General test implementation improvements neededSeveral patterns throughout the test suite need attention:
- Avoid force clicks (used in multiple tests)
- Replace hardcoded selectors with data-testid attributes
- Remove explicit waits and timeouts
- Improve error handling in API calls
Consider implementing these improvements across all tests:
- Use
data-testidattributes for all selectors- Replace waits with proper assertions
- Implement proper API error handling
- Use custom commands for common operations
The shell script results confirm the patterns mentioned in the review comment. Let me check for data-testid attributes to complete the verification.
Let me check the locator patterns being used in these tests to better understand the current selector strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for force clicks and hardcoded selectors rg --type js "force: true|cy\.wait|sleep|\.first\(\)" app/client/cypress/e2e/Regression/ServerSide/ApiTests/Length of output: 4247
Script:
#!/bin/bash # Check for data-testid usage in API tests rg --type js "data-testid|data-cy" app/client/cypress/e2e/Regression/ServerSide/ApiTests/Length of output: 91
Script:
#!/bin/bash # Check selector patterns in API tests rg --type js "cy\.get\(['\"].*['\"]" app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.jsLength of output: 308
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js (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.
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12319609183. |
NandanAnantharamu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/cypress/e2e/Sanity/Datasources/RestApiOAuth2Validation_spec.ts (3)
19-20: Consider using a more trackable skip mechanismWhile the skip is documented with an issue reference, consider using Cypress-native pending mechanism with additional metadata for better tracking.
- //Existing Bug: https://github.com/appsmithorg/appsmith/issues/37353 - it.skip("1. Create an API with app url and save as Datasource for Authorization code details test", function () { + // @issue https://github.com/appsmithorg/appsmith/issues/37353 + it("1. Create an API with app url and save as Datasource for Authorization code details test", { pending: true }, function () {
Line range hint
43-52: Maintain consistency in test improvementsApply the same improvements suggested for the first test:
- Replace
agHelper.GetNClickwith Cypress native commands- Consider extracting the consent handling logic into a reusable helper
Line range hint
1-54: Consider architectural improvements for OAuth2 testingTo improve the maintainability and reliability of OAuth2 testing:
- Create a dedicated OAuth2 testing utility class
- Move configuration to environment variables
- Implement proper test isolation and cleanup
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Sanity/Datasources/RestApiOAuth2Validation_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Sanity/Datasources/RestApiOAuth2Validation_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/Sanity/Datasources/RestApiOAuth2Validation_spec.ts (2)
Line range hint 1-18: LGTM! Well-structured test setup
The imports and describe block follow Cypress best practices with appropriate tags and clear descriptions.
Line range hint 21-41: Refactor to follow Cypress best practices
The test implementation could be improved to better align with the provided guidelines:
- Avoid using
agHelper.GetNClickas it might involve implicit waits - Consider moving API URLs to a configuration file
|
/ci-test-limit-count run_count=100 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12345944297. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12345944297. |
cypress/e2e/Sanity/Datasources/RestApiOAuth2Validation_spec.ts --> there is a bug hence commenting this test
cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js --> there is a bug hence commenting one test in this spec file
/ok-to-test tags="@tag.Sanity"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12349492331
Commit: b7237c4
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 16 Dec 2024 10:00:38 UTC
Summary by CodeRabbit
Bug Fixes
Tests