test: replace 3rd party api with TED #36594
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces significant modifications to the test specifications for server-side on-load tests. It updates the API endpoint from a public URL to a local host address, alters the associated JavaScript function names, and changes the verification messages in the tests. Additionally, the specification for limited tests has been redirected to focus on the new server-side tests instead of client-side templates. 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
|
github-actions
bot
added
skip-changelog
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11081524333. |
![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 (4)
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JsOnLoad3_Spec.ts (4)
40-41: Good job updating the API endpoint, class! Let's make it even better!I see you've updated the API endpoint to use TED, which is exactly what we wanted. Well done! However, let's think about making our code more flexible. Can you tell me why using an environment variable for the API URL might be a good idea?
Consider using an environment variable for the API URL:
`${Cypress.env('TED_API_URL') || 'http://host.docker.internal:5001'}`This way, we can easily change the URL for different environments without modifying the code. What do you think about this approach?
57-58: Excellent work updating the function name and call! Let's add a little extra credit!You've done a great job updating the function to use the new AppsmithTed API. That's exactly what we were looking for! Now, let's think about error handling. What might happen if the API call fails?
Consider adding error handling to make our test more robust:
callAppsmithTed: async () => { try { return await AppsmithTed.run(); } catch (error) { cy.log('Error calling AppsmithTed:', error); throw error; } }This way, if there's an error, we'll see it in our Cypress logs. What do you think about this addition?
74-74: Nice update to the function name! Let's clarify our intentions.You've correctly updated the function name here. Good attention to detail! Now, let's think about the comment. How can we make it clearer for our future selves or other developers?
Consider updating the comment to be more specific:
jsEditor.EnableDisableAsyncFuncSettings("callAppsmithTed", false); // Async execution disabled; will be enabled when mapped to widgetThis clarifies exactly what's happening and why. What do you think about this change?
79-83: Great job updating the widget and assertions! Let's polish it up a bit more.You've done well updating the Input1 default value and the toast message assertion. These changes are consistent with our move to the TED API. However, let's think about how we can make our code even better.
For the default value, consider using template literals for better readability:
`{{${jsObjName}.callAppsmithTed.data}}`For the assertion, let's make it more specific:
cy.get(locators._toastMsg) .should("contain", "AppsmithTed") .and("contain", jsName) .and("contain", "will be executed automatically on page load");What do you think about these changes? How do they improve our code?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JsOnLoad3_Spec.ts (3 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JsOnLoad3_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 (2)
app/client/cypress/limited-tests.txt (2)
Line range hint
1-8: Class, let's review the importance of our instructions!Now, children, I want you all to pay close attention to these instructions in our
limited-tests.txtfile. They're like the rules of our classroom - very important!Remember:
- We use this file to run a limited set of tests.
- If you want to run all the tests, you need to uncomment that special line.
- The
ci-test-limitcommand relies on this file, so we must keep it neat and tidy.Let's all say it together: "We won't run the entire test suite with
ci-test-limit!" Very good!
2-2: Well done, class! This change looks good.I'm pleased to see you've updated the test specification to focus on server-side testing. This aligns nicely with our lesson plan of replacing the 3rd party API with TED. Remember, children, when we make changes like this, it's important to ensure we're not leaving any gaps in our test coverage.
Let's do a quick pop quiz to make sure we haven't forgotten anything:
✅ Verification successful
Excellent work! The changes have been successfully verified.
You've correctly removed the old client-side test and added the new server-side test in the appropriate location. This ensures our test suite remains up-to-date and aligned with our project goals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the old test file still exists and if the new test file is present echo "Question 1: Is the old test file still in our project?" fd --type f "Fork_Template_spec.js" || echo "Correct! The old file is no longer present." echo "Question 2: Is our new test file where it should be?" fd --type f "JsOnLoad3_Spec.ts" || echo "Oh dear, we might need to double-check our new file's location."Length of output: 376
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11081524333. |
reverted limited run file
replace 3rd party API with TED
/ok-to-test tags="@tag.JS"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11102666520
Commit: 06354a9
Cypress dashboard.
Tags:
@tag.JSSpec:
Mon, 30 Sep 2024 09:26:01 UTC