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

chore: removed old flags for airgap instances #36609

Merged
merged 19 commits into from
Oct 7, 2024

Conversation

AmanAgarwal041
Copy link
Contributor

@AmanAgarwal041 AmanAgarwal041 commented Sep 30, 2024

Description

Removed all the occurrences of listed flags in the codebase:

  1. ab_ds_binding_enabled
  2. ab_ds_schema_enabled
  3. ab_gsheet_schema_enabled
  4. ab_learnability_discoverability_collapse_all_except_data_enabled
  5. ab_learnability_ease_of_initial_use_enabled
  6. ab_mock_mongo_schema_enabled
  7. ab_start_with_data_default_enabled
  8. rollout_js_enabled_one_click_binding_enabled

Fixes #36256
or
Fixes Issue URL

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

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/11177173738
Commit: bfbf6bb
Cypress dashboard.
Tags: @tag.All
Spec:


Fri, 04 Oct 2024 10:31:10 UTC

Communication

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

  • Yes
  • No

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced test coverage for the Community Issues page, focusing on pagination, search, filtering, and issue management.
    • Improved functionality for adding new rows to table widgets, including visibility controls and state validations.
  • Bug Fixes

    • Resolved issues related to the visibility of UI elements when adding new rows and ensured accurate data reflection in the table.
  • Tests

    • Expanded tests for pagination, row selection, search functionality, and filtering logic in table widgets.
    • Added comprehensive assertions for client-side search and filtering scenarios, including checks for modal visibility during issue management.
  • Chores

    • Removed obsolete feature flags and streamlined logic for managing feature flags across components.

Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

Walkthrough

The changes in this pull request enhance the Cypress end-to-end tests for the Community Issues page and the Table widget. Key improvements include expanded checks for pagination, row selection, and validation of search functionality. Additionally, comprehensive testing of filtering logic and CRUD operations related to issues has been refined. The modifications aim to bolster the robustness of the test suite by ensuring thorough validation of user interactions and data integrity.

Changes

File Path Change Summary
app/client/cypress/e2e/Regression/Apps/CommunityIssues_Spec.ts Enhanced Cypress tests for pagination, row selection, search functionality, client-side search, filtering logic, and CRUD operations related to issues.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js Improved tests for adding new rows to a table widget, validating UI elements, row addition behavior, and interaction states.

Assessment against linked issues

Objective Addressed Explanation
Fix Cypress test cases related to removed flags (Task #36256)

Possibly related PRs

Suggested labels

Bug, Needs Triaging

Suggested reviewers

  • ApekshaBhosale
  • rahulbarwal
  • sagar-qa007

🎉 In the realm of code, where tests do play,
Community Issues shine brighter today!
Pagination and search, all working in sync,
With each little change, we’re closer, I think!
So raise up your flags, let’s celebrate this feat,
For testing's a journey, and this one’s a treat! 🎊


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9461218 and bfbf6bb.

📒 Files selected for processing (1)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js

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

@github-actions github-actions bot added Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Product Issues related to a specific integration Query & JS Pod Issues related to the query & JS Pod Task A simple Todo Tech Debt Issues or Tasks which are tech debts skip-changelog Adding this label to a PR prevents it from being listed in the changelog labels Sep 30, 2024
@AmanAgarwal041
Copy link
Contributor Author

/ci-test-limit

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: 14

🧹 Outside diff range and nitpick comments (48)
app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2Widget_selectedRow_Input_widget_spec.js (2)

Line range hint 14-26: Class, let's improve our test reliability by avoiding cy.wait.

In our first test case, we're using cy.wait("@updateLayout"). While this works, it's like watching the clock instead of checking if your experiment is actually complete. Let's consider using Cypress's built-in retry-ability for a more robust approach. For example:

cy.get('@updateLayout').its('response.body.responseMeta.status').should('eq', 200);

This way, we're actually checking for the condition we care about, rather than just waiting for a fixed amount of time.


Line range hint 14-45: Let's make our test descriptions more informative, students!

Our current test case descriptions are a bit like vague essay titles. They tell us what we're testing, but not the specific scenario or expected outcome. Let's make them more descriptive. For example:

  1. "Input widget displays default value from table widget v2 correctly"
  2. "Input widget updates correctly when a different row is selected in the table"

Remember, good test descriptions are like good essay titles - they should give a clear idea of what's being tested and what the expected outcome is.

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_DefaultSearch_Input_spec.js (2)

Line range hint 41-43: Let's clean up our work, class!

The cy.log() statement here is like writing notes in the margin of your test book - it's helpful while you're working, but not necessary for the final product. We can remove this line to keep our test clean and focused:

cy.readTableV2dataPublish("0", "0").then((tabData) => {
  const tabValue = tabData;
  expect(tabValue).to.be.equal("2736212");
  // Remove the following line
  // cy.log("the value is" + tabValue);
  cy.get(publish.inputWidget + " " + "input")
    .first()
    .invoke("attr", "value")
    .should("contain", tabValue);
});

Line range hint 43-47: Let's make our test more robust, like a well-built science project!

Instead of using complex selectors, we should use data-* attributes. This makes our tests more resilient to changes in the UI. Here's how we could improve this part:

cy.get('[data-cy="input-widget"]')
  .first()
  .invoke("attr", "value")
  .should("contain", tabValue);

Remember to add the corresponding data-cy attribute to your input widget in the actual application code.

app/client/cypress/limited-tests.txt (1)

Line range hint 26-29: Let's improve our documentation, shall we?

Class, I appreciate the inclusion of instructions for running all specs and the note about ci-test-limit. However, to make this file even more user-friendly, consider the following suggestions:

  1. Add a brief explanation of when and why one might want to run all specs.
  2. Provide more context about the ci-test-limit command. What does it do, and when should it be used?
  3. Consider adding a section with examples of how to use this file in different scenarios.

Remember, clear documentation is like a good lesson plan – it helps everyone understand and follow along more easily!

Here's a suggested improvement:

 # For running all specs - uncomment below:
 #cypress/e2e/**/**/*
 
-#ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command.
+# Note: The ci-test-limit command uses this file to run a minimum set of specs.
+# It should not be used to run the entire test suite.
+
+# Usage examples:
+# 1. To run specific tests: Use the lists above
+# 2. To run all tests: Uncomment the line above
+# 3. For CI with limited tests: Use the ci-test-limit command
app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js (2)

Line range hint 33-39: Well done, class! Let's review the changes.

I'm pleased to see the expansion of the pagination section and the verification of pagination functionality. However, we can make a small improvement to follow best practices.

Let's replace the static wait with a more robust assertion:

-      agHelper.WaitUntilToastDisappear("done");
+      cy.get(commonlocators.toastMsg).should('not.exist');

This change will make our test more reliable and less prone to timing issues. Remember, in testing, we always want to avoid unnecessary waits!


Line range hint 41-58: Excellent work on addressing Bug #22477!

Your test case effectively verifies the behavior of the next page button when the last page is reached. I'm particularly impressed with your attention to detail in checking both the disabled state and the absence of a toast message.

To make our test even more robust, let's add an assertion to verify that we're on the last page:

agHelper.GetNClick(table._nextPage("v2"));

agHelper.AssertAttribute(table._nextPage("v2"), "disabled", "disabled");
agHelper.AssertElementAbsence(locators._toastMsg);
// Add this line
cy.get('.t--widget-tablewidgetv2 .page-item').last().should('contain', '2');

This additional assertion will ensure that we're truly on the last page when the next button is disabled. Remember, in testing, we can never be too thorough!

app/client/cypress/e2e/Regression/ClientSide/Binding/InputWidget_TableV2_Sorting_spec.js (2)

36-36: Excellent addition to our test, students!

You've shown great attention to detail by expanding the row selection section before proceeding with the test. This ensures our test won't fail simply because a section is collapsed. Gold star for you!

However, let's make it even better. Can anyone tell me how we might improve this further? That's right! We should add a small wait after expanding the section to ensure it's fully loaded. Let's modify it like this:

table.ExpandIfCollapsedSection("rowselection");
cy.wait(500); // Short wait to ensure section is fully expanded

This will make our test even more robust. Remember, in testing as in life, patience is a virtue!


Line range hint 1-78: Class, let's review our work as a whole.

You've done a commendable job creating tests for our input and table widget interactions. Your coverage of default values, sorting, and column identification shows good thinking. However, we have some areas for improvement:

  1. I notice we're using cy.wait() in several places. Remember our lesson on avoiding explicit waits? Instead, let's use Cypress's built-in retry-ability. For example, replace:

    cy.wait(1000);
    cy.readTableV2dataPublish("0", "0")

    with:

    cy.readTableV2dataPublish("0", "0", { timeout: 10000 })
  2. We're using attribute selectors like publish.inputWidget + " " + "input". Let's improve this by using data attributes. Modify your app code to include data attributes, then update the selectors:

    cy.get('[data-cy="input-widget"]')
  3. Lastly, let's enhance our assertions. Instead of separate expect and cy.log statements, combine them:

    cy.readTableV2dataPublish("0", "0").should((tabValue) => {
      expect(tabValue).to.equal("6788734");
      cy.get('[data-cy="input-widget"]').should('have.value', tabValue);
    });

By making these changes, we'll have more robust and maintainable tests. Remember, good testing is the foundation of reliable software!

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Edge_case_spec.js (2)

Line range hint 70-70: Let's remove this wait, shall we?

Class, I've spotted a cy.wait(1000) on line 70. Remember our golden rule: avoid using cy.wait in our Cypress tests. It's like watching the clock during an exam - it doesn't help and only makes time pass slower!

Instead, let's think about what we're waiting for. Are we waiting for an element to appear? A network request to complete? Let's use Cypress's built-in retry-ability and wait for the specific condition we need.

For example, if we're waiting for rows to be selected, we could do something like:

cy.get('.selected-row').should('have.length', 3)

This way, we're telling Cypress exactly what to expect, rather than arbitrarily waiting. It's like raising your hand when you're done with a question, instead of just waiting for a set amount of time!


Line range hint 33-35: Time to refine our selectors, class!

I've noticed we're using CSS selectors in our cy.get() calls. While these work, they're not the best practice according to our coding guidelines. It's like using a nickname instead of a student's proper name - it might work, but it's not always clear or consistent!

Let's improve these by using data-* attributes instead. For example, instead of:

cy.get(`${widgetsPage.textWidget} .bp3-ui-text`)

We could use something like:

cy.get('[data-testid="text-widget-content"]')

This makes our tests more resilient to changes in the CSS and clearer to read. It's like using proper nouns in your essays - much more precise!

Remember to update the application code to include these data-* attributes where needed. It's a bit more work now, but it'll make our tests much easier to maintain in the future!

Also applies to: 44-46, 54-56, 65-67, 75-77

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_MultiRowSelect_spec.js (2)

Line range hint 41-53: Impressive work on this test case, students!

Your test demonstrates a thorough understanding of event handling and user feedback verification. The inclusion of toast message checks is particularly commendable.

However, I have a small suggestion to enhance your test:

Consider adding a negative test case to ensure that the toast message doesn't appear when a row is deselected. This would provide more comprehensive coverage of the onRowSelected behavior.

Here's an example of how you might implement this:

// Deselect the first row
cy.get(".t--table-multiselect").first().click({ force: true });
// Ensure the toast message doesn't appear
cy.get(commonlocators.toastmsg).should("not.exist");

Line range hint 55-73: Excellent work on this final test case, class!

Your test thoroughly covers the behavior of default selected rows and their interaction with the header cell. You've maintained our best practices throughout, which is commendable.

To further enhance this test, I have a small suggestion:

Consider adding an assertion to verify the state of the header cell after selecting and deselecting all rows. This would ensure that the header cell's state is consistent with the row selection state.

Here's an example of how you might implement this:

// After selecting all rows
cy.get(".t--table-multiselect-header").first().should("have.class", "selected");

// After deselecting all rows
cy.get(".t--table-multiselect-header").first().should("not.have.class", "selected");
app/client/cypress/e2e/Regression/ClientSide/Binding/TextTableV2_spec.js (1)

Line range hint 116-137: Well done, class! This new test case is a valuable addition.

The new test case for verifying default row selection functionality is well-structured and comprehensive. It covers both the editor and deployed states of the application, which is excellent practice.

However, let's make a small improvement to enhance our test's reliability:

Consider replacing the hard-coded wait with a more robust solution:

-cy.wait("@updateLayout");
+cy.intercept("POST", "/api/v1/layouts/update").as("updateLayout");
+cy.wait("@updateLayout").its("response.statusCode").should("equal", 200);

This change will make our test more resilient to timing issues and ensure that the layout update has completed successfully before proceeding.

app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (2)

Line range hint 1-24: Excellent work on improving the componentDidUpdate method, students!

You've made some smart changes here:

  1. Adding a check for string or finite values in defaultOptionValue improves our type handling.
  2. Using xorWith and equal for change detection is a more robust approach.

These changes will make our component more reliable and easier to maintain. However, I have a small suggestion to make the code even clearer:

Consider extracting the change detection logic into a separate helper function. This would make the componentDidUpdate method easier to read and test. Here's an example:

private hasDefaultOptionValueChanged(
  currentValue: string[] | OptionValue[],
  prevValue: string[] | OptionValue[]
): boolean {
  const isStringArray = currentValue.some(
    (value: any) => isString(value) || isFinite(value)
  );

  return isStringArray
    ? xorWith(currentValue as string[], prevValue as string[], equal).length > 0
    : xorWith(currentValue as OptionValue[], prevValue as OptionValue[], equal).length > 0;
}

Then in componentDidUpdate, you could simply call:

if (this.hasDefaultOptionValueChanged(this.props.defaultOptionValue, prevProps.defaultOptionValue) && this.props.isDirty) {
  this.props.updateWidgetMetaProperty("isDirty", false);
}

This would make your code more modular and easier to understand at a glance. Keep up the good work!


Line range hint 1-13: A gold star for adding the setSelectedOptions setter!

This is a valuable addition to our MultiSelectWidget. It allows other parts of the application to programmatically set the selected options, which is very useful for creating dynamic user interfaces.

Great job on enhancing the widget's functionality!

To make sure everyone understands how to use this new setter, let's add some documentation. Consider adding a comment above the setSelectedOptions configuration like this:

/**
 * Sets the selected options of the MultiSelect widget.
 * @param {array} options - An array of option values to be set as selected.
 */
setSelectedOptions: {
  path: "defaultOptionValue",
  type: "array",
  accessor: "selectedOptionValues",
},

This will help other developers understand how to use this setter effectively. Remember, clear documentation is like a good lesson plan - it helps everyone learn and use the code correctly!

app/client/src/widgets/SelectWidget/widget/index.tsx (2)

Line range hint 165-171: Excellent simplification of our property updates!

Students, observe how we've streamlined our getPropertyUpdatesForQueryBinding method. By removing the conditional check and always updating the dynamicPropertyPathList with 'sourceData', we've made our code more consistent and easier to understand.

However, let's consider a small improvement in our code style:

Consider using the spread operator for clarity:

-dynamicPropertyPathList.push({ key: "sourceData" });
+dynamicPropertyPathList = [...dynamicPropertyPathList, { key: "sourceData" }];

This approach creates a new array, which can be beneficial in certain React contexts and makes the addition of the new item more explicit.


Line range hint 1012-1023: Bravo on optimizing our filter change handling!

Class, let's examine the improvements in our onFilterChange method. We're now updating the widget's meta property 'filterText' and intelligently triggering server-side filtering only when necessary. This is an excellent optimization that will improve our application's performance.

To further enhance this method, consider the following suggestion:

We could add a debounce mechanism to prevent rapid successive calls to onFilterUpdate. This would be particularly beneficial for performance in cases of fast typing. Here's how you might implement it:

import { debounce } from 'lodash';

// At the class level
private debouncedFilterUpdate = debounce((value: string) => {
  if (this.props.onFilterUpdate && this.props.serverSideFiltering) {
    super.executeAction({
      triggerPropertyName: "onFilterUpdate",
      dynamicString: this.props.onFilterUpdate,
      event: {
        type: EventType.ON_FILTER_UPDATE,
      },
    });
  }
}, 300); // 300ms delay

// In the onFilterChange method
onFilterChange = (value: string) => {
  this.props.updateWidgetMetaProperty("filterText", value);
  this.debouncedFilterUpdate(value);
};

This change would make our widget even more efficient and responsive. What do you think about this enhancement?

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_ClientSide_Search_spec.js (1)

Line range hint 29-29: Let's avoid using cy.wait; use assertions or built-in retries instead

To make our tests more efficient and less flaky, we should avoid using cy.wait. Instead, we can use assertions or Cypress's built-in retry mechanisms to wait for elements or conditions.

You can replace the cy.wait(1000); with an assertion that checks for the desired state:

-// Disable Client Search
-_.agHelper.CheckUncheck(commonlocators.clientSideSearch, false);
-cy.wait(1000); //wait & then read the table value
+// Disable Client Search and wait for the table to update
+_.agHelper.CheckUncheck(commonlocators.clientSideSearch, false);
+cy.readTableV2dataPublish("0", "0").should("exist");

This way, the test waits dynamically for the table to update without a fixed delay.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (4)

Line range hint 130-131: Avoid Using cy.wait Statements

It's important to understand that using cy.wait with static wait times can lead to flaky tests. Instead, you should use assertions or commands that wait for certain conditions to be met, ensuring your tests are reliable and stable.

Please remove the cy.wait(100); statement and rely on an assertion to verify that the necessary conditions are met before proceeding. Here's how you can adjust your code:

- cy.wait(100);
  agHelper.ValidateToastMessage("This is a test message");

The ValidateToastMessage function should handle the waiting internally by checking for the toast message's appearance.


Line range hint 76-77: Use Data Attributes Instead of CSS Selectors

When selecting elements for interaction, it's best to use data-* attributes. This practice enhances the robustness of your tests by reducing their susceptibility to UI changes that might affect class names or structures.

Please update your selectors to use data-* attributes or locator variables. For example:

- cy.get(".t--propertypane").contains("Style").click({ force: true });
+ cy.get("[data-testid='t--propertypane']").contains("Style").click({ force: true });

This change will make your test more resilient and easier to maintain.


Line range hint 78-80: Avoid Complex CSS Selectors; Utilize Locator Variables

Using long or complex CSS selectors can make your tests fragile and harder to read. It's advisable to use locator variables, which are predefined selectors that improve code readability and maintainability.

Please modify your code to use locator variables from your locators file. For example:

- cy.get(".t--property-control-horizontalalignment .ads-v2-segmented-control__segments-container")
+ cy.get(commonLocators.horizontalAlignmentSegment)

This adjustment will help ensure that your tests are less affected by changes in the UI structure.


Line range hint 125-126: Enhance Test Reliability Without Static Waits

Using static waits like cy.wait(100); doesn't guarantee that the application is ready for the next action, which can cause intermittent test failures.

Instead of a static wait, consider using an assertion to confirm that the expected element or message is present before proceeding. For example:

- cy.wait(100);
  agHelper.ValidateToastMessage("This is a test message");
+ agHelper.ValidateToastMessage("This is a test message");

The ValidateToastMessage function should inherently wait for the toast message to appear, so the explicit wait is unnecessary.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)

Line range hint 38-43: Avoid hardcoded strings in assertions

To make your tests more robust and easier to maintain, it's advisable to avoid hardcoding strings in your assertions. Instead, define expected values as constants or variables. This approach improves clarity and simplifies updates.

Consider applying this change:

+const expectedUserName = "Lindsay Ferguson";
table.ReadTableRowColumnData(1, 3, "v2").then((cellData) => {
-  expect(cellData).to.eq("Lindsay Ferguson");
+  expect(cellData).to.eq(expectedUserName);
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (12)

Line range hint 21-28: Avoid Using Plain Strings for Selectors

Using class selectors like .t--property-control-allowaddingarow is discouraged. Instead, utilize data attributes and locator variables for consistency and reliability.

Refactor the selectors as follows:

-cy.get(".t--property-control-allowaddingarow").should("exist");
-cy.get(".t--property-control-allowaddingarow input").should("exist");
-cy.get(".t--add-new-row").should("not.exist");
-propPane.TogglePropertyState("Allow adding a row", "Off", null);
-cy.get(".t--add-new-row").should("not.exist");
-cy.get(".t--property-control-onsave").should("not.exist");
-cy.get(".t--property-control-ondiscard").should("not.exist");
-cy.get(".t--property-control-defaultvalues").should("not.exist");
+cy.get(propPane._allowAddARow).should("exist");
+cy.get(propPane._allowAddARowToggle).should("exist");
+cy.get(table._addNewRow).should("not.exist");
+propPane.TogglePropertyState("Allow adding a row", "Off");
+cy.get(table._addNewRow).should("not.exist");
+cy.get(propPane._onSave).should("not.exist");
+cy.get(propPane._onDiscard).should("not.exist");
+cy.get(propPane._defaultValues).should("not.exist");

Ensure that the locator variables like propPane._allowAddARow and table._addNewRow are defined in your locators file.


Line range hint 34-39: Avoid Disabling Tests Temporarily with Code

Instead of disabling the add new row link by entering inline edit mode, consider programmatically setting the state or mocking the behavior. This avoids potential flakiness in tests.


Line range hint 47-49: Use Data Attributes for Selectors

Selecting elements using class names can be unreliable. Replace:

cy.get(".tableWrap .new-row").should("exist");

With a selector that uses data attributes or locator variables.

Refactor as:

-cy.get(".tableWrap .new-row").should("exist");
+cy.get(table._newRow).should("exist");

Define table._newRow in your locators file.


Line range hint 54-56: Use Locator Variables Instead of Plain Strings

In the line:

cy.updateCodeInput(
  ".t--property-control-defaultvalues",
  "{{{step: 'newStepCell'}}}",
);

Replace the plain string selector with a locator variable.

Refactor as:

-cy.updateCodeInput(
-  ".t--property-control-defaultvalues",
-  "{{{step: 'newStepCell'}}}",
-);
+cy.updateCodeInput(
+  propPane._defaultValues,
+  "{{{step: 'newStepCell'}}}",
+);

Ensure propPane._defaultValues is defined.


Line range hint 62-65: Avoid Using Plain Strings in Assertions

When asserting table data, use locator variables and avoid hardcoded indices.

Refactor as:

-cy.readTableV2data(0, 0).then((val) => {
+cy.readTableV2data(rowIndex, colIndex).then((val) => {
   expect(val).to.equal("newStepCell");
 });

Define rowIndex and colIndex as needed.


Line range hint 71-88: Encapsulate Repeated Selectors Using Locator Variables

The repeated use of selectors like .t--widget-tablewidgetv2 .t--search-input should be replaced with locator variables to improve test maintainability.

Refactor as:

-cy.get(".t--widget-tablewidgetv2 .t--search-input").should("exist");
+cy.get(table._searchInput).should("exist");

Repeat this change for all similar selectors, defining appropriate locator variables in your locators file.


Line range hint 92-109: Avoid Using cy.get with Complex Selectors

Using complex CSS selectors can make tests brittle.

Refactor selectors to use data attributes or locator variables. For example:

-cy.get(".t--discard-new-row").click({ force: true });
+cy.get(table._discardNewRow).click();

Define table._discardNewRow accordingly.


Line range hint 115-124: Use Locator Variables for Dynamic Selectors

When constructing selectors with variables, ensure you're using locator variables and data attributes.

Refactor:

-cy.get(
-  `[data-colindex=0][data-rowindex=0] .t--inlined-cell-editor`,
-).should("exist");
+cy.get(table._inlinedCellEditor(0, 0)).should("exist");

Define table._inlinedCellEditor as a function that returns the selector string.


Line range hint 129-136: Avoid Hardcoding Values in Tests

Instead of hardcoding indices and values, use constants or variables for better readability and flexibility.

Consider defining variables for row and column indices, and expected values.


Line range hint 142-146: Use Proper Widget Selection Methods

When adding widgets, use the appropriate helper functions to interact with the canvas, ensuring consistency across tests.

Refactor:

-cy.dragAndDropToCanvas("textwidget", { x: 300, y: 600 });
+entityExplorer.DragDropWidgetNVerify("textwidget", 300, 600);

Ensure entityExplorer is correctly imported and used.


Line range hint 161-168: Use Exact Property Labels for Toggling

When toggling properties, ensure the labels match exactly to avoid errors due to typos or formatting differences.

Verify that the property labels used in propPane.TogglePropertyState match those in the UI.


Line range hint 172-174: Avoid Using cy.wait in Tests

The explicit wait cy.wait(1000); should be replaced with proper synchronization.

Refactor as:

-cy.wait(1000);
+agHelper.WaitUntilToastDisappear();

Or wait for a specific element to appear or disappear.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts (5)

Line range hint 141-141: Avoid using agHelper.Sleep() to ensure test reliability

Using agHelper.Sleep() can make our tests less reliable because it introduces arbitrary delays. Instead, let's wait for specific elements or conditions to ensure our application is ready before proceeding.


Line range hint 165-165: Avoid using agHelper.Sleep() to enhance test efficiency

Including agHelper.Sleep() can lead to inefficient and flaky tests. Let's use explicit waits or assertions to synchronize our test execution with the application's state.


Line range hint 182-183: Refrain from using cy.wait() with fixed timeouts

Relying on cy.wait(500) can cause our tests to be flaky. It's better to wait for specific elements or events. Let's find a more reliable synchronization method to improve test stability.


Line range hint 183-184: Use data-* attributes and locator variables for selectors

In lines 183-184, using complex CSS selectors like ".t--table-filter:nth-child(2) .t--table-filter-value-input input[type=text]" can make our tests brittle. Let's utilize data-* attributes and locator variables to enhance the robustness and maintainability of our tests.


Line range hint 240-240: Avoid using fixed timeouts like .wait(500)

Using .wait(500) after typing can make tests flaky. Instead, consider waiting for specific elements or conditions to ensure the application is ready before proceeding.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_spec.js (2)

Line range hint 69-70: Use Data- Attributes Instead of CSS Selectors*

When selecting elements in your tests, it's important to use data-* attributes rather than CSS selectors. Using selectors like cy.contains('[role="columnheader"] .draggable-header', "userName") can make your tests brittle, as any change in the DOM structure or classes can break them. By using data-* attributes, you ensure that your tests are more stable and maintainable.

Consider updating your code as follows:

- cy.contains('[role="columnheader"] .draggable-header', "userName")
+ cy.get("[data-colindex='3'] .draggable-header").click({ force: true });

Ensure that you have the appropriate data-* attributes added to your elements in the application code to support this change.


Line range hint 71-71: Avoid Using Fixed Delays Like cy.wait(1000);

Relying on fixed delays such as cy.wait(1000); can lead to unreliable tests. The application might take more or less time to reach the desired state, causing tests to fail intermittently. Instead, it's better to wait for a specific condition or element to appear before proceeding.

Here's how you can improve your test:

- cy.wait(1000);
+ cy.get(".t--table-widget").should("be.visible");

Replace the selector with the one that best indicates the table has finished sorting.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts (4)

Line range hint 41-41: Replace arbitrary waits with condition-based waits

At line 41:

agHelper.GetNClick(table._filterInputValue, 0).type("19").wait(500);

Using .wait(500); introduces an unnecessary delay and can cause the test to be unreliable. It's better to wait for the expected change in the table data.

You can remove the .wait(500); and add an assertion or wait for a specific element to confirm the filter has applied:

agHelper.GetNClick(table._filterInputValue, 0).type("19");
table.ReadTableRowColumnData(0, 3, "v2").then(($cellData) => {
  expect($cellData).to.eq("Tobias Funke");
});

Line range hint 96-96: Avoid using .wait(500);—wait for the expected condition instead

In line 96, after modifying the filter input:

.wait(500);

This fixed wait might not align with the actual time needed for the table to update.

Replace .wait(500); with a check for the updated table data:

table.ReadTableRowColumnData(0, 3, "v2").then(($cellData) => {
  expect($cellData).to.eq("Ryan Holmes");
});

Line range hint 147-147: Use dynamic waits rather than fixed delays

At line 147:

.wait(500);

Fixed waits can make tests slower and less reliable. It's better to wait for the action to complete by checking for a specific outcome.

After typing in the new filter value, add an assertion to wait for the table to reflect the changes:

table.ReadTableRowColumnData(0, 3, "v2").then(($cellData) => {
  expect($cellData).to.eq("Ryan Holmes");
});

Line range hint 26-26: Remember to avoid using agHelper.Sleep(); in tests

At line 26:

agHelper.Sleep(2000); //table to filter & records to disappear

Again, using fixed sleep periods isn't recommended. We should wait for a specific condition instead.

Use a method like table.WaitForTableEmpty("v2"); or check for the expected data.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_DisplayText_spec.ts (1)

Line range hint 43-43: Avoid using agHelper.Sleep(1000) to improve test reliability.

On line 43, the use of agHelper.Sleep(1000) is discouraged. Relying on static waits can lead to flaky tests due to timing inconsistencies. Instead, consider using dynamic waits that respond to application state changes.

Replace the sleep with a proper wait method, such as waiting for an element to be visible or a network call to complete. This ensures your test proceeds only when the application is ready, enhancing stability.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6f27959 and b92a9d0.

📒 Files selected for processing (34)
  • app/client/cypress/e2e/Regression/ClientSide/Autocomplete/PropertyPaneSlashCommand_spec.ts (0 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/InputWidget_TableV2_Sorting_spec.js (2 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2Widget_selectedRow_Input_widget_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_ClientSide_Search_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_DefaultSearch_Input_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TextTableV2_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/JSEnabledByDefaultExperiment_spec.ts (0 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (2 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Edge_case_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_1_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_DisplayText_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_MultiRowSelect_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_spec.js (3 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (2 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/server_side_filtering_spec_1.ts (1 hunks)
  • app/client/cypress/limited-tests.txt (1 hunks)
  • app/client/cypress/support/Objects/FeatureFlags.ts (0 hunks)
  • app/client/cypress/support/Pages/Table.ts (1 hunks)
  • app/client/cypress/support/e2e.js (0 hunks)
  • app/client/src/ce/entities/FeatureFlag.ts (0 hunks)
  • app/client/src/components/editorComponents/CodeEditor/codeEditorUtils.ts (0 hunks)
  • app/client/src/components/editorComponents/PartialImportExport/PartialExportModal/unitTestUtils.ts (0 hunks)
  • app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (1 hunks)
  • app/client/src/constants/WalkthroughConstants.ts (0 hunks)
  • app/client/src/pages/Editor/DatasourceInfo/QueryTemplates.tsx (1 hunks)
  • app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx (1 hunks)
  • app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1 hunks)
  • app/client/src/widgets/SelectWidget/widget/index.tsx (1 hunks)
  • app/client/src/widgets/TableWidgetV2/widget/index.tsx (1 hunks)
🔥 Files not summarized due to errors (1)
  • app/client/src/components/editorComponents/PartialImportExport/PartialExportModal/unitTestUtils.ts: Error: Server error: no LLM provider could handle the message
💤 Files with no reviewable changes (8)
  • app/client/cypress/e2e/Regression/ClientSide/Autocomplete/PropertyPaneSlashCommand_spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/JSEnabledByDefaultExperiment_spec.ts
  • app/client/cypress/support/Objects/FeatureFlags.ts
  • app/client/cypress/support/e2e.js
  • app/client/src/ce/entities/FeatureFlag.ts
  • app/client/src/components/editorComponents/CodeEditor/codeEditorUtils.ts
  • app/client/src/components/editorComponents/PartialImportExport/PartialExportModal/unitTestUtils.ts
  • app/client/src/constants/WalkthroughConstants.ts
🧰 Additional context used
📓 Path-based instructions (20)
app/client/cypress/e2e/Regression/ClientSide/Binding/InputWidget_TableV2_Sorting_spec.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/e2e/Regression/ClientSide/Binding/TableV2Widget_selectedRow_Input_widget_spec.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/e2e/Regression/ClientSide/Binding/TableV2_ClientSide_Search_spec.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/e2e/Regression/ClientSide/Binding/TableV2_DefaultSearch_Input_spec.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/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.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/e2e/Regression/ClientSide/Binding/TextTableV2_spec.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/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.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/e2e/Regression/ClientSide/Widgets/TableV2/Edge_case_spec.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/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_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/TableV2/TableV2Filter1_2_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/TableV2/TableV2Filter2_1_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/TableV2/TableV2Filter2_2_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/TableV2/TableV2_DisplayText_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/TableV2/TableV2_MultiRowSelect_spec.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/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_spec.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/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.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/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.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/e2e/Regression/ClientSide/Widgets/TableV2/server_side_filtering_spec_1.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.
app/client/cypress/support/Pages/Table.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/Binding/TableV2_Widget_API_Pagination_spec.js (1)
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2023-12-08T08:59:53.560Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
🔇 Additional comments (26)
app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2Widget_selectedRow_Input_widget_spec.js (1)

33-33: Well done, class! This addition improves our test reliability.

The new line _.table.ExpandIfCollapsedSection("rowselection"); is a smart move. It ensures that the row selection section is expanded before we interact with it. This is like making sure your textbook is open to the right page before you start reading. It helps prevent potential errors and makes our test more robust.

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_DefaultSearch_Input_spec.js (1)

30-30: Well done, class! This addition is a gold star moment.

Adding this line to expand the search and filters section is like opening your textbook before starting your homework. It ensures we can see and interact with all the important parts of our Table widget before we begin our test. This is a smart move that will help our test run smoothly every time.

app/client/cypress/limited-tests.txt (2)

1-24: Well done on maintaining a clear and organized file structure, class!

The file is neatly organized with clear sections for different categories of tests. This makes it easy for everyone to understand and maintain. Keep up the good work!


2-24: Class, let's discuss the importance of these new test specifications!

I'm pleased to see the addition of new test specifications, especially those related to the TableV2 widget. These tests will help ensure the reliability and functionality of this important component.

However, I have a few questions for you to ponder:

  1. For the tests under "Not fixed", what steps are being taken to address and resolve these issues?
  2. For the "Fixed" tests, have you considered adding comments to explain what was fixed in each case?
  3. Have you thought about grouping similar tests together for better organization?

Remember, thorough testing is key to maintaining a robust application. Keep up the excellent work!

To ensure all these test files exist, let's run a quick check:

app/client/cypress/e2e/Regression/ClientSide/Binding/InputWidget_TableV2_Sorting_spec.js (1)

10-10: Well done, class! You've added a new import.

I'm pleased to see you've imported the table object from ObjectsCore. This addition shows good foresight, as it will allow us to use table-related operations in our test cases. Remember, children, organized imports make for tidy code!

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Edge_case_spec.js (2)

29-29: Good addition, class! Let's expand on this.

The new line table.ExpandIfCollapsedSection("rowselection"); is a smart move. It ensures that the "rowselection" section is expanded before we proceed with our test. This is like making sure all your textbooks are open to the right page before we start our lesson!

However, let's take a moment to reflect on our coding guidelines:

  1. We're using a locator variable (table.ExpandIfCollapsedSection) instead of a plain string. Good job!
  2. We're not using any cy.wait, cy.pause, or agHelper.sleep(). Excellent!
  3. We're avoiding XPaths and CSS selectors. Well done!

Remember, class, always strive for clarity and consistency in your code. It's like keeping your desk tidy - it makes everything easier to find and understand!


Line range hint 1-79: Class, let's sum up our lesson for today!

Overall, this test file for the Table widget v2 edge cases is well-structured and covers important scenarios. The new addition on line 29 is a good improvement to ensure test reliability.

However, we have a few areas where we can make our code even better:

  1. Remove the use of cy.wait() and replace it with more specific wait conditions.
  2. Update our selectors to use data-* attributes instead of CSS selectors.
  3. Consider using API calls for login/logout/signup if authentication is needed in these tests.

Remember, writing good tests is like writing a good essay. We need to be clear, precise, and follow our guidelines. Keep up the good work, and let's strive to make our test suite the best it can be!

Don't forget to review these changes with your study group (team) and make the necessary updates. Class dismissed!

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_MultiRowSelect_spec.js (4)

Line range hint 15-22: Well done, class! This test case is a shining example of good practices.

I'm pleased to see that you've followed our Cypress best practices. You've used the recommended data-* attributes for selectors and avoided any unnecessary waits. Keep up the excellent work!


Line range hint 24-29: Excellent job on this test case, students!

Your attention to detail is commendable. You've correctly used the data-* attributes for selectors and maintained a clean, efficient test structure. This is precisely what we aim for in our Cypress tests.


Line range hint 31-39: Another stellar example, class!

Your test case demonstrates a clear understanding of the Table Widget V2's behavior. You've maintained our best practices by using data-* attributes and avoiding any unnecessary waits. Your attention to the header cell's state change is particularly noteworthy.


Line range hint 1-73: Excellent work on this test suite, class!

Your test cases for the Table Widget V2 row multi-select validation are comprehensive and well-structured. You've consistently followed our Cypress best practices, using appropriate selectors and avoiding unnecessary waits.

To further elevate your work, consider implementing the suggestions I've provided for the fourth and fifth test cases. These additions will enhance the robustness of your tests and provide even more comprehensive coverage.

Remember, continuous improvement is key in software testing. Keep up the great work!

app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx (1)

137-137: Class, let's examine this interesting change!

Well done on simplifying the code by removing the feature flag dependency. This change will ensure consistent behavior across all instances.

However, I'd like you to raise your hand and answer these questions:

  1. Is this change intentional and approved by the product team?
  2. Have we considered how this might affect our users' experience with the property pane?

To make sure we're all on the same page, let's add a comment explaining this change:

+  // Note: All sections now default to expanded state as per the removal of the feature flag
   true,

Remember, clear documentation is key to maintaining our codebase!

app/client/src/pages/Editor/DatasourceInfo/QueryTemplates.tsx (4)

Line range hint 1-25: Good job cleaning up the imports, class!

I see you've removed the import for WalkthroughContext. That's excellent housekeeping! Remember, keeping our imports tidy is like keeping our desks clean - it helps us work more efficiently.


Line range hint 172-214: Let's wrap up our review of the QueryTemplates component, class!

You've done a commendable job in removing the walkthrough-related code from this component. It's like decluttering our classroom - we've removed what we no longer need, but kept all the important learning materials.

I'm pleased to see that the core functionality of creating and updating query actions remains intact. The rendering of template options in the menu items is unchanged, which is excellent.

However, I want you all to think about this: How might removing the walkthrough feature affect new users? Are there other ways we can guide them through using these query templates? Perhaps we could add some helpful tooltips or improve our documentation?

Remember, good code is not just about what works now, but also about making it easy for others (including your future selves) to understand and use. Keep up the good work, and always think about the user experience!


Line range hint 119-170: Now, let's examine our updateQueryAction function, class!

I'm glad to see you've tidied up this function too. Just like in createQueryAction, you've removed the walkthrough-related code. This is good consistency!

However, I want you to think about something: Are there any other parts of our application that might have been relying on these walkthrough features? It's like removing a chapter from a textbook - we need to make sure no other chapters were referencing it.

Also, notice how our analytics logging has changed. We're still tracking important information, but we've removed the walkthrough-specific data. This is a good example of how our code evolves as our application's features change.

Let's do a quick check to make sure we haven't missed anything:

#!/bin/bash
# Description: Check for any remaining references to walkthrough in our codebase

# Test: Search for any remaining walkthrough references
rg --type typescript 'walkthrough' -i

This will help us ensure we've removed all references to the walkthrough feature. If we find any, we might need to update those areas too!


Line range hint 80-117: Let's review the changes in our createQueryAction function, students!

I'm pleased to see that you've removed the walkthrough-related code. It's like taking training wheels off a bicycle - we're moving forward! However, let's make sure our bicycle still rides smoothly.

Can you double-check that removing the isWalkthroughOpened variable and popFeature function call doesn't affect the query creation process? It's important to ensure that our actions still work as expected, even without the walkthrough.

Also, notice how we've simplified our analytics event. It's like writing a more concise essay - we're only including the most relevant information now.

To make sure everything is working correctly, let's run a quick test:

This will help us see if there are any places where we might need to update how we're calling createQueryAction.

✅ Verification successful

Let's refine our approach to searching for createQueryAction in TypeScript files!

It seems that ripgrep doesn't recognize typescript as a file type. No worries—we can adjust our script to target .ts and .tsx files directly. This should help us locate all instances of createQueryAction effectively.


Great job! The createQueryAction function is still being called correctly.

All instances of createQueryAction in QueryTemplates.tsx and other relevant files are intact, ensuring that the removal of walkthrough-related code doesn't disrupt the query creation process. Everything looks smooth!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if createQueryAction is still being called correctly

# Test: Search for createQueryAction usage
rg --type typescript 'createQueryAction\(' -A 5

Length of output: 72


Script:

#!/bin/bash
# Description: Check if createQueryAction is still being called correctly

# Search for createQueryAction usage in .ts and .tsx files
rg 'createQueryAction\(' --glob '*.ts' --glob '*.tsx' -A 5

Length of output: 2345

app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (1)

196-196: Excellent simplification, class!

I see you've removed the conditional check for the JavaScript-enabled one-click binding feature flag. This is a good step towards cleaning up our codebase and aligning with our lesson plan of removing old flags.

Let's break it down for the class:

  1. Before: We were checking a condition to decide whether to enable JavaScript bindings.
  2. After: We're always setting it to true.

This change suggests that the feature has been fully rolled out and is now a standard part of our application. Well done!

However, let's not forget our homework:

  1. Ensure all related code that might have depended on this flag has been updated.
  2. Update our documentation to reflect that this feature is now always enabled.

Keep up the good work, and remember: in coding, as in the classroom, simplicity is often the best teacher!

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

891-891: Attention, class! Let's examine this new method.

The getFormattedTomorrowDates method is a well-structured addition to our codebase. It provides a helpful utility for obtaining tomorrow's date in two different formats. This method demonstrates good use of JavaScript's Date object and formatting functions.

app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1)

183-183: Well done on simplifying the code, class!

I see you've removed the conditional check for a feature flag and added 'sourceData' to the dynamicPropertyPathList. This is a good step forward! It shows that we're confident in the feature and are treating 'sourceData' as a dynamic property. Remember, this means 'sourceData' will now be re-evaluated when dependencies change, so make sure it's handled efficiently in the rest of the codebase.

app/client/src/widgets/SelectWidget/widget/index.tsx (2)

Line range hint 165-171: Well done on enhancing the query generation!

Class, let's take a moment to appreciate the improvement in our getQueryGenerationConfig method. By adding the where clause that references the widget's filterText, we've made our server-side filtering more robust. This change allows for more dynamic and responsive queries based on user input.


Line range hint 1-1023: Excellent work on improving our SelectWidget!

Class, let's recap the enhancements we've seen in this file:

  1. We've improved our query generation capabilities.
  2. We've simplified our property update logic.
  3. We've optimized our filter change handling.

These changes collectively contribute to a more efficient, responsive, and maintainable SelectWidget. Keep up the good work, and remember to consider the suggestions for further improvements. Well done!

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (1)

155-159: ⚠️ Potential issue

Avoid Using cy.wait in Tests

The use of cy.wait(1000); is discouraged as it can lead to flaky tests.

Refactor to use assertions or other synchronization methods. For example, wait for a specific element to appear:

-cy.wait(1000);
+agHelper.WaitUntilElementAppear(propPane._propertyControl("Show pagination"));

Ensure agHelper.WaitUntilElementAppear is implemented appropriately.

Likely invalid or redundant comment.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_spec.js (2)

129-129: Verify the Escaping in the Selector String

In the line _.table.ExpandIfCollapsedSection("search\\&filters");, you're using double backslashes to escape the & character. This might not be necessary and can cause confusion or errors. Ensure that the escaping is intentional and functioning as expected.

Please check if the double backslash is required. If not, you can simplify it:

- _.table.ExpandIfCollapsedSection("search\\&filters");
+ _.table.ExpandIfCollapsedSection("search&filters");

128-128: ⚠️ Potential issue

Avoid Using Fixed Delays Like cy.wait(500);

Using arbitrary waits like cy.wait(500); can slow down your test suite and still may not ensure that the application is ready for the next action. It's better to wait for a specific event or element to ensure synchronization.

Consider this adjustment:

- cy.wait(500);
+ cy.get(".t--property-pane").should("be.visible");

This change waits until the property pane is visible, ensuring that the application is ready for the next steps in your test.

Likely invalid or redundant comment.

app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)

277-277: Well done on simplifying the dynamic property updates!

By always pushing tableData to dynamicPropertyPathList, you've removed the unnecessary feature flag check and streamlined the code. This enhancement makes the getPropertyUpdatesForQueryBinding method cleaner and more maintainable.


277-277: Ensure all remnants of the removed feature flag are eliminated.

Since the feature flag rollout_js_enabled_one_click_binding_enabled has been retired, it's important to verify that there are no lingering references elsewhere in the codebase. Residual references could cause unexpected behavior or build issues.

Please run the following script to check for any remaining references:

Comment on lines 27 to 29
);
// validation of data displayed in input widgets based on search value set
EditorNavigation.SelectEntityByName("Table1", EntityType.Widget);
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

Time for some homework revisions, students!

Let's improve our test by avoiding the use of cy.wait("@updateLayout"). Instead, we can use Cypress's built-in retry-ability to wait for elements to be in the correct state. Here's an example of how we could rewrite this:

cy.get(".t--property-control-allowsearching input").should('be.visible').click({ force: true });

This way, we're letting Cypress automatically wait for the element to be visible before clicking, which is more reliable than using a fixed wait time.

Also applies to: 33-34

Comment on lines 892 to 906
public ExpandIfCollapsedSection(sectionName: string) {
cy.get(`.t--property-pane-section-collapse-${sectionName}`)
.scrollIntoView()
.then(($element) => {
cy.wrap($element)
.siblings(".bp3-collapse")
.then(($sibling) => {
const siblingHeight = $sibling.height(); // Get the height of the sibling element

if (!siblingHeight) {
$element.click();
}
});
});
}
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

Now, let's turn our attention to the ExpandIfCollapsedSection method.

This method is designed to expand a collapsed section in the property pane if it's not already expanded. While the intention is good, there are a few areas where we can improve:

  1. The method uses Cypress commands directly, which might not be consistent with the rest of the class that seems to use an agHelper for most Cypress interactions.

  2. The method doesn't handle potential errors, such as if the element is not found.

  3. The logic for determining if a section is collapsed (by checking the height of a sibling element) might be fragile if the DOM structure changes.

Let's consider refactoring this method to make it more robust and consistent with the rest of the class. Here's a suggested improvement:

- public ExpandIfCollapsedSection(sectionName: string) {
-   cy.get(`.t--property-pane-section-collapse-${sectionName}`)
-     .scrollIntoView()
-     .then(($element) => {
-       cy.wrap($element)
-         .siblings(".bp3-collapse")
-         .then(($sibling) => {
-           const siblingHeight = $sibling.height(); // Get the height of the sibling element
-
-           if (!siblingHeight) {
-             $element.click();
-           }
-         });
-     });
- }
+ public ExpandIfCollapsedSection(sectionName: string) {
+   return this.agHelper.GetElement(`.t--property-pane-section-collapse-${sectionName}`)
+     .then(($element) => {
+       if ($element.siblings(".bp3-collapse").height() === 0) {
+         this.agHelper.ScrollTo(`.t--property-pane-section-collapse-${sectionName}`);
+         this.agHelper.ClickButton($element);
+       }
+     })
+     .catch((error) => {
+       this.agHelper.LogErrorAndFail(`Failed to expand section ${sectionName}: ${error}`);
+     });
+ }

This refactored version:

  1. Uses the agHelper for consistency with the rest of the class.
  2. Handles potential errors with a catch block.
  3. Simplifies the logic for checking if the section is collapsed.
  4. Uses more descriptive method names from the agHelper.

Remember, class, consistency and error handling are key to maintaining a robust test suite!

Committable suggestion was skipped due to low confidence.

Comment on lines 24 to 26
// Input onsearchtextchanged control
cy.get(".t--property-control-onsearchtextchanged .t--js-toggle")
.first()
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

Remember to use locator variables and data- attributes instead of CSS selectors*

As we've learned, using locator variables and data-* attributes enhances the maintainability and reliability of our tests. Relying on plain strings and CSS paths can make the tests brittle and harder to maintain. Let's replace the CSS selectors with appropriate locator variables.

Here's how you might adjust the code:

-cy.get(".t--property-control-onsearchtextchanged .t--js-toggle")
-  .first()
-  .click();
+cy.get(commonlocators.onSearchTextChangedToggle)
+  .click();

If commonlocators.onSearchTextChangedToggle doesn't exist, please consider adding it to your locators file.

📝 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
// Input onsearchtextchanged control
cy.get(".t--property-control-onsearchtextchanged .t--js-toggle")
.first()
// Input onsearchtextchanged control
cy.get(commonlocators.onSearchTextChangedToggle)
.click();

@@ -35,6 +35,7 @@ describe(
entityExplorer.DragDropWidgetNVerify(draggableWidgets.TABLE, 300, 300);

// turn on filtering for the table - it is disabled by default in this PR(#34593)
table.ExpandIfCollapsedSection("search\\&filters");
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

Remember to use locator variables instead of plain strings.

To ensure consistency and maintainability, it's important to use locator variables for selectors. Instead of passing a plain string "search\\&filters", consider defining and using a locator variable for the "search&filters" section.

@@ -35,6 +35,7 @@ describe(
before(() => {
_.entityExplorer.DragDropWidgetNVerify(_.draggableWidgets.TABLE);
// turn on filtering for the table - it is disabled by default in this PR(#34593)
_.table.ExpandIfCollapsedSection("search\\&filters");
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

Remember to use locator variables instead of plain strings for locators

Dear student, to maintain code consistency and adhere to our best practices, please use locator variables instead of plain strings when specifying locators. This enhances the maintainability and readability of the code.

You can apply the following change to fix this:

- _.table.ExpandIfCollapsedSection("search\\&filters");
+ _.table.ExpandIfCollapsedSection(_.locators.searchAndFiltersSection);

If a locator variable for "search\\&filters" does not exist yet, kindly define it accordingly.

Committable suggestion was skipped due to low confidence.

@@ -237,6 +240,7 @@ describe(
it("7. should check that adding cyclic dependency in the table doesn't crash the app", () => {
//_.deployMode.NavigateBacktoEditor();
cy.openPropertyPane("tablewidgetv2");
_.table.ExpandIfCollapsedSection("rowselection");
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

Consider Using Locator Variables Instead of Plain Strings

Using plain strings like _.table.ExpandIfCollapsedSection("rowselection"); can make your tests harder to maintain, especially if the string is used in multiple places or may change in the future. By defining locator variables or constants, you centralize these values and make updates easier.

For example:

const rowSelectionSection = "rowselection";
_.table.ExpandIfCollapsedSection(rowSelectionSection);

This approach improves readability and maintainability of your tests.

Comment on lines 19 to 20
table.ExpandIfCollapsedSection("search\\&filters");
agHelper.GetNClick(".t--property-control-allowfiltering input");
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

Remember to use locator variables instead of plain strings for selectors

At line 20, we have:

agHelper.GetNClick(".t--property-control-allowfiltering input");

Using plain string selectors can make our tests brittle and harder to maintain. It's important to use locator variables, which promote reuse and make changes easier to manage.

Here's how you can update the code:

+const allowFilteringToggle = ".t--property-control-allowfiltering input";
...
-agHelper.GetNClick(".t--property-control-allowfiltering input");
+agHelper.GetNClick(allowFilteringToggle);

@@ -16,6 +16,7 @@
it("1. Verify Table Filter for 'empty'", function () {
entityExplorer.DragDropWidgetNVerify("tablewidgetv2", 650, 250);
// turn on filtering for the table - it is disabled by default in this PR(#34593)
table.ExpandIfCollapsedSection("search\\&filters");
agHelper.GetNClick(".t--property-control-allowfiltering input");
table.AddSampleTableData();
propPane.UpdatePropertyFieldValue(
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

Avoid using agHelper.Sleep();—wait for specific conditions instead

In line 22, the code uses:

agHelper.Sleep(2000); //table to filter & records to disappear

Relying on fixed delays can lead to flaky tests because the actual load time may vary. Instead, we should wait for a specific element or condition that indicates the table has finished filtering.

Consider replacing the sleep with a wait for the table to become empty:

table.WaitForTableEmpty("v2");

This ensures the test proceeds only after the table is fully filtered.

@@ -17,6 +17,7 @@ describe(
entityExplorer.DragDropWidgetNVerify("tablewidgetv2", 650, 250);
//propPane.EnterJSContext("Table data", JSON.stringify(this.dataSet.TableInput));
// turn on filtering for the table - it is disabled by default in this PR(#34593)
table.ExpandIfCollapsedSection("search\\&filters");
agHelper.GetNClick(".t--property-control-allowfiltering input");
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

Let's improve selector usage by adopting locator variables

In line 21, you're directly using a CSS selector ".t--property-control-allowfiltering input". To make our tests more robust and maintainable, it's important to use locator variables and leverage data-* attributes for selectors, as highlighted in our coding guidelines. This practice enhances readability and eases future updates to the code.

To implement this improvement, let's define a locator variable using a data-* attribute:

Add the following in your locators file (e.g., locators.js):

export const allowFilteringToggle = '[data-cy="t--property-control-allowfiltering"] input';

Then, update line 21 as shown below:

- agHelper.GetNClick(".t--property-control-allowfiltering input");
+ agHelper.GetNClick(allowFilteringToggle);

@@ -28,6 +28,7 @@ describe(
before(() => {
entityExplorer.DragDropWidgetNVerify("tablewidgetv2", 650, 250);
// turn on filtering for the table - it is disabled by default in this PR(#34593)
table.ExpandIfCollapsedSection("search\\&filters");
agHelper.GetNClick(".t--property-control-allowfiltering input");
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

Remember to use locator variables instead of plain strings for selectors.

In line 32, you've used a plain string selector in agHelper.GetNClick(".t--property-control-allowfiltering input"). It's important to use locator variables for selectors to maintain consistency and ease future updates.

Let's define a locator variable for .t--property-control-allowfiltering input and replace the plain string. This practice enhances code readability and adherence to our coding standards.

@AmanAgarwal041
Copy link
Contributor Author

/ci-test-limit runId=11102991010

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

🧹 Outside diff range and nitpick comments (8)
app/client/cypress/e2e/Regression/ClientSide/DynamicHeight/JsonForm_spec.ts (1)

Line range hint 1-65: Class, let's talk about good habits in our code!

Now, children, I've noticed something in our test that we need to improve. Can anyone spot the line that appears several times and starts with agHelper.Sleep? Yes, that's right!

Using agHelper.Sleep(5000) is like taking a nap in the middle of a test - it's not very efficient! Instead of sleeping, we should be actively waiting for something to happen. Here are some better ways to do this:

  1. Use cy.wait('@apiCall') if you're waiting for a specific API call to complete.
  2. Use cy.get('element').should('be.visible') to wait for an element to appear.
  3. If you're waiting for a change in an element's property, use something like cy.get('element').should('have.css', 'height', expectedHeight).

Remember, class, in testing as in life, it's better to be active and attentive rather than just waiting around!

Let's work on replacing these agHelper.Sleep(5000) calls with more appropriate waiting mechanisms. Who wants to come to the board and show us how to do the first one?

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2TextPagination_spec.js (3)

Line range hint 96-97: Consider using data- attributes or locator variables instead of class selectors*

Using class selectors like ".show-page-items" and ".page-item" can make your tests fragile if class names change in the future. It's a good practice to use data-* attributes or predefined locator variables for more robust and maintainable selectors.

You might update the selectors like this:

- cy.get(".show-page-items").should("contain", "20 Records");
+ cy.get(locators._showPageItems).should("contain", "20 Records");

- cy.get(".page-item").next().should("contain", "of 2");
+ cy.get(locators._pageItem).next().should("contain", "of 2");

Ensure that the appropriate locator variables locators._showPageItems and locators._pageItem are defined in your locators file.


Line range hint 99-99: Avoid using class selectors; prefer data- attributes or locator variables*

Referencing elements by class names like ".t--table-widget-next-page" may lead to brittle tests if those class names change. It's advisable to use data-* attributes or locator variables for more stable and maintainable selectors.

Consider updating your selectors:

- cy.get(".t--table-widget-next-page").should("not.have.attr", "disabled");
+ cy.get(locators._tableWidgetNextPage).should("not.have.attr", "disabled");

...

- cy.get(".t--table-widget-next-page").should("have.attr", "disabled");
+ cy.get(locators._tableWidgetNextPage).should("have.attr", "disabled");

Make sure that locators._tableWidgetNextPage is properly defined in your locators file.

Also applies to: 103-103


Line range hint 102-102: Avoid using static waits like cy.wait(500)

Including fixed-duration waits can make your tests less reliable and slower. It's better to wait for specific conditions or elements to ensure that the application is ready before proceeding.

Consider replacing the static wait with a dynamic wait. Since you're checking that the "Next Page" button has the disabled attribute in the next line, you might not need the static wait at all.

- cy.wait(500);

If a wait is necessary, you can wait for a network call or an element state:

- cy.wait(500);
+ cy.wait("@postExecute");

Or ensure the element is in the expected state:

- cy.wait(500);
+ cy.get(locators._tableWidgetNextPage).should("have.attr", "disabled");
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_PropertyPane_2_spec.js (4)

Line range hint 16-17: Avoid Fixed Delays with cy.wait(5000);

In lines 16-17, you're using cy.wait(5000); to introduce a fixed delay in your test. Relying on fixed delays can lead to flaky tests because it doesn't guarantee that the desired state has been reached. Instead, you should use dynamic waits that wait for specific events or elements.

You already have cy.wait("@updateLayout"); immediately after the fixed wait. This dynamic wait should be sufficient. Let's remove the unnecessary fixed delay:

- // eslint-disable-next-line cypress/no-unnecessary-waiting
- cy.wait(5000);

65-65: Use Locator Variables Instead of Plain Strings

In line 65, you're using a plain string "search\\&filters" in the table.ExpandIfCollapsedSection method. According to our coding guidelines, it's better to use locator variables or data-* attributes for locators rather than plain strings. This practice enhances maintainability and reduces the likelihood of errors when locators change.

Consider defining a locator variable for "search\\&filters" in your locators file and using that variable here.


Line range hint 70-70: Correct Typographical Errors in Comments

The comment on line 70 contains typographical errors: "// Chage deat search text value to \"data\"". Clear and correct comments improve code readability and maintainability.

Please apply the following correction:

- // Chage deat search text value to "data"
+ // Change default search text value to "data"

Line range hint 91-94: Prefer Data- Attributes Over CSS Selectors*

In lines 91-94, you are using a CSS selector ".t--property-control-propertyname pre span span" to select an element. While this works, using data-* attributes for selectors is a better practice. It makes your tests more robust and less likely to break with UI changes.

Consider updating the selector to use a data-* attribute:

- cy.get(".t--property-control-propertyname pre span span").should(
+ cy.get("[data-testid='t--property-control-propertyname']").should(
    "have.text",
    "customColumn18",
);

This change will make your test more maintainable and aligned with best practices.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b92a9d0 and 63010c6.

📒 Files selected for processing (22)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/InputWidget_TableV2_Sorting_spec.js (2 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2TextPagination_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2Widget_selectedRow_Input_widget_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_ClientSide_Search_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_DefaultSearch_Input_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TextTableV2_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/DynamicHeight/JsonForm_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (2 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Edge_case_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_1_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_DisplayText_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_MultiRowSelect_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_PropertyPane_2_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_spec.js (3 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/server_side_filtering_spec_1.ts (1 hunks)
  • app/client/cypress/support/Pages/PropertyPane.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
  • app/client/cypress/e2e/Regression/ClientSide/Binding/InputWidget_TableV2_Sorting_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2Widget_selectedRow_Input_widget_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_ClientSide_Search_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_DefaultSearch_Input_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Binding/TextTableV2_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Edge_case_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_1_Spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_DisplayText_spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_MultiRowSelect_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/server_side_filtering_spec_1.ts
🧰 Additional context used
📓 Path-based instructions (4)
app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2TextPagination_spec.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/e2e/Regression/ClientSide/DynamicHeight/JsonForm_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/TableV2/TableV2_PropertyPane_2_spec.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/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.
📓 Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2TextPagination_spec.js (1)
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2023-12-08T08:59:53.560Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/DynamicHeight/JsonForm_spec.ts (1)

19-19: Good addition, class! Let's discuss why this is important.

Children, the new line propPane.ExpandIfCollapsedSection("general"); is like making sure your textbook is open to the right page before you start reading. It ensures that the "general" section of the properties pane is visible before we try to change any settings. This makes our test more reliable, just like how checking your pencil works before starting a test makes sure you can answer all the questions.

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2TextPagination_spec.js (1)

93-94: Excellent work on updating the pagination settings!

You've correctly expanded the pagination section and updated the "Total Records" field to "20". This will help ensure that the table displays the correct number of records and pagination controls as intended.

Comment on lines +689 to +704

public ExpandIfCollapsedSection(sectionName: string) {
cy.get(`.t--property-pane-section-collapse-${sectionName}`)
.scrollIntoView()
.then(($element) => {
cy.wrap($element)
.siblings(".bp3-collapse")
.then(($sibling) => {
const siblingHeight = $sibling.height(); // Get the height of the sibling element

if (!siblingHeight) {
$element.click();
}
});
});
}
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

Class, let's review this new method together!

The ExpandIfCollapsedSection method is a good addition to our toolkit. It's like a helpful classroom assistant, making sure all our "learning sections" are open and ready for us to explore. However, we can make it even better!

  1. Error Handling: We should add a safety net in case we can't find our section. It's like having a backup plan for when a student is absent.

  2. Confirmation: Let's add a way to make sure our section actually opened. It's like double-checking that everyone has their textbooks open to the right page.

  3. Neat Handwriting: Our code's indentation is a bit messy. Remember, neat handwriting makes it easier for everyone to read!

Here's how we could improve it:

public ExpandIfCollapsedSection(sectionName: string) {
  cy.get(`.t--property-pane-section-collapse-${sectionName}`)
    .scrollIntoView()
    .then(($element) => {
      cy.wrap($element)
        .siblings(".bp3-collapse")
        .then(($sibling) => {
          const siblingHeight = $sibling.height();
          if (!siblingHeight) {
            cy.wrap($element).click().then(() => {
              // Verify that the section is now expanded
              cy.wrap($sibling).should('have.css', 'height').and('not.eq', '0px');
            });
          }
        });
    })
    .should('exist')
    .then(($element) => {
      if (!$element.length) {
        throw new Error(`Section "${sectionName}" not found`);
      }
    });
}

This improved version is like a well-organized lesson plan. It checks for errors, confirms our actions, and keeps everything tidy. Great job on adding this helpful method to our class!

@AmanAgarwal041
Copy link
Contributor Author

/ci-test-limit

Copy link

github-actions bot commented Oct 1, 2024

@AmanAgarwal041 AmanAgarwal041 added the ok-to-test Required label for CI label Oct 1, 2024
@AmanAgarwal041
Copy link
Contributor Author

/ci-test-limit

Copy link

github-actions bot commented Oct 2, 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: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 92ce628 and c2158d8.

📒 Files selected for processing (2)
  • app/client/cypress/e2e/Regression/Apps/CommunityIssues_Spec.ts (5 hunks)
  • app/client/cypress/limited-tests.txt (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 (1)
app/client/cypress/e2e/Regression/Apps/CommunityIssues_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 (4)
app/client/cypress/e2e/Regression/Apps/CommunityIssues_Spec.ts (4)

77-78: Well done verifying the 'Server side pagination' setting

You've correctly expanded the "pagination" section and asserted that "Server side pagination" is enabled. This ensures the feature is functioning as expected.


Line range hint 80-84: Excellent validation of the default selected row

By expanding the "rowselection" section and validating the "Default selected row", you've ensured that the table selection behaves correctly.


119-120: Good job toggling 'Server side pagination' off

Disabling "Server side pagination" and verifying the application's response helps confirm the feature's behavior under different settings.


132-132: Appropriate update of 'Default selected row'

By changing the "Default selected row" to "1", you've properly tested how the table handles different default selections.

@@ -140,6 +144,7 @@ describe(

it("5. Verify Default search text in table as per 'Default search text' property set + Bug 12228", () => {
EditorNavigation.SelectEntityByName("Table1", EntityType.Widget);
propPane.ExpandIfCollapsedSection("search\\&filters");
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

Please review the escape characters in 'search\&filters'

It appears you're using an extra backslash in "search\&filters". Ensure this is intentional, as unnecessary escape characters might lead to issues. If the section name is "search&filters", you can write it without the double backslash.

Proposed change:

-propPane.ExpandIfCollapsedSection("search\\&filters");
+propPane.ExpandIfCollapsedSection("search&filters");
📝 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
propPane.ExpandIfCollapsedSection("search\\&filters");
propPane.ExpandIfCollapsedSection("search&filters");

@@ -171,6 +176,7 @@

it("6. Validate Search table with Client Side Search enabled & disabled & onSearchTextChanged is set", () => {
EditorNavigation.SelectEntityByName("Table1", EntityType.Widget);
propPane.ExpandIfCollapsedSection("search\\&filters");
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

Confirm the necessity of the escape character in 'search\&filters'

Double-check whether the backslash in "search\&filters" is necessary. Using unnecessary escape characters can cause confusion and potential errors.

Suggested change:

-propPane.ExpandIfCollapsedSection("search\\&filters");
+propPane.ExpandIfCollapsedSection("search&filters");
📝 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
propPane.ExpandIfCollapsedSection("search\\&filters");
propPane.ExpandIfCollapsedSection("search&filters");

@rishabhrathod01
Copy link
Contributor

/build-deploy-preview skip-tests=true

Copy link

github-actions bot commented Oct 7, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11209651511.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 36609.
recreate: .

Copy link

github-actions bot commented Oct 7, 2024

Deploy-Preview-URL: https://ce-36609.dp.appsmith.com

Copy link
Contributor

@rishabhrathod01 rishabhrathod01 left a comment

Choose a reason for hiding this comment

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

Code LGTM

@rishabhrathod01 rishabhrathod01 merged commit 56e7f89 into release Oct 7, 2024
84 checks passed
@rishabhrathod01 rishabhrathod01 deleted the chore/airgap-remove-old-flags branch October 7, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Product Issues related to a specific integration ok-to-test Required label for CI Query & JS Pod Issues related to the query & JS Pod skip-changelog Adding this label to a PR prevents it from being listed in the changelog Task A simple Todo Tech Debt Issues or Tasks which are tech debts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Removed the old flags from the codebase which have been rolled out
2 participants