test: replaced 3rd party apit in treeSelect #36598
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces significant changes to the Cypress testing framework for the TreeSelect widget. It updates the API endpoint from a mock to a local Docker host, modifies data processing methods, and changes the image file format for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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=25 |
github-actions
bot
added
skip-changelog
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11092865484. |
![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/Widgets/TreeSelect/Tree_Select_2_spec.ts (3 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TreeSelect/Tree_Select_2_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.
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 (4)
app/client/cypress/limited-tests.txt (2)
2-2: Well done, class! Let's examine this change.I'm pleased to see that you've updated the test specification path. This change aligns perfectly with our lesson on replacing the 3rd party API in treeSelect. Moreover, you've made an excellent decision to switch from JavaScript to TypeScript. Can anyone tell me why this is beneficial?
Line range hint
1-7: Excellent work on maintaining the file structure!Class, I want you to notice how well-organized this file is. The comments clearly explain its purpose and how to use it. Remember, good documentation is just as important as good code. Keep up the good work!
app/client/cypress/e2e/Regression/ClientSide/Widgets/TreeSelect/Tree_Select_2_spec.ts (2)
Line range hint
1-438: Let's take a step back and look at the bigger picture of our test file.I've noticed a few areas where we can improve our coding practices:
I see you're using
cy.waitin some places. Remember, we want to avoid usingcy.waitin our Cypress tests. Instead, try using Cypress's built-in retry-ability and assertions to wait for elements or states.There are some hardcoded selectors in the test. We should be using data-* attributes for selectors to make our tests more robust and easier to maintain.
I don't see any use of
LoginFromAPI,LogOutviaAPI, orSignupFromAPI. If these are applicable to our test scenario, we should consider using them to improve test efficiency.Let's work on refactoring these areas to align with our best practices. It will make our tests more reliable and easier to maintain in the long run.
Let's run a quick check to see how many times we're using cy.wait:
#!/bin/bash # Count occurrences of cy.wait grep -c "cy.wait" app/client/cypress/e2e/Regression/ClientSide/Widgets/TreeSelect/Tree_Select_2_spec.tsThis will give us an idea of how many instances we need to refactor.
173-175: Class, let's examine the changes in our API setup and data handling.I see you've updated the API endpoint to use a local Docker host. While this might work for your current setup, it's important to consider the portability of our tests. Remember, good tests should be able to run in various environments!
As for the data mapping, you've made it more specific to the expected data structure. That's a step in the right direction, but let's make sure we're not making our tests too brittle. Can you explain your reasoning behind this change?
Let's run a quick check to see if this API is accessible:
This will help us understand if the new endpoint is reliable for our tests.
Also applies to: 186-186
| `{{download('http://host.docker.internal:4200/photo-1503469432756-4aae2e18d881.jpeg', 'flower.svg', 'image/svg+xml').then(() => { | ||
| showAlert('Download Success', ''); | ||
| });}}`, | ||
| ); |
There was a problem hiding this comment.
Now, let's turn our attention to the onOptionChange function, specifically the download part.
I notice you've changed the download URL to a JPEG image. That's fine, but your success message still talks about an SVG file. Remember, consistency is key in coding! Let's make sure our messages accurately reflect what we're doing.
Also, I'd like you to think about error handling here. What happens if the download fails? It's always good to plan for different scenarios!
Let's update the success message to match the file type:
- `{{download('http://host.docker.internal:4200/photo-1503469432756-4aae2e18d881.jpeg', 'flower.svg', 'image/svg+xml').then(() => {
+ `{{download('http://host.docker.internal:4200/photo-1503469432756-4aae2e18d881.jpeg', 'flower.jpeg', 'image/jpeg').then(() => {
showAlert('Download Success', '');
});}}`,This change ensures our message matches the actual file being downloaded.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `{{download('http://host.docker.internal:4200/photo-1503469432756-4aae2e18d881.jpeg', 'flower.svg', 'image/svg+xml').then(() => { | |
| showAlert('Download Success', ''); | |
| });}}`, | |
| ); | |
| `{{download('http://host.docker.internal:4200/photo-1503469432756-4aae2e18d881.jpeg', 'flower.jpeg', 'image/jpeg').then(() => { | |
| showAlert('Download Success', ''); | |
| });}}`, | |
| ); |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11092865484. |
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11094221108. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11094221108. |
reverted limited run file
Replaced 3rd Party API with TED
/ok-to-test tags="@tag.Select"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11102740010
Commit: 692dd28
Cypress dashboard.
Tags:
@tag.SelectSpec:
Mon, 30 Sep 2024 09:21:05 UTC