chore: fetch spec with removed third party #36636
Conversation
WalkthroughThe changes in this pull request primarily involve enhancements to the Cypress test suite, particularly focusing on server-side JavaScript function execution. Modifications include updating fetch call tests to ensure cookies are not sent, introducing delays in fetch calls, and altering button functionality to fetch user names instead of IDs. Additionally, the test specification file has been updated to reflect these changes, shifting the focus from client-side to server-side testing. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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
the
skip-changelog
|
/ci-test-count-limit run_count=10 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/limited-tests.txt (1)
Line range hint
3-6: Excellent comments, class! Let's review them together.These comments are like good instructions in your textbook. They clearly explain how to run all specs and the purpose of this file. Well done!
However, to make it even clearer for everyone, especially new students joining our class, we could add a bit more detail. What do you think about adding a brief explanation of what 'ci-test-limit' means? It's always good to spell out abbreviations!
Here's a suggestion to improve the last comment:
-#ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command. +# The 'ci-test-limit' (Continuous Integration Test Limit) command uses this file to run a minimum set of specs. +# Do not run the entire test suite with this command, as it's designed for quick, limited testing in the CI pipeline.What do you think, class? Does this make it clearer for everyone?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: You might be missing the article “the” here.
Context: ... limited tests - give the spec names in below format: cypress/e2e/Regression/ServerSi...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (1 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_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 (1)
2-2: Class, let's examine this change in our test specification!Now, pay attention to how we've shifted our focus from client-side template testing to server-side JavaScript function execution. This is quite a significant change, isn't it? We're moving from
Fork_Template_spec.jstoFetch_Spec.ts.Can anyone tell me what the change in file extension from
.jsto.tsmight indicate? That's right! It suggests we're now using TypeScript, which gives us stronger typing. This is like using a more precise ruler in our measurements!This change aligns well with our lesson plan of updating third-party URLs and improving our testing process. Remember, in software development, we're always looking to refine and improve our methods!
Let's do a quick exercise to verify this change:
Class, what do you think we'll find when we run this?
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (1)
81-81: Class, let's discuss the button label change.Good job updating the button label to "getUserName"! This change accurately reflects the new functionality of fetching a user's name instead of their ID. Remember, clear and descriptive labels help improve the user experience and make our code more self-explanatory.
| `{{fetch('http://host.docker.internal:5001/v1/genderize_agify?name=sagar') | ||
| .then(res => { | ||
| if (!res.ok) { | ||
| throw new Error('Network response was not ok'); | ||
| } | ||
| return res.json(); | ||
| }) | ||
| .then(json => { | ||
| const name = json.name; // Get the name | ||
| showAlert("Name: " + name); | ||
| }) | ||
| .catch(error => { | ||
| console.error("Fetch error:", error); | ||
| showAlert("Failed to fetch user data: " + error.message); | ||
| })}}` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Excellent work on improving the fetch call, but let's refine it further.
I'm impressed with the improvements you've made to the fetch call. The error handling is a great addition, showing good attention to potential issues. However, we need to make a few adjustments:
- Instead of using
showAlert, let's use Cypress commands for assertions. This will make our tests more robust and easier to debug. - We should avoid hardcoding URLs in our tests. Consider using environment variables or configuration files for the API endpoint.
Here's how we could improve this:
cy.intercept('GET', '**/v1/genderize_agify*').as('getUserName');
cy.get('[data-cy=getUserName]').click();
cy.wait('@getUserName').then((interception) => {
expect(interception.response.statusCode).to.equal(200);
expect(interception.response.body).to.have.property('name', 'sagar');
});This approach uses Cypress commands for assertions and network interception, making our test more reliable and easier to maintain.
| agHelper.ClickButton("getUserName"); | ||
| agHelper.AssertContains("Name: sagar", "exist"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Let's improve our button selection and assertion, class.
While clicking the button and asserting the result is good, we can make it even better:
-
Instead of using
agHelper.ClickButton("getUserName"), let's use a data attribute for more reliable selection:cy.get('[data-cy=getUserName]').click();
-
For the assertion, let's use Cypress commands instead of
agHelper.AssertContains:cy.contains('Name: sagar').should('be.visible');
These changes will make our test more robust and easier to maintain. Remember, in Cypress, we always strive for clear, reliable selectors and assertions!
|
/ci-test-count-limit run_count=10 |
|
/ci-test-limit-count run_count=10 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11144425433. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11144425433. |
|
/ci-test-limit-count run_count=10 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11155403098. |
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/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_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 (1)
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (1)
84-106: Excellent implementation of error handling and retries infetchDataDear students, you've done a commendable job incorporating error handling and a retry mechanism within your
fetchDatafunction. This demonstrates a solid understanding of asynchronous operations and resilience in network requests.
| ); | ||
|
|
There was a problem hiding this comment.
Avoid using agHelper.Sleep(), utilize proper synchronization methods
Class, it's important to remember that using agHelper.Sleep() is discouraged in our testing code. Relying on arbitrary sleep durations can lead to flaky tests and inefficient execution.
Please consider removing agHelper.Sleep(2000); and instead use Cypress's built-in commands for synchronization. For instance, you can use cy.wait() with aliases to wait for specific network calls or cy.get() to wait for elements to appear before interacting with them.
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11155403098. |
|
/ci-test-limit-count run_count=10 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11155992956. |
|
/ci-test-limit-count run_count=10 runId=11155992956 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11156317043. |
|
/ci-test-limit-count run_count=10 runId=11155992956 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11156361022. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11155992956. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11156361022. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (2)
Line range hint
15-40: Class, let's improve our test assertions and avoid sleeps!While your test structure is good, we need to make a few improvements:
- Instead of using
showAlertfor assertions, let's use Cypress commands. This will make our tests more robust and easier to debug. Here's how we can refactor this:cy.window().its('store').invoke('dispatch', { type: 'FETCH_USER_DETAILS' }).then((response) => { expect(response.username).to.equal('anonymousUser'); });
- We should avoid using
agHelper.Sleep(2000). In Cypress, we want our tests to be deterministic. Instead, let's use Cypress's built-in waiting mechanisms:cy.get('@jsObject').should('exist');Remember, class, in Cypress, we always strive for clear, reliable selectors and assertions!
Line range hint
41-76: Let's refine our testing approach, class!Your test structure for checking fetch with setTimeout is on the right track, but we can make it even better:
- Instead of using
agHelper.Sleep(3000), let's utilize Cypress's built-in waiting mechanisms. We can intercept the network request and wait for it:cy.intercept('GET', '/api/v1/users/me').as('getUser'); jsEditor.SelectFunctionDropdown("invoker"); jsEditor.RunJSObj(); cy.wait('@getUser');
- For our assertion, instead of using
agHelper.AssertContains, let's use Cypress commands:cy.contains('anonymousUser').should('be.visible');Remember, in Cypress, we always aim for deterministic tests that don't rely on arbitrary timeouts. Keep up the good work, and let's make our tests even more robust!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_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.
📓 Learnings (1)
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts (4)
Learnt from: sagar-qa007 PR: appsmithorg/appsmith#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56 Timestamp: 2024-07-16T06:44:55.263Z Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep` in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.Learnt from: sagar-qa007 PR: appsmithorg/appsmith#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33 Timestamp: 2024-07-16T08:25:23.799Z Learning: Avoid using sleep functions like `agHelper.Sleep` and `this.Sleep` in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.Learnt from: sagar-qa007 PR: appsmithorg/appsmith#35412 File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99 Timestamp: 2024-08-06T05:59:19.000Z Learning: In Cypress tests within the `app/client/cypress` directory, avoid using `agHelper.Sleep`, `this.Sleep`, and other related sleep functions to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable. Instead, use Cypress methods like `cy.get()`, `cy.contains()`, and `cy.intercept()` to wait for specific conditions.Learnt from: ApekshaBhosale PR: appsmithorg/appsmith#35412 File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99 Timestamp: 2024-08-06T05:39:47.929Z Learning: In Cypress tests, avoid using `agHelper.Sleep` as it can lead to flaky tests and unnecessary delays. Instead, use Cypress methods like `cy.get()`, `cy.contains()`, and `cy.intercept()` to wait for specific conditions.
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts
Show resolved
Hide resolved
Description
Updated third party URLs.
Fixes #
36437Automation
/ok-to-test tags="@tag.JS"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11157028440
Commit: 3a3ab54
Cypress dashboard.
Tags:
@tag.JSSpec:
Thu, 03 Oct 2024 07:04:14 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation