fix: update column position check in TableV2 freeze column test #41130
Conversation
WalkthroughA 1000ms wait was added in a Cypress end-to-end test for the TableV2 widget before verifying the position of the custom frozen column "customColumn1" after updating table data. The expected column position was changed from 5 to 4. No other code or logic was modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js (1)
19-19: Replace hardcoded cy.wait() calls with dynamic waits.Multiple hardcoded
cy.wait()calls violate the coding guidelines. These make tests non-deterministic and unreliable.Replace with proper element waiting or use Appsmith's dynamic wait helpers:
- cy.wait(2000); + // Wait for table to be ready after drag and drop + cy.get('[data-testid="table-widget"]').should('be.visible'); - cy.wait(500); + // Wait for column position to stabilize + cy.get('[data-testid="table-header"]').should('be.visible'); - cy.wait(300); + // Wait for add column action to complete + cy.get('[data-testid="custom-column"]').should('exist');Also applies to: 33-33, 54-54, 77-77, 89-89, 100-100, 111-111
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js (1)
1-118: Good test structure with room for improvement.Positive aspects:
- Well-organized test cases with clear descriptions
- Proper use of tags and test hierarchy
- Good use of custom Cypress commands
Suggestions for enhancement:
- Consider extracting magic numbers (column positions) to constants
- Ensure all selectors use data-* attributes consistently
- Add more descriptive comments for complex test scenarios
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/**/**.*
⚙️ CodeRabbit Configuration File
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.
Files:
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js
🧠 Learnings (2)
📓 Common learnings
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: 2024-10-08T15:32:39.374Z
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.
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: 2024-07-26T21:12:57.228Z
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.
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js:35-37
Timestamp: 2024-11-13T09:11:36.959Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js`, when writing Cypress end-to-end tests for the Image Widget in JavaScript, use `viewWidgetsPage.imageinner` as the selector when asserting 'src' and 'alt' attributes to maintain consistency.
Learnt from: abhishek-bandameedi
PR: appsmithorg/appsmith#35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Learnt from: abhishek-bandameedi
PR: appsmithorg/appsmith#35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-07-24T08:29:41.208Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-07-16T06:44:55.118Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep`, and other related sleep functions in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:81-82
Timestamp: 2025-01-13T15:14:42.085Z
Learning: In Cypress test files, unsafe optional chaining is acceptable when the intention is for the test to fail if the expected data structure is not present, as it helps validate the API response structure.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:102-102
Timestamp: 2025-01-13T15:14:12.806Z
Learning: In Cypress Git sync tests, explicit assertions after _.gitSync.SwitchGitBranch() are not required as the helper method likely handles the verification internally.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:52-58
Timestamp: 2025-01-13T15:15:05.763Z
Learning: The Git sync helper functions (_.gitSync.CommitAndPush, _.gitSync.MergeToMaster) in Cypress tests already include built-in assertions, so additional explicit assertions after these calls are not needed.
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_spec.js:44-47
Timestamp: 2024-11-13T09:07:54.931Z
Learning: When reviewing code in `app/client/cypress/**` paths, ensure that suggestions are appropriate for test code and not for `src` code. Avoid applying source code checks to test code unless relevant.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js (7)
Learnt from: vhemery
PR: #37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js:35-37
Timestamp: 2024-11-13T09:11:36.959Z
Learning: In the file app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js, when writing Cypress end-to-end tests for the Image Widget in JavaScript, use viewWidgetsPage.imageinner as the selector when asserting 'src' and 'alt' attributes to maintain consistency.
Learnt from: sagar-qa007
PR: #34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: Learnt from: sagar-qa007
PR: #34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-07-16T06:44:55.118Z
Learning: Avoid using sleep functions like agHelper.Sleep, this.Sleep, and other related sleep functions in Cypress tests within the app/client/cypress directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
Learnt from: abhishek-bandameedi
PR: #35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Learnt from: abhishek-bandameedi
PR: #35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-07-24T08:29:41.208Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Learnt from: Aishwarya-U-R
PR: #29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
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.
Learnt from: Aishwarya-U-R
PR: #29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
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.
Learnt from: brayn003
PR: #38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:81-82
Timestamp: 2025-01-13T15:14:42.085Z
Learning: In Cypress test files, unsafe optional chaining is acceptable when the intention is for the test to fail if the expected data structure is not present, as it helps validate the API response structure.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js (1)
50-50: Test assertion fix looks correct.The position change from 5 to 4 aligns with the PR objective of fixing the freeze column test. This appears to be the correct expected position for the frozen custom column after data update.
|
/ci-test-limit-count run_count=20 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/16490527528. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/16490527528. |
|
/ci-test-limit-count run_count=20 update_snapshot=false specs_to_run=cypress/e2e/Regression/ClientSide/Widgets/TableV2/freeze_column_query_change_spec.js |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/16494125507. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/16494125507. |
Description
Problem
The freeze column test in TableV2 is failing due to incorrect column position checks.
Root cause
The test logic for verifying column positions does not accurately reflect the expected behavior after columns are frozen.
Solution
This PR handles updating the column position check logic in the TableV2 freeze column test to ensure it correctly validates the positions of columns after freezing.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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.Table"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/16494079609
Commit: b0496c5
Cypress dashboard.
Tags:
@tag.TableSpec:
Thu, 24 Jul 2025 10:56:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit