Skip to content

chore: fetch spec with removed third party #36636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sagar-qa007 merged 9 commits into from
Oct 3, 2024
Merged

chore: fetch spec with removed third party #36636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4b38812
Updated fetch spec
sagar-qa007 Oct 1, 2024
38b475b
prettier update
sagar-qa007 Oct 1, 2024
7180da3
Adding a line
sagar-qa007 Oct 2, 2024
0ab5d31
reverted changes for prettier
sagar-qa007 Oct 2, 2024
20018d4
Updated spec with retry
sagar-qa007 Oct 3, 2024
a49d01f
updated flow to remove flakyness
sagar-qa007 Oct 3, 2024
d71687c
Run api
sagar-qa007 Oct 3, 2024
c9a584a
reverted the change
sagar-qa007 Oct 3, 2024
3a3ab54
removed comment
sagar-qa007 Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,28 @@ describe("Tests fetch calls", { tags: ["@tag.JS"] }, () => {
it("3. Tests if fetch works with store value", function () {
entityExplorer.DragDropWidgetNVerify("buttonwidget", 500, 200);
EditorNavigation.SelectEntityByName("Button1", EntityType.Widget);
propPane.TypeTextIntoField("Label", "getUserID");
propPane.TypeTextIntoField("Label", "getUserName");
propPane.EnterJSContext(
"onClick",
`{{fetch('https://jsonplaceholder.typicode.com/todos/1')
.then(res => res.json())
.then(json => storeValue('userId', json.userId))
.then(() => showAlert("UserId: " + appsmith.store.userId))}}`,
`{{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);
})}}`

@coderabbitai coderabbitai Bot Oct 1, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Instead of using showAlert, let's use Cypress commands for assertions. This will make our tests more robust and easier to debug.
  2. 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.

);

@coderabbitai coderabbitai Bot Oct 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

agHelper.Sleep(2000);
agHelper.ClickButton("getUserID");
agHelper.AssertContains("UserId: 1", "exist");
agHelper.ClickButton("getUserName");
agHelper.AssertContains("Name: sagar", "exist");

@coderabbitai coderabbitai Bot Oct 1, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Instead of using agHelper.ClickButton("getUserName"), let's use a data attribute for more reliable selection:

    cy.get('[data-cy=getUserName]').click();

  2. 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!

});
});
2 changes: 1 addition & 1 deletion app/client/cypress/limited-tests.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# To run only limited tests - give the spec names in below format:
cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js
cypress/e2e/Regression/ServerSide/JsFunctionExecution/Fetch_Spec.ts

# For running all specs - uncomment below:
#cypress/e2e/**/**/*
Expand Down