test: adding tests for jsobject regression tests #38214
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThis pull request introduces a comprehensive test suite for JavaScript Objects (JSObjects) in the Cypress testing framework. The changes include adding a new test specification file Changes
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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=1 |
github-actions
bot
added
skip-changelog
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12379191533. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts (1)
70-77: Remove commented-out test code.Commented-out test code should either be implemented or removed to maintain code cleanliness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)app/client/cypress/support/Pages/EntityExplorer.ts(1 hunks)app/client/cypress/support/Pages/JSEditor.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/cypress/limited-tests.txt
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/support/Pages/EntityExplorer.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/e2e/Regression/ClientSide/JSObject/JSObject_Tests_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/support/Pages/JSEditor.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.
| assertHelper.AssertNetworkStatus("@postExecute", 200); | ||
| agHelper.ClickButton("Got it"); | ||
| assertHelper.AssertNetworkStatus("@updateLayout", 200); | ||
| agHelper.Sleep(2000); |
There was a problem hiding this comment.
Remove explicit sleep calls.
Using agHelper.Sleep() violates the coding guidelines. Use Cypress's built-in retry-ability and assertions instead.
- agHelper.Sleep(2000);
+ agHelper.AssertElementVisibility(table.locators._table);📝 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.
| agHelper.Sleep(2000); | |
| agHelper.AssertElementVisibility(table.locators._table); |
| PageLeftPane.switchSegment(PagePaneSegment.UI); | ||
| entityExplorer.DragDropWidgetNVerify(draggableWidgets.BUTTON, 500, 100); | ||
| propPane.EnterJSContext("onClick", "{{JSObject11.myFun1();}}", true, false); | ||
| agHelper.GetNClick(locators._widgetInDeployed("buttonwidget")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data- attributes for selectors.*
Replace hardcoded widget selector with a data-* attribute as per coding guidelines.
- agHelper.GetNClick(locators._widgetInDeployed("buttonwidget"));
+ agHelper.GetNClick("[data-testid='t--widget-buttonwidget']");Committable suggestion skipped: line range outside the PR's diff.
| @@ -72,6 +72,7 @@ export class EntityExplorer { | |||
| _widgetTagSuggestedWidgets = ".widget-tag-collapsible-suggested"; | |||
| _widgetTagBuildingBlocks = ".widget-tag-collapsible-building-blocks"; | |||
| _widgetSeeMoreButton = "[data-testid='t--explorer-ui-entity-tag-see-more']"; | |||
| _entityName = ".t--entity-name" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reusing existing selector.
The selector .t--entity-name is already used in the code (e.g., in DeleteAllQueriesForDB method). To maintain DRY principles, consider reusing the existing selector reference.
- _entityName = ".t--entity-name"Committable suggestion skipped: line range outside the PR's diff.
| _addJSObj = '[data-testid="t--ide-tabs-add-button"]'; | ||
| _jsPageActions = ".entity-context-menu"; | ||
| _moreActions = '[data-testid="t--more-action-trigger"]'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate selector and follow consistent naming.
The _moreActions selector duplicates the existing contextMenuTriggerLocator property. Both use the same data-testid attribute.
_addJSObj = '[data-testid="t--ide-tabs-add-button"]';
_jsPageActions = ".entity-context-menu";
- _moreActions = '[data-testid="t--more-action-trigger"]';📝 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.
| _addJSObj = '[data-testid="t--ide-tabs-add-button"]'; | |
| _jsPageActions = ".entity-context-menu"; | |
| _moreActions = '[data-testid="t--more-action-trigger"]'; | |
| _addJSObj = '[data-testid="t--ide-tabs-add-button"]'; | |
| _jsPageActions = ".entity-context-menu"; |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12379191533. |
|
/ci-test-limit-count run_count=15 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12386012942. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12386012942. |
|
/ci-test-limit-count run_count=1 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12405640526. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts(1 hunks)app/client/cypress/support/Objects/CommonLocators.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Objects/CommonLocators.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.
| @@ -346,4 +346,5 @@ export class CommonLocators { | |||
| _showBoundary = ".show-boundary"; | |||
| _entityItem = "[data-testid='t--entity-item-Api1']"; | |||
| _rowData = "[data-colindex='0'][data-rowindex='0']"; | |||
| _editorTab = ".editor-tab"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data-testid attribute instead of CSS class selector
According to the coding guidelines, we should use data-* attributes for selectors instead of CSS class selectors.
Replace the CSS class selector with a data-testid attribute:
- _editorTab = ".editor-tab";
+ _editorTab = "[data-testid='t--editor-tab']";This change requires updating the corresponding HTML element to include the data-testid attribute:
<div data-testid="t--editor-tab">...</div>
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12405640526. |
|
/ci-test-limit-count run_count=10 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts (2)
21-22: Empty before() hook should be removed or utilizedThe empty before() hook serves no purpose and should either be removed or implemented with necessary test setup.
- before(() => {});
70-86: Remove commented test codeThe commented code for "Copy to Page" and "Rename" verification should either be implemented or removed. Keeping commented code adds unnecessary complexity and maintenance overhead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_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 (3)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts (3)
272-273: Remove explicit sleep calls
Using agHelper.Sleep() violates the coding guidelines. Use Cypress's built-in retry-ability and assertions instead.
- agHelper.Sleep(2000);
+ agHelper.AssertElementVisibility(table.locators._table);122-122: Use data- attributes for selectors*
Replace hardcoded widget selector with a data-* attribute as per coding guidelines.
- agHelper.GetNClick(locators._widgetInDeployed("buttonwidget"));
+ agHelper.GetNClick("[data-testid='t--widget-buttonwidget']");286-309: Add timeline for addressing skipped tests
The skipped tests reference bugs #35385 and #38216, but there's no indication of when these will be addressed. Consider:
- Adding TODO comments with expected timeline
- Creating GitHub issues if not already existing
- Adding acceptance criteria for when these tests can be unskipped
✅ Verification successful
Both skipped tests are tracking active issues that need resolution
Both referenced issues are still open:
- [Bug]: Creation of indentical name JSObject in a page #35385 (High priority): Creation of identical name JSObject bug
- [Bug]: Navigation between Jsobjects not working properly #38216 (Medium priority): Navigation between JSObjects not working properly
The skipped tests are correctly tracking real issues, with proper bug references and assigned owners. No timeline comments needed as these are being actively worked on.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced issues are still open
gh issue view 35385
gh issue view 38216
Length of output: 2034
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12417485057. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12417485057. |
| agHelper.GetNClickByContains(HomePage.portalMenuItem, "Are you sure?"); | ||
| agHelper.ValidateToastMessage("JSObject4 deleted successfully"); | ||
|
|
||
| // Verify Copy to Page |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
app/client/cypress/support/Pages/HomePage.ts (2)
92-92: Consider keeping _forkModal private and providing a public accessor method instead.The underscore prefix naming convention suggests this should be a private member. Consider keeping it private and adding a public getter method like
getForkModalSelector()to maintain encapsulation while allowing external access.- public _forkModal = ".fork-modal"; + private _forkModal = ".fork-modal"; + public getForkModalSelector() { + return this._forkModal; + }
Line range hint
32-38: Consider using data-testid for fork modal selector.Replace the class selector with a data-testid attribute for better test maintainability and resilience to styling changes.
- public _forkModal = ".fork-modal"; + public _forkModal = "[data-testid=t--fork-modal]";app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts (2)
9-10: Remove unnecessary empty options objectThe empty options object in the describe function can be removed.
-describe("Fork application", {}, function () { +describe("Fork application", function () {
10-25: Add more assertions for robust testingThe test would benefit from additional assertions to verify the fork operation more thoroughly:
- Verify the new application's workspace
- Verify the application's content after forking
Consider adding these assertions after navigation:
// Verify workspace cy.get('[data-testid="t--workspace-name"]').should('be.visible'); // Verify application content cy.get('[data-testid="t--application-content"]') .should('exist') .and('contain', 'JS object testing');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts(1 hunks)app/client/cypress/support/Pages/HomePage.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/support/Pages/HomePage.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/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_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/ClientSide/JSObject/JSObject_ForkApp_spec.ts (1)
1-8: LGTM! Clean imports structure
The imports are well-organized and follow good practices.
| it("1. Fork app and verify", () => { | ||
| homePage.ImportApp("jsObjectTesting.json"); | ||
| agHelper.GetNClick(homePage._applicationName); | ||
| agHelper.GetNClickByContains; |
There was a problem hiding this comment.
Remove incomplete statement
This line appears to be an incomplete function call and serves no purpose.
- agHelper.GetNClickByContains;| ); | ||
| agHelper.GetNClick(locators._forkAppToWorkspaceBtn); | ||
| assertHelper.AssertNetworkStatus("@postForkAppWorkspace", 200); | ||
| agHelper.WaitUntilEleDisappear(homePage._forkModal); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace wait with better assertion
According to the guidelines, we should avoid using wait functions. Instead, use Cypress's built-in retry-ability with assertions.
- agHelper.WaitUntilEleDisappear(homePage._forkModal);
+ cy.get(homePage._forkModal).should('not.exist');📝 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.
| agHelper.WaitUntilEleDisappear(homePage._forkModal); | |
| cy.get(homePage._forkModal).should('not.exist'); |
| agHelper.AssertElementExist( | ||
| `${homePage._applicationCard}:contains('JS object testing upto 1.5 MB (1)')`, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data- attributes for selectors*
Avoid using string selectors in assertions. Instead, use data-* attributes for more reliable element selection.
- agHelper.AssertElementExist(
- `${homePage._applicationCard}:contains('JS object testing upto 1.5 MB (1)')`,
- );
+ cy.get('[data-testid="t--application-card"]')
+ .should('exist')
+ .and('contain', 'JS object testing upto 1.5 MB (1)');📝 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.
| agHelper.AssertElementExist( | |
| `${homePage._applicationCard}:contains('JS object testing upto 1.5 MB (1)')`, | |
| ); | |
| cy.get('[data-testid="t--application-card"]') | |
| .should('exist') | |
| .and('contain', 'JS object testing upto 1.5 MB (1)'); |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
app/client/cypress/support/Objects/CommonLocators.ts (1)
349-349:⚠️ Potential issueUse data-testid attribute instead of CSS class selector
The selector uses a CSS class which violates the coding guidelines for Cypress tests.
Replace the CSS class selector with a data-testid attribute:
- _editorTab = ".editor-tab"; + _editorTab = "[data-testid='t--editor-tab']";app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts (2)
19-19:⚠️ Potential issueRemove incomplete statement
This line appears to be an incomplete function call and serves no purpose.
- agHelper.GetNClickByContains;
27-27: 🛠️ Refactor suggestionReplace wait with better assertion
According to the guidelines, we should avoid using wait functions. Instead, use Cypress's built-in retry-ability with assertions.
- agHelper.WaitUntilEleDisappear(homePage._forkModal); + cy.get(homePage._forkModal).should('not.exist');
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts (1)
45-51: Add assertions for JSObject creationThe JSObject creation lacks proper assertions to verify the success of the operation.
jsEditor.CreateJSObject('"MiddleName": "Test",\n', { paste: false, toRun: false, completeReplace: false, shouldCreateNewJSObj: false, lineNumber: 5, - }); + }); + cy.get('.t--js-editor').should('be.visible'); + cy.get('.t--js-editor').should('contain', 'MiddleName');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)app/client/cypress/support/Objects/CommonLocators.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/limited-tests.txt
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/support/Objects/CommonLocators.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/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_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 (2)
app/client/cypress/support/Objects/CommonLocators.ts (1)
350-351: LGTM! Well-structured selector using data-testid
The method follows best practices by using data-testid attribute for element selection.
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts (1)
1-14: LGTM! Clean imports structure
The imports are well-organized and properly destructured.
| agHelper.AssertClassExists(locators._entityTestId("J16"), "active"); | ||
| agHelper.GetNClick(locators._entityTestId("J17")); | ||
| agHelper.AssertClassExists(locators._entityTestId("J17"), "active"); | ||
| agHelper.GetNAssertContains(".CodeMirror-line ", '"MiddleName": "Test"'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data-testid for CodeMirror assertions
Avoid using class selectors for assertions. Instead, use data-testid attributes.
- agHelper.GetNAssertContains(".CodeMirror-line ", '"MiddleName": "Test"');
+ cy.get('[data-testid="code-editor-line"]').should('contain', '"MiddleName": "Test"');Committable suggestion skipped: line range outside the PR's diff.
| for (let i = 1; i <= 11; i++) { | ||
| agHelper.GetNClick(locators._entityTestId(`JS${i}`)); | ||
| agHelper.FailIfErrorToast(""); | ||
| agHelper.AssertClassExists(locators._entityTestId(`JS${i}`), "active"); | ||
| } | ||
| for (let i = 12; i <= 17; i++) { | ||
| agHelper.GetNClick(locators._entityTestId(`J${i}`)); | ||
| agHelper.FailIfErrorToast(""); | ||
| agHelper.AssertClassExists(locators._entityTestId(`J${i}`), "active"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate loops
The two loops have similar logic and can be combined to reduce code duplication.
- for (let i = 1; i <= 11; i++) {
- agHelper.GetNClick(locators._entityTestId(`JS${i}`));
- agHelper.FailIfErrorToast("");
- agHelper.AssertClassExists(locators._entityTestId(`JS${i}`), "active");
- }
- for (let i = 12; i <= 17; i++) {
- agHelper.GetNClick(locators._entityTestId(`J${i}`));
- agHelper.FailIfErrorToast("");
- agHelper.AssertClassExists(locators._entityTestId(`J${i}`), "active");
- }
+ const entityIds = [
+ ...Array.from({ length: 11 }, (_, i) => `JS${i + 1}`),
+ ...Array.from({ length: 6 }, (_, i) => `J${i + 12}`)
+ ];
+ entityIds.forEach(id => {
+ cy.get(locators._entityTestId(id))
+ .click()
+ .should('have.class', 'active');
+ agHelper.FailIfErrorToast("");
+ });📝 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.
| for (let i = 1; i <= 11; i++) { | |
| agHelper.GetNClick(locators._entityTestId(`JS${i}`)); | |
| agHelper.FailIfErrorToast(""); | |
| agHelper.AssertClassExists(locators._entityTestId(`JS${i}`), "active"); | |
| } | |
| for (let i = 12; i <= 17; i++) { | |
| agHelper.GetNClick(locators._entityTestId(`J${i}`)); | |
| agHelper.FailIfErrorToast(""); | |
| agHelper.AssertClassExists(locators._entityTestId(`J${i}`), "active"); | |
| } | |
| const entityIds = [ | |
| ...Array.from({ length: 11 }, (_, i) => `JS${i + 1}`), | |
| ...Array.from({ length: 6 }, (_, i) => `J${i + 12}`) | |
| ]; | |
| entityIds.forEach(id => { | |
| cy.get(locators._entityTestId(id)) | |
| .click() | |
| .should('have.class', 'active'); | |
| agHelper.FailIfErrorToast(""); | |
| }); |
|
/ci-test-limit-count run_count=1 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12442044213. |
/ok-to-test tags="@tag.All"
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12441999066
Commit: ada8c84
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Anvil/Widgets/AnvilTableWidgetSnapshot_spec.ts
- cypress/e2e/Regression/ClientSide/Binding/TableV2TextPagination_spec.js
- cypress/e2e/Regression/ClientSide/Github/EnableGithub_spec.ts
- cypress/e2e/Regression/ServerSide/QueryPane/AddWidgetTableAndBind_spec.js
List of identified flaky tests.Sat, 21 Dec 2024 05:20:23 UTC
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor
EntityExplorer,JSEditor, andCommonLocatorsclasses to enhance functionality and UI management.