Skip to content
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.

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

test: new test cases framework functions and Action selector #37566

Merged
merged 59 commits into from
Dec 16, 2024

Conversation

sagar-qa007
Copy link
Contributor

@sagar-qa007 sagar-qa007 commented Nov 19, 2024

Description

E2E Test cases for framework function & Action selector.

Fixes Issue URL

Automation

/ok-to-test tags="@tag.All"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12343756587
Commit: 56f7a2e
Cypress dashboard.
Tags: @tag.All
Spec:


Mon, 16 Dec 2024 03:08:25 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Enhanced property pane with new button widget identification.
    • New selectors for action cards and toast notifications.
    • Improved modal interaction with new properties for buttons.
    • Added functionality for toggling JavaScript execution on page load.
    • New methods for character manipulation and console log management.
    • Comprehensive application configurations for interval management.
    • New applications and widgets for managing intervals and clipboard functionality.
    • New functionality for resetting widgets and handling navigation actions.
    • Introduction of new JSON configurations for various applications.
  • Bug Fixes

    • Improved error handling for various actions and functions.
  • Tests

    • Extensive test coverage for new and existing functionalities, including action selectors, interval functions, alert handling, and geolocation features.
    • New tests for clipboard functionality, download actions, and local storage operations.
    • Enhanced tests for Radio and CategorySlider widgets to improve validation of JavaScript context and error handling.

Copy link
Contributor

coderabbitai bot commented Nov 19, 2024

Walkthrough

The changes in this pull request encompass updates across multiple files, primarily enhancing the functionality and test coverage of various components within the application. Key modifications include the introduction of new methods and properties in the PropertyPane, CommonLocators, and ApiPage classes, as well as the addition of several test cases for the clearStore, setInterval, and clearInterval functionalities. New JSON configuration files for applications and UI components are also introduced, providing structured representations of their properties and behaviors.

Changes

File Change Summary
app/client/cypress/support/Pages/PropertyPane.ts - Added private variable _buttonWidget for button identification.
- Added public method _getActionCardSelector(type: string) for dynamic selector generation.
- Added public method `ToggleJSModeByIndex(endp: string, toToggleOnJS: true
app/client/cypress/locators/commonlocators.json - Added locators for tostifyIcon and downloadFileType.
- Locator for fixedFooterInput remains unchanged.
app/client/cypress/support/Objects/CommonLocators.ts - Added properties _modalButtonText and _showBoundary for modal and boundary element selection.
app/client/cypress/support/Pages/ApiPage.ts - Added private property runOnPageLoadJSObject.
- Added public methods ToggleOnPageLoadRunJsObject(enable: boolean) and clickSettingIcon(enable: boolean).
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts - Added four new test cases for clearStore functionality with various assertions.
app/client/cypress/support/Pages/AggregateHelper.ts - Added method RemoveChars for character manipulation.
- Added console log management methods: captureConsoleLogs, verifyConsoleLogNotContainingError, verifyConsoleLogContainsExpectedMessage, clearConsoleLogs.
app/client/cypress/support/Pages/JSEditor.ts - Changed visibility of _jsObjName from private to public.
- Added method currentJSObjectName().
app/client/cypress/fixtures/setIntervalApp.json - Added configuration for setIntervalApp with multiple button widgets and JavaScript functions.
app/client/cypress/fixtures/clearIntervalApp.json - Introduced configuration for managing intervals with buttons and JavaScript functions.
app/client/cypress/fixtures/ActionSelectorAppNewExported.json - Defined structure for ActionSelectorAppNew (2) with metadata and page details.
app/client/cypress/fixtures/PartialImportExport/frameworkFunPartialPage.json - Added structured representation for a button widget with various properties.
app/client/cypress/support/Pages/PartialImportExport.ts - Added method OpenImportModalWithPage(pageName: string) for importing specific pages.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_ActionSelector_General_spec.ts - Introduced tests for verifying action selector functionalities.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_ClearIntervalFunctions_spec.ts - Added tests for verifying clearInterval functionality under various conditions.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_CopyToClipboardFunctions_spec.ts - Added tests for validating "Copy to clipboard" functionality.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_DownloadFunctions_spec.ts - Added tests for verifying download functionalities.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_LocalRemoveValueFunctions_spec.ts - Added tests for verifying removeValue() functionality in local storage.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_LocalStoreValueFunctions_spec.ts - Added tests for verifying storeValue() functionality in local storage.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_NavigateToFunctions_spec.ts - Added tests for verifying navigation functionalities.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_PostWindowFunctions_spec.ts - Added tests for verifying postWindowMessage() functionality.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_ResetWidgetFunctions_spec.ts - Added tests for verifying resetWidget functionality.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_SetIntervalFunctions_spec.ts - Added tests for verifying setInterval functionality.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_ShowAlertFunctions_spec.ts - Added tests for verifying "Show Alert" action.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_ShowCloseModalFunctions_spec.ts - Added tests for verifying "Show Modal" functionality.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_StopWatchGeoLocationFunctions_spec.ts - Added tests for verifying StopWatchGeoLocation functionality.
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_WatchGeoLocationFunctions_spec.ts - Added tests for verifying "Watch GeoLocation" functionality.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Radio/Radio2_spec.ts - Enhanced validation error handling for Radio Widget options.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Sliders/CategorySlider_spec.ts - Updated JavaScript mode toggling for the "options" property in multiple test cases.

Suggested reviewers

  • ApekshaBhosale
  • NandanAnantharamu

🎉 In the land of code where changes flow,
New features sprout, and tests start to grow.
With buttons and locators, oh what a sight,
Enhancing our app, making it bright!
So here's to the coders, both near and far,
Your work is a treasure, our shining star! 🌟


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fd8d5e2 and 56f7a2e.

📒 Files selected for processing (3)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/Radio/Radio2_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/Sliders/CategorySlider_spec.ts (3 hunks)
  • app/client/cypress/support/Pages/PropertyPane.ts (2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/Radio/Radio2_spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/Sliders/CategorySlider_spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/support/Pages/PropertyPane.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/Widgets/Radio/Radio2_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/e2e/Regression/ClientSide/Widgets/Sliders/CategorySlider_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/ClientSide/Widgets/Radio/Radio2_spec.ts (1)
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Radio/Radio2_spec.ts:404-407
Timestamp: 2024-11-12T08:11:36.416Z
Learning: The `Radio2_spec.ts` file is not part of the initial phase for the removal of static waits in the pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1".
🔇 Additional comments (3)
app/client/cypress/support/Pages/PropertyPane.ts (3)

710-729: LGTM! Well-implemented toggle functionality.

The method properly handles JS mode toggling with index support, making it more flexible for multiple toggles.


181-181: 🛠️ Refactor suggestion

Make the selector more flexible and parameterized.

The hardcoded "Button1" in the selector makes it less reusable.

-_buttonWidget = "[data-widgetname-cy='Button1']";
+_buttonWidgetSelector = (name: string) => `[data-widgetname-cy='${name}']`;

Likely invalid or redundant comment.


182-183: 🛠️ Refactor suggestion

Remove duplicate selector method.

This method is redundant with _actionCardByTitle defined at line 40.

-_getActionCardSelector = (type: string) =>
-  `[data-testid='action-card-Show ${type}']`;

Use the existing _actionCardByTitle method instead.

Likely invalid or redundant comment.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sagar-qa007
Copy link
Contributor Author

/ci-test-limit-count run_count=15

Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
app/client/cypress/limited-tests.txt (1)

Line range hint 1-6: Consider adding a version control comment

Since this file controls CI test execution, consider adding a comment with the last modification date and the reason for changes to help track test suite evolution.

 # To run only limited tests - give the spec names in below format:
+# Last updated: November 2024 - Replaced template tests with action execution tests
 cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts
 # For running all specs - uncomment below:
 #cypress/e2e/**/**/*
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7612e4f and a88888a.

📒 Files selected for processing (2)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts (1 hunks)
  • app/client/cypress/limited-tests.txt (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_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 (1)
app/client/cypress/limited-tests.txt (1)

2-2: Verify the new test file exists and follows best practices

Let's ensure the new test file exists and follows our Cypress coding guidelines.

✅ Verification successful

Test file verified and follows best practices

The new test file exists and follows Cypress best practices:

  • Uses page object model with proper imports
  • No prohibited practices like cy.wait or it.only
  • Uses proper test organization with describe and it blocks
  • Implements proper navigation testing with URL assertions
  • Uses data-driven selectors instead of brittle selectors

The old test file Fork_Template_spec.js is not referenced anywhere else in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the new test file exists and verify its contents
if [ -f "app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts" ]; then
    echo "File exists. Checking contents..."
    # Check for prohibited practices
    rg -i "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts"
    # Check for proper selector usage
    rg "\.btn\.|button\[|xpath\=|\.submit" "app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts"
else
    echo "ERROR: Test file not found!"
fi

# Check if the old test file is still referenced elsewhere
rg "Fork_Template_spec\.js" --type txt --type typescript --type javascript

Length of output: 750


Script:

#!/bin/bash
# Check if the new test file exists
if [ -f "app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts" ]; then
    echo "File exists. Checking contents..."
    # Check for prohibited practices with fixed regex
    rg -i "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts"
    # Check for proper selector usage with fixed regex
    rg "\.btn|button\[|xpath=|\.submit" "app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts"
    # Show file contents for manual inspection
    cat "app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts"
else
    echo "ERROR: Test file not found!"
fi

# Search for old test file references with individual file types
rg "Fork_Template_spec\.js" --type js
rg "Fork_Template_spec\.js" --type ts
rg "Fork_Template_spec\.js" -g "*.txt"

Length of output: 10601

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11913100291.
Cypress dashboard url: Click here!
All Cypress tests have passed 🎉🎉🎉

***** Repeat Run Summary ***** Total Tests with repeat: 60 Total Passed: 42 Total Failed: 18 Total Skipped: 0 *****************************

@sagar-qa007
Copy link
Contributor Author

/ci-test-limit-count run_count=15

Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a88888a and 4790898.

📒 Files selected for processing (2)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/PropertyPane.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/PropertyPane.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.

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11928726909.
Cypress dashboard url: Click here!
All Cypress tests have passed 🎉🎉🎉

***** Repeat Run Summary ***** Total Tests with repeat: 60 Total Passed: 58 Total Failed: 2 Total Skipped: 0 *****************************

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowAlertFunctions_spec.ts (3)

24-24: Remove or clarify the TODO comment

The comment "Please check requirement for this test case" should either be removed or replaced with actual requirements documentation.


25-62: Optimize test execution flow

Consider the following improvements:

  1. The deploy-test-navigate back pattern is repeated, consider extracting it into a helper function
  2. Replace element visibility checks with more specific assertions
- deployMode.DeployApp();
- agHelper.AssertElementVisibility(appSettings.locators._header);
- agHelper.ClickButton("Submit");
- agHelper.AssertElementVisibility(locators._toastMsg);
- deployMode.NavigateBacktoEditor();
+ // Extract to helper
+ function testInDeployMode(assertion: () => void) {
+   deployMode.DeployApp();
+   assertion();
+   deployMode.NavigateBacktoEditor();
+ }

120-125: Strengthen toast message assertions

Consider adding more robust assertions for toast messages:

  1. Check for message presence and content in a single assertion
  2. Add timeout configurations for toast visibility
// Add helper function
function assertToast(message: string, type: string) {
  cy.get(commonlocators.tostifyIcon)
    .should('be.visible')
    .and('have.attr', 'type', type)
    .parent()
    .should('contain.text', message);
}

Also applies to: 184-189, 248-253, 312-317

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4790898 and cc8200d.

📒 Files selected for processing (2)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowAlertFunctions_spec.ts (1 hunks)
  • app/client/cypress/locators/commonlocators.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowAlertFunctions_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/locators/commonlocators.json (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/locators/commonlocators.json (1)

245-245: LGTM!

The locator follows the established pattern using data-* attributes.

app/client/cypress/locators/commonlocators.json Outdated Show resolved Hide resolved
@sagar-qa007
Copy link
Contributor Author

/ci-test-limit-count run_count=10

Copy link

Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11947275309.
Cypress dashboard url: Click here!
All Cypress tests have passed 🎉🎉🎉

***** Repeat Run Summary ***** Total Tests with repeat: 70 Total Passed: 69 Total Failed: 1 Total Skipped: 0 *****************************

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (4)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowCloseModalFunctions_spec.ts (3)

25-62: Consider replacing page refresh with a more deterministic approach

The test uses agHelper.RefreshPage() which might introduce unnecessary waiting. Consider using Cypress's built-in mechanisms to wait for specific conditions instead of refreshing the page.

- agHelper.RefreshPage();
+ // Wait for specific condition instead
+ agHelper.WaitUntilAllToastsDisappear();
+ agHelper.WaitUntilEleAppear(locators._modalWrapper);

71-108: Consider extracting repeated deployment validation into a helper function

The deployment and validation steps are duplicated. Consider extracting them into a reusable helper function.

// Suggested helper function:
const validateInDeployedMode = (action: () => void) => {
  deployMode.DeployApp();
  agHelper.AssertElementVisibility(appSettings.locators._header);
  action();
  deployMode.NavigateBacktoEditor();
};

1-163: Overall: Good test coverage with room for improvement

The tests thoroughly cover modal functionality scenarios. However, there are several areas for improvement:

  1. Avoid direct Cypress commands in favor of helper methods
  2. Reduce code duplication in deployment and JS object creation
  3. Consider more deterministic waiting strategies

The test structure and assertions are solid, but implementing the suggested refactors would improve maintainability.

app/client/cypress/support/Objects/CommonLocators.ts (1)

345-345: LGTM with a suggestion for the modal button locator.

The modal button locator uses data-testid which is good, but consider making it more specific by using a dedicated data-testid for the button itself.

-  _modalButtonText = "[data-testid='modal-wrapper'] .bp3-button";
+  _modalButtonText = "[data-testid='modal-button']";
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ba6a61d and 59e9362.

📒 Files selected for processing (5)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowAlertFunctions_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowCloseModalFunctions_spec.ts (1 hunks)
  • app/client/cypress/support/Objects/CommonLocators.ts (1 hunks)
  • app/client/cypress/support/Pages/ApiPage.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_NavigateToFunctions_spec.ts
🧰 Additional context used
📓 Path-based instructions (4)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowAlertFunctions_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/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowCloseModalFunctions_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/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/support/Pages/ApiPage.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 (5)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowCloseModalFunctions_spec.ts (1)

1-24: LGTM: Clean imports and minimal setup

The imports are well-organized and the setup is appropriately scoped.

app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_ShowAlertFunctions_spec.ts (3)

1-23: LGTM! Clean setup with proper imports.

The test setup is minimal and focused on the required functionality.


24-24: Please clarify the test case requirements.

The comment suggests uncertainty about the test requirements. This should be resolved before proceeding.

Would you like me to help create a GitHub issue to track the requirements clarification?


64-107: Consider previous parameterization suggestion.

This test case could benefit from the parameterization approach suggested in the previous review.

app/client/cypress/support/Pages/ApiPage.ts (1)

99-100: LGTM!

The selector is well-defined and follows the pattern used in other similar properties in the class.

@sagar-qa007
Copy link
Contributor Author

/ci-test-limit-count run_count=10

Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (6)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts (1)

82-86: Refactor to reduce code duplication

Multiple test cases have similar setup and teardown code. Abstract common code into helper functions to improve maintainability.

Also applies to: 115-137, 150-155, 180-187, 212-238, 264-287, 312-316

app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_LocalStoreValueFunctions_spec.ts (1)

39-39: Avoid using strings in assertions

In several assertions, you're using string literals directly, such as in agHelper.AssertText(locators._textInside, "text", "true"). As per the coding guidelines, avoid using strings for assertions. Consider using variables or constants instead.

Also applies to: 46-46, 69-69, 74-74, 92-92, 140-140, 190-190, 196-196, 220-220, 224-224, 253-253, 260-260, 266-266, 267-267, 290-290, 291-291, 296-296, 297-297

app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_LocalRemoveValueFunctions_spec.ts (4)

36-36: Avoid hardcoded strings in assertions

Replace hardcoded expected values in assertions with constants or variables to enhance maintainability and clarity.

Also applies to: 53-53, 99-99, 106-106, 135-135, 164-164, 227-227, 275-275, 293-293, 365-365, 369-369, 448-448, 506-506, 517-517, 555-555, 562-562, 607-607, 614-614


15-17: Update test suite description for accuracy

The description mentions "Local store value function," but the tests focus on removeValue(). Update the description to reflect the tests accurately.


401-401: Ensure unique and descriptive test case titles

Test case "5" title is similar to test "4". Provide unique titles for each test to avoid confusion and improve readability.


243-264: Refactor repeated code for storing values

The code for storing values is duplicated in multiple tests. Consider creating helper functions to reduce repetition and improve maintainability.

Also applies to: 403-424

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 59e9362 and 9906ca7.

📒 Files selected for processing (6)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_LocalRemoveValueFunctions_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_LocalStoreValueFunctions_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts (1 hunks)
  • app/client/cypress/limited-tests.txt (1 hunks)
  • app/client/cypress/locators/HomePage.js (2 hunks)
  • app/client/cypress/support/Pages/AggregateHelper.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 (5)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_LocalRemoveValueFunctions_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/e2e/Regression/ClientSide/ActionExecution/ActionSelector_LocalStoreValueFunctions_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/e2e/Regression/ClientSide/ActionExecution/ClearStore_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/locators/HomePage.js (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/AggregateHelper.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.
🪛 Biome (1.9.4)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts

[error] 240-240: 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)

🔇 Additional comments (3)
app/client/cypress/locators/HomePage.js (2)

58-59: LGTM! Proper usage of data-testid

The selector follows the best practices by using data-testid attribute.


107-109: LGTM! Proper usage of data-testid

These selectors follow best practices by using data-testid attributes.

app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ActionSelector_LocalStoreValueFunctions_spec.ts (1)

15-304: Overall code looks good

The test cases are well-structured and adhere to Cypress best practices.

Comment on lines +298 to +307
cy.window().then((win) => {
const newTab = win.open(win.location.href, "_blank");
if (newTab) {
cy.wrap(newTab).should(() => {
const isEmpty = Object.keys(newTab.localStorage).length === 0;
expect(isEmpty).to.be.false;
});
newTab.close();
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using 'window.open' in Cypress tests

Cypress does not support multiple tabs or window.open. Consider simulating the behavior within the same tab.

agHelper.GetNClick(propPane._deleteWidget);
});

it.only("4. To verify that clearStore() only affects the current tab's storage and does not inadvertently impact session storage across tabs.", function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove 'it.only' to execute all tests

The it.only method skips other tests. Please remove it to ensure all tests are executed.

Apply this diff to fix the issue:

- it.only("4. To verify that clearStore() only affects the current tab's storage and does not inadvertently impact session storage across tabs.", function () {
+ it("4. To verify that clearStore() only affects the current tab's storage and does not inadvertently impact session storage across tabs.", function () {
📝 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("4. To verify that clearStore() only affects the current tab's storage and does not inadvertently impact session storage across tabs.", function () {
it("4. To verify that clearStore() only affects the current tab's storage and does not inadvertently impact session storage across tabs.", function () {
🧰 Tools
🪛 Biome (1.9.4)

[error] 240-240: 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)

@sagar-qa007
Copy link
Contributor Author

/ci-test-limit-count run_count=5

Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts (3)

82-86: Consider extracting cleanup logic into afterEach hook

The widget cleanup logic is repeated across all test cases. Consider moving it to an afterEach hook to follow DRY principles.

+ afterEach(() => {
+   EditorNavigation.SelectEntityByName("Button1", EntityType.Widget);
+   agHelper.GetNClick(propPane._deleteWidget);
+   EditorNavigation.SelectEntityByName("Button2", EntityType.Widget);
+   agHelper.GetNClick(propPane._deleteWidget);
+ });

140-149: Enhance assertions with specific error messages

The assertions could be more specific about what is being tested. Consider adding custom error messages to make test failures more descriptive.

- agHelper.AssertContains(
+ agHelper.AssertContains(
    JSON.stringify({
      persistentVal1: "persistent value 1",
      persistentVal2: "persistent value 2",
      sessionVal1: "session value 1",
      sessionVal2: "session value 2",
    }),
+   "Store should contain both persistent and session values before clearing"
  );
- agHelper.AssertContains(JSON.stringify({}));
+ agHelper.AssertContains(JSON.stringify({}), "Store should be empty after clearing");

Line range hint 1-316: Consider implementing shared test utilities

The test file has repeated patterns for:

  1. JS object creation
  2. Button setup
  3. Widget cleanup

Consider extracting these into shared test utilities to improve maintainability.

Example utility function:

const setupTestButtons = (jsObjectBody: string, button1Label: string, button2Label: string) => {
  entityExplorer.DragDropWidgetNVerify("buttonwidget", 100, 100);
  jsEditor.CreateJSObject(jsObjectBody, {
    paste: true,
    completeReplace: true,
    toRun: false,
    prettify: false,
    shouldCreateNewJSObj: true,
  });

  EditorNavigation.SelectEntityByName("Button1", EntityType.Widget);
  propPane.UpdatePropertyFieldValue("Label", "");
  propPane.TypeTextIntoField("Label", button1Label);
  
  // ... rest of the setup logic
};
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9906ca7 and 1203bf5.

📒 Files selected for processing (1)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_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/ActionExecution/ClearStore_spec.ts (1)

160-163: ⚠️ Potential issue

Add cleanup for localStorage test data

Test data added to localStorage should be cleaned up after the test to avoid affecting other tests.

+ before(() => {
+   cy.window().then((win) => {
+     win.localStorage.clear();
+   });
+ });

  cy.window().then((win) => {
    win.localStorage.setItem("unrelatedKey1", "unrelated value 1");
    win.localStorage.setItem("unrelatedKey2", "unrelated value 2");
  });

+ after(() => {
+   cy.window().then((win) => {
+     win.localStorage.removeItem("unrelatedKey1");
+     win.localStorage.removeItem("unrelatedKey2");
+   });
+ });

Likely invalid or redundant comment.

@sagar-qa007 sagar-qa007 added the ok-to-test Required label for CI label Dec 10, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c14d734 and 9cda459.

📒 Files selected for processing (3)
  • app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/AggregateHelper.ts (1 hunks)
  • app/client/cypress/support/Pages/PropertyPane.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/cypress/support/Pages/PropertyPane.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_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/AggregateHelper.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 (9)
app/client/cypress/e2e/Regression/ClientSide/ActionExecution/ClearStore_spec.ts (5)

151-154: Use API calls for test cleanup instead of UI interactions

Similar to the previous test, replace widget deletion via UI with API calls.


160-163: LGTM! Direct localStorage usage is appropriate here

The direct manipulation of localStorage is justified for testing isolation of clearStore() functionality.


234-237: Use API calls for test cleanup instead of UI interactions

Replace widget deletion via UI with API calls.


313-316: Use API calls for test cleanup instead of UI interactions

Replace widget deletion via UI with API calls.


298-307: ⚠️ Potential issue

Avoid using window.open() in Cypress tests

Cypress doesn't support multiple browser tabs. Consider simulating the behavior within the same window context.

-      cy.window().then((win) => {
-        const newTab = win.open(win.location.href, "_blank");
-        if (newTab) {
-          cy.wrap(newTab).should(() => {
-            const isEmpty = Object.keys(newTab.localStorage).length === 0;
-            expect(isEmpty).to.be.false;
-          });
-          newTab.close();
-        }
-      });
+      // Simulate multi-tab behavior in same window
+      cy.window().then((win) => {
+        const storageSnapshot = { ...win.localStorage };
+        win.localStorage.clear();
+        expect(Object.keys(storageSnapshot).length).to.be.greaterThan(0);
+        Object.assign(win.localStorage, storageSnapshot);
+      });

Likely invalid or redundant comment.

app/client/cypress/support/Pages/AggregateHelper.ts (4)

1969-1980: Remove console.table from verification methods.

The use of console.table in test verification methods adds unnecessary noise to test output.

  public verifyConsoleLogNotContainingError(): void {
    cy.get("@error")
      .invoke("getCalls")
      .then((calls) => {
-       console.table(calls);
        cy.wrap(calls).each((call) => {
          // ... rest of the code
        });
      });
  }

  public verifyConsoleLogContainsExpectedMessage(message: string): void {
    cy.get("@log")
      .invoke("getCalls")
      .then((calls) => {
-       console.table(calls);
        cy.wrap(calls).each((call) => {
          // ... rest of the code
        });
      });
  }

Also applies to: 1982-1993


1947-1957: 🛠️ Refactor suggestion

Consider consolidating with existing RemoveCharsNType method and improve type safety.

Apply this diff to improve the implementation:

-  public RemoveChars(selector: string, charCount = 0, index = 0) {
+  public RemoveChars(selector: string, charCount = 0, index = 0): Cypress.Chainable<JQuery<HTMLElement>> {
+    if (charCount < -1) {
+      throw new Error("charCount must be >= -1");
+    }
+    
+    const element = this.GetElement(selector).eq(index);
+    
     if (charCount > 0) {
-      this.GetElement(selector)
-        .eq(index)
-        .focus()
-        .type("{backspace}".repeat(charCount), { timeout: 2, force: true })
-        .wait(50);
+      return element
+        .focus()
+        .type("{backspace}".repeat(charCount), { timeout: 2, force: true })
+        .wait(500);
     } else {
-      if (charCount == -1) this.GetElement(selector).eq(index).clear();
+      if (charCount === -1) {
+        return element.clear();
+      }
+      return element;
     }
   }

1995-2001: 🛠️ Refactor suggestion

Add error handling for clearConsoleLogs method.

  public clearConsoleLogs(): void {
    cy.window().then((win) => {
+     if (!win.console) {
+       cy.log('Console object not available');
+       return;
+     }
      cy.spy(win.console, "log").as("log");
      cy.spy(win.console, "error").as("error");
      cy.spy(win.console, "warn").as("warn");
    });
  }

Likely invalid or redundant comment.


1959-1967: 🛠️ Refactor suggestion

Add cleanup mechanism for console spies.

The method creates spies but doesn't provide a mechanism to restore them. This could lead to memory leaks in long-running test suites.

+ private consoleSpies: { logSpy: any; errorSpy: any; warnSpy: any } | null = null;

  public captureConsoleLogs(): void {
    cy.window()
      .its("console")
      .then((console) => {
-       cy.spy(console, "log").as("log");
-       cy.spy(console, "error").as("error");
-       cy.spy(console, "warn").as("warn");
+       const logSpy = cy.spy(console, "log").as("log");
+       const errorSpy = cy.spy(console, "error").as("error");
+       const warnSpy = cy.spy(console, "warn").as("warn");
+       this.consoleSpies = { logSpy, errorSpy, warnSpy };
      });
  }

+ public restoreConsoleLogs(): void {
+   if (this.consoleSpies) {
+     Object.values(this.consoleSpies).forEach(spy => spy.restore());
+     this.consoleSpies = null;
+   }
+ }

Likely invalid or redundant comment.

Comment on lines +82 to +86
EditorNavigation.SelectEntityByName("Button1", EntityType.Widget);
agHelper.GetNClick(propPane._deleteWidget);
EditorNavigation.SelectEntityByName("Button2", EntityType.Widget);
agHelper.GetNClick(propPane._deleteWidget);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use API calls for test cleanup instead of UI interactions

Replace widget deletion via UI with API calls for more reliable and faster test execution.

-      EditorNavigation.SelectEntityByName("Button1", EntityType.Widget);
-      agHelper.GetNClick(propPane._deleteWidget);
-      EditorNavigation.SelectEntityByName("Button2", EntityType.Widget);
-      agHelper.GetNClick(propPane._deleteWidget);
+      // Use API calls to delete widgets
+      cy.deleteWidget("Button1");
+      cy.deleteWidget("Button2");

Committable suggestion skipped: line range outside the PR's diff.

@appsmithorg appsmithorg deleted a comment from coderabbitai bot Dec 10, 2024
@appsmithorg appsmithorg deleted a comment from coderabbitai bot Dec 10, 2024
@sagar-qa007 sagar-qa007 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Dec 10, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
app/client/cypress/support/Pages/ApiPage.ts (1)

103-105: LGTM! Consider simplifying the selector.

The property declarations follow good practices with settingsTriggerLocator using data-testid. However, the runOnPageLoadJSObject selector could be simplified for better maintainability.

Consider using a data-testid attribute instead of the complex CSS selector:

-  private runOnPageLoadJSObject = "input[name^='execute-on-page-load'][type='checkbox']";
+  private runOnPageLoadJSObject = "[data-testid='execute-on-page-load-checkbox']";
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c6250d0 and fd8d5e2.

📒 Files selected for processing (3)
  • app/client/cypress/locators/commonlocators.json (1 hunks)
  • app/client/cypress/support/Pages/ApiPage.ts (2 hunks)
  • app/client/cypress/support/Pages/JSEditor.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/client/cypress/support/Pages/JSEditor.ts
  • app/client/cypress/locators/commonlocators.json
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/ApiPage.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/support/Pages/ApiPage.ts (1)

502-506: Redundant parameter default and implementation.

Comment on lines +508 to +512
public clickSettingIcon(enable: boolean) {
this.agHelper.GetNClick(this.settingsTriggerLocator);
if (enable) this.agHelper.CheckUncheck(this.runOnPageLoadJSObject, true);
else this.agHelper.CheckUncheck(this.runOnPageLoadJSObject, false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor to remove code duplication.

The toggle logic is duplicated between clickSettingIcon and ToggleOnPageLoadRunJsObject. This violates the DRY principle.

Consider refactoring to reuse the existing toggle method:

  public clickSettingIcon(enable: boolean) {
    this.agHelper.GetNClick(this.settingsTriggerLocator);
-    if (enable) this.agHelper.CheckUncheck(this.runOnPageLoadJSObject, true);
-    else this.agHelper.CheckUncheck(this.runOnPageLoadJSObject, false);
+    this.ToggleOnPageLoadRunJsObject(enable);
  }
📝 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
public clickSettingIcon(enable: boolean) {
this.agHelper.GetNClick(this.settingsTriggerLocator);
if (enable) this.agHelper.CheckUncheck(this.runOnPageLoadJSObject, true);
else this.agHelper.CheckUncheck(this.runOnPageLoadJSObject, false);
}
public clickSettingIcon(enable: boolean) {
this.agHelper.GetNClick(this.settingsTriggerLocator);
this.ToggleOnPageLoadRunJsObject(enable);
}

@sagar-qa007 sagar-qa007 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Dec 11, 2024
@sagar-qa007 sagar-qa007 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Dec 13, 2024
Copy link
Collaborator

@NandanAnantharamu NandanAnantharamu left a comment

Choose a reason for hiding this comment

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

Lgtm

@sagar-qa007 sagar-qa007 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Dec 14, 2024
@sagar-qa007 sagar-qa007 merged commit a5fa2aa into release Dec 16, 2024
287 of 294 checks passed
@sagar-qa007 sagar-qa007 deleted the test/frameworkfunctions branch December 16, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog Test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants