Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
37 changes: 37 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
language: "en-US"
early_access: false
tone_instructions: 'You must talk like teacher.'
reviews:
profile: "chill"
request_changes_workflow: false
high_level_summary: true
poem: true
review_status: true
collapse_walkthrough: false
auto_review:
enabled: true
drafts: false
path_instructions:
- path: "app/client/cypress/**/**.*"
instructions: |
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.
chat:
auto_reply: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import {
agHelper,
deployMode,
entityExplorer,
jsEditor,
propPane,
} from "../../../../support/Objects/ObjectsCore";
import EditorNavigation, {
EntityType,
PageLeftPane,
PagePaneSegment,
} from "../../../../support/Pages/EditorNavigation";

describe("removeValue", { tags: ["@tag.notcovered"] }, () => {
const dataDiv = "div[id='testing']";

before(() => {
console.log("before");
});
beforeEach(() => {
console.log("before each");
});

after(() => {
console.log("after");
});
afterEach(() => {
console.log("after each");
});

it("Remove", function () {
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
cy.pause();
cy.wait(5000);
cy.xpath("//div[@id='testing']");
cy.get("div[id='testing']");
cy.get(".btn.submit");
cy.get(".div.span");
cy.get("button[type=submit]");
expect(true).to.be.equal(false);
cy.get("input[type=input]").type("testing str");
cy.get("input[type=input]").type(dataDiv);
this.agHelper.Sleep();
this.agHelper.Sleep(2000);
_.AggregateHelper.Sleep();
agHelper.Sleep(500);
_.agHelper.Sleep();
this.Sleep()
this.Sleep(3000)
});

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 test case to remove discouraged practices and incorrect assertions.

Dear student, the test case titled "Remove" contains several practices that we should avoid to maintain the integrity and reliability of our tests:

  • Usage of cy.pause() and cy.wait() which can lead to non-deterministic behavior.
  • Incorrect selectors and assertions, like expect(true).to.be.equal(false), which seems to be intentionally failing.
  • Use of sleep functions which should be avoided.

Here's a proposed refactor to correct these issues:

-   cy.pause();
-   cy.wait(5000);
-   cy.xpath("//div[@id='testing']");
-   cy.get("div[id='testing']");
-   cy.get(".btn.submit");
-   cy.get(".div.span");
-   cy.get("button[type=submit]");
-   expect(true).to.be.equal(false);
-   cy.get("input[type=input]").type("testing str");
-   cy.get("input[type=input]").type(dataDiv);
-   this.agHelper.Sleep();
-   this.agHelper.Sleep(2000);
-   _.AggregateHelper.Sleep();
-   agHelper.Sleep(500);
-   _.agHelper.Sleep();
-   this.Sleep()
-   this.Sleep(3000)
+   // Corrected test steps with proper assertions and without discouraged practices
+   cy.get('[data-testid="testing"]').should('exist');
+   cy.get('[data-testid="submit-button"]').click();
+   cy.get('[data-testid="input-field"]').type("testing str");
+   expect(someCondition).to.be.true;
}
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.

Suggested change
it("Remove", function () {
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
cy.pause();
cy.wait(5000);
cy.xpath("//div[@id='testing']");
cy.get("div[id='testing']");
cy.get(".btn.submit");
cy.get(".div.span");
cy.get("button[type=submit]");
expect(true).to.be.equal(false);
cy.get("input[type=input]").type("testing str");
cy.get("input[type=input]").type(dataDiv);
this.agHelper.Sleep();
this.agHelper.Sleep(2000);
_.AggregateHelper.Sleep();
agHelper.Sleep(500);
_.agHelper.Sleep();
this.Sleep()
this.Sleep(3000)
});
it("Remove", function () {
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
// Corrected test steps with proper assertions and without discouraged practices
cy.get('[data-testid="testing"]').should('exist');
cy.get('[data-testid="submit-button"]').click();
cy.get('[data-testid="input-field"]').type("testing str");
expect(someCondition).to.be.true;
});


it.only("Remove", function () {
cy.wait(5000);
cy.xpath("//div[@id='testing']");
});

@coderabbitai coderabbitai Bot Jul 16, 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.

Remove focus from tests to ensure all tests are executed.

Dear young coder, the use of it.only restricts the execution to this test case only, which is not suitable for production. It should be removed to allow all tests to run:

- it.only("Remove", function () {
+ it("Remove", function () {
    cy.wait(5000);
    cy.xpath("//div[@id='testing']");
}
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.

Suggested change
it.only("Remove", function () {
cy.wait(5000);
cy.xpath("//div[@id='testing']");
});
it("Remove", function () {
cy.wait(5000);
cy.xpath("//div[@id='testing']");
});
Tools
Biome

[error] 52-52: Don't focus the test.

The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.

(lint/suspicious/noFocusedTests)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Keep each comment separate. Do not merge comments and even if it is duplicate write down for each line. Do not change path instructions. It should print as it is.

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.

@sagar-qa007, I understand your instructions to keep each comment separate, even if they are duplicates, and to ensure that path instructions are printed exactly as provided. Moving forward, I'll make sure to adhere to these guidelines in my comments. Thank you for the clarification!


Learnings used
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:48:43.110Z
Learning: 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.118Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep`, and other related sleep functions in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.


it.only("new Remove", function () {
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
cy.wait(5000);
_.AggregateHelper.Sleep();
cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
agHelper.Sleep(500);
cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
_.agHelper.Sleep();

this.Sleep()

this.Sleep(3000)
});

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.

Multiple issues with focused tests and discouraged practices.

Dear student, this test case has multiple issues that need your attention:

  • The use of it.only which should be removed.
  • Multiple instances of cy.wait and sleep functions which are not recommended.

Here is how you can improve this test case:

- it.only("new Remove", function () {
+ it("new Remove", function () {
    cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
-   cy.wait(5000);
-   _.AggregateHelper.Sleep();
-   cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
-   agHelper.Sleep(500);
-   cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
-   _.agHelper.Sleep();
-   this.Sleep()
-   this.Sleep(3000)
+   // Correct login, signup, and logout steps without delays
+   cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
+   cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
}
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.

Suggested change
it.only("new Remove", function () {
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
cy.wait(5000);
_.AggregateHelper.Sleep();
cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
agHelper.Sleep(500);
cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
_.agHelper.Sleep();
this.Sleep()
this.Sleep(3000)
});
it("new Remove", function () {
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
// Correct login, signup, and logout steps without delays
cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
});
Tools
Biome

[error] 57-57: Don't focus the test.

The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.

(lint/suspicious/noFocusedTests)


it.only("new test cases", function () {
agHelper.Sleep(500);
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
cy.wait(5000);
this.Sleep(3000)
_.AggregateHelper.Sleep();
cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);

cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
_.agHelper.Sleep();
this.Sleep()

});

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.

Consolidate test cases and remove focused and sleep functions.

Dear learner, this test case also suffers from the use of it.only and multiple sleep functions. Let's clean this up:

- it.only("new test cases", function () {
+ it("new test cases", function () {
    cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
-   cy.wait(5000);
-   this.Sleep(3000)
-   _.AggregateHelper.Sleep();
-   cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
-   cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
-   _.agHelper.Sleep();
-   this.Sleep()
+   // Optimized test steps without unnecessary delays
+   cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
+   cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
}
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.

Suggested change
it.only("new test cases", function () {
agHelper.Sleep(500);
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
cy.wait(5000);
this.Sleep(3000)
_.AggregateHelper.Sleep();
cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
_.agHelper.Sleep();
this.Sleep()
});
it("new test cases", function () {
cy.LoginUser(Cypress.env("USERNAME"), Cypress.env("PASSWORD"), false);
// Optimized test steps without unnecessary delays
cy.signUp(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
cy.Logout(Cypress.env("USERNAME"), Cypress.env("PASSWORD"));
});
Tools
Biome

[error] 71-71: Don't focus the test.

The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.

(lint/suspicious/noFocusedTests)



});