test: updated API edit spec #37262
test: updated API edit spec #37262
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe changes in this pull request involve updates to the Cypress end-to-end test suite for API functionality, specifically enhancing the robustness of a test case related to the confirmation dialog. Additionally, the path for a test specification in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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=100 |
github-actions
bot
added
skip-changelog
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11705049286. |
![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
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js (1)
Line range hint
1-140: General test suite improvements neededSeveral areas in the test suite need attention to align with Cypress best practices:
- Replace hardcoded wait in the last test:
- .wait(3000) + .should('have.value', 'http://host.docker.internal:5001/{{Button2.text}}?key=test&val={{Button2.text}}')
- Consider using data-* attributes instead of class selectors:
- cy.get(".CodeMirror-placeholder") + cy.get('[data-cy="code-editor-placeholder"]')
- Replace jQuery-style selectors:
- cy.get(".ads-v2-modal__content").find("button") + cy.get('[data-cy="confirmation-modal-button"]')
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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.
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: LGTM! Verifying the test file path.
The updated path aligns with the PR's focus on API edit specifications.
Let's verify the existence of the specified test file:
✅ Verification successful
Let me search for the test file in case it's in a different location.
The test file path in limited-tests.txt is correct
The test file exists at app/client/cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js. The path in limited-tests.txt correctly references this file using the Cypress convention of starting from the cypress/e2e/ directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the specified test file
test_file="cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js"
if [ -f "$test_file" ]; then
echo "Test file exists: $test_file"
# Show the first few lines of the file for context
head -n 5 "$test_file"
else
echo "Warning: Test file not found: $test_file"
fi
Length of output: 333
Script:
#!/bin/bash
# Search for API_Edit_spec.js file in the repository
fd "API_Edit_spec.js"
# Search for any file containing API_Edit in the name
fd "API_Edit"
# Let's also check the content of limited-tests.txt to see the context
cat app/client/cypress/limited-tests.txt
Length of output: 510
| cy.get("body").then((body) => { | ||
| if (body.find(apiwidget.confirmBeforeExecute).length > 0) { | ||
| cy.get(apiwidget.confirmBeforeExecute).click({ force: true }); | ||
| } else { | ||
| cy.get(apiwidget.settings).click({ force: true }); | ||
| cy.get(apiwidget.confirmBeforeExecute).click({ force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve element interaction strategy
The current implementation uses jQuery-style DOM checks and forced clicks, which can lead to flaky tests. Consider using Cypress's built-in retry-ability and assertions instead.
Here's a more robust approach:
- cy.get("body").then((body) => {
- if (body.find(apiwidget.confirmBeforeExecute).length > 0) {
- cy.get(apiwidget.confirmBeforeExecute).click({ force: true });
- } else {
- cy.get(apiwidget.settings).click({ force: true });
- cy.get(apiwidget.confirmBeforeExecute).click({ force: true });
- }
- });
+ cy.get(apiwidget.settings).click();
+ cy.get(apiwidget.confirmBeforeExecute)
+ .should('be.visible')
+ .click();Key improvements:
- Removes force: true clicks which bypass Cypress's actionability checks
- Uses Cypress's built-in retry-ability with should('be.visible')
- Simplifies the flow by letting Cypress handle the element's visibility
Committable suggestion skipped: line range outside the PR's diff.
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11705049286. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Updated Confirm button click part where there was flaky behaviour noticed
Summary by CodeRabbit
Bug Fixes
Chores