-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: Anvil on canvas UI tests #35342
Conversation
WalkthroughThe recent changes introduce a comprehensive testing suite for the Anvil editor's canvas UI, significantly enhancing both unit and end-to-end tests. Key functionalities, including widget interactions and visual updates, are thoroughly validated. With an emphasis on accessibility and user experience, the adjustments improve responsiveness and maintainability through refined component behaviors and new testing methods. Changes
Assessment against linked issues
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
@sagar-qa007 @ApekshaBhosale Please review the cypress test. I've followed some existing patterns used by existing tests. No new waits. Individual tests cleanup in the tests themselves, making them idempotent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts (1)
149-170
: Consider removing agHelper.Sleep for better test performance.The
MoveSelectedAnvilWidget
method usesagHelper.Sleep
, which can slow down tests and make them less reliable. Consider using alternative strategies to wait for necessary conditions, such as checking for specific DOM changes.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts (1 hunks)
- app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts (1 hunks)
- app/client/cypress/support/Pages/Anvil/Locators/index.ts (2 hunks)
- app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/AnvilWidgetNameComponent.test.tsx (1 hunks)
- app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/AnvilWidgetNameComponent.tsx (2 hunks)
- app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/SplitButton.test.tsx (1 hunks)
- app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/SplitButton.tsx (1 hunks)
- app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/index.test.tsx (1 hunks)
- app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/index.tsx (4 hunks)
Additional context used
Path-based instructions (3)
app/client/cypress/support/Pages/Anvil/Locators/index.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/Anvil/AnvilOnCanvasUI_spec.ts (1)
Pattern
app/client/cypress/**/**.*
: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.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 not posted (27)
app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/AnvilWidgetNameComponent.test.tsx (2)
25-32
: Great job on the rendering test!This test correctly verifies that the
AnvilWidgetNameComponent
renders with the expected text. It uses best practices by employingscreen
andrender
from React Testing Library.
34-54
: Excellent work on testing the drag event!This test effectively simulates a drag event and verifies that the event handler is called. It uses
jest.spyOn
to track the function call, which is a good practice.app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/SplitButton.test.tsx (5)
30-36
: Well done on verifying rendering and styling!This test ensures that the
SplitButton
renders with the correct text and style. UsingtoHaveStyle
is a good approach to verify CSS properties.
38-46
: Great job on testing the click event!This test correctly simulates a click event and verifies that the event handler is called. Using
userEvent.click
is a good practice for simulating user interactions.
48-54
: Nice work on the left toggle click test!This test accurately checks the left toggle's click functionality and ensures the handler is called. It follows best practices for interaction testing.
56-62
: Great job on the right toggle click test!This test ensures the right toggle's click functionality is working and the handler is called. It uses best practices for interaction testing.
64-77
: Excellent test for conditional rendering!This test correctly verifies that the toggles are hidden when the
disable
prop is true. Checking for the absence of elements is a good approach for testing conditional rendering.app/client/cypress/support/Pages/Anvil/Locators/index.ts (1)
34-37
: Great addition of the on-canvas UI selectors!The
anvilOnCanvasUISelectors
are well-defined using data-testid attributes, which is a best practice for reliable and maintainable selectors. Their integration intoanvilLocators
is seamless and enhances functionality.app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/AnvilWidgetNameComponent.tsx (4)
64-64
: Ensure all necessary dependencies are included.The
useCallback
hook forhandleSelectParent
now includesselectWidget
in its dependency array, which is a good practice to prevent stale closures. Make sure all dependencies that are used within the callback are included.
72-72
: Verify dependency inclusion forhandleSelectWidget
.The
useCallback
hook forhandleSelectWidget
correctly includesselectWidget
andprops.widgetId
in its dependency array. This ensures the latest values are used when the callback is invoked.
76-76
: CheckuseCallback
dependencies forhandleDebugClick
.Including
dispatch
andprops.widgetId
in the dependency array ensures that thehandleDebugClick
function always references the latest values. This is a good practice for maintaining correct behavior.
97-97
: Addition ofdata-testid
attribute.The
data-testid
attribute enhances the testability of the component by providing a stable selector for automated tests. This is a valuable addition for improving test coverage and reliability.app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/SplitButton.tsx (4)
120-120
: Addition ofdata-testid
forSplitButtonWrapper
.The
data-testid
attribute provides a stable reference for testing theSplitButton
component. This is a good practice for improving test reliability.
126-126
: Testability enhancement withdata-testid
for left toggle.Adding
data-testid
to the left toggle span facilitates automated testing by providing a consistent selector. This aligns with best practices for testable UI components.
134-134
: Improved testability withdata-testid
for clickable button.The
data-testid
attribute on the button element enhances its identification in tests, supporting robust and maintainable test scripts.
143-143
: Testability improvement withdata-testid
for right toggle.The addition of
data-testid
to the right toggle span aids in maintaining consistent and reliable test scripts.app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/index.tsx (5)
1-1
: Introduction ofuseRef
for cleanup function.Using
useRef
to store the cleanup function ensures a consistent function reference across renders. This is a more React-friendly approach and improves the reliability of side-effect management.
90-90
: Use ofuseRef
for cleanup logic.The
cleanup
function is now stored in auseRef
, which is a best practice for maintaining a stable reference to the cleanup logic. This change enhances the component's state management.
99-99
: Assignment tocleanup.current
.Assigning the result of
handleWidgetUpdate
tocleanup.current
ensures that the latest cleanup logic is preserved. This approach is effective for managing side effects.
107-107
: Invocation ofcleanup.current
.Calling
cleanup.current()
in theuseEffect
cleanup phase ensures that the latest cleanup function is executed. This maintains the integrity of the component's lifecycle management.
110-114
: Expanded dependency array foruseEffect
.Including
nameComponentState
,widgetElement
,widgetNameComponent
, andwidgetsEditorElement
in the dependency array ensures that the effect re-runs when these values change. This improves the component's responsiveness to state changes.app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts (2)
17-88
: Effective use of Cypress for UI testing.The test case effectively verifies the visibility and positioning of the widget name component. It uses helper functions for interactions and assertions, which enhances readability and maintainability. Ensure that all selectors use data-* attributes for consistency.
89-142
: Good practice in testing multiple widget selections.The test case correctly verifies the visibility of multiple widget names. Ensure that all selectors are based on data-* attributes to align with best practices.
app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/index.test.tsx (4)
78-105
: Comprehensive test for component visibility.The test case effectively verifies the visibility and elements of the
AnvilWidgetName
component. Ensure that all possible states are tested for completeness.
107-125
: Clear test for component absence.The test accurately verifies that the
AnvilWidgetName
component is not displayed when not focused or selected. This is a good practice to ensure correct component behavior.
127-154
: Effective test for error state visibility.The test case correctly checks for the presence of an error toggle when errors exist. Ensure that different error scenarios are covered for comprehensive testing.
156-183
: Thorough test for disabling parent toggle.The test case effectively verifies that the parent toggle is not shown when disabled. This ensures the component behaves as expected under various conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts (1 hunks)
- app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts
- app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts
app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts
changes look good to me @riodeuno Have you run the nest test on EE as well? |
@ApekshaBhosale Ran it 5 times (1 using all tests, 4 using limited test) on EE. All runs passed. I'm merging this in. |
Description
Unit
index.test.tsx
- (Main connected component that uses floating ui)AnvilWidgetNameComponent.test.tsx
(Standalone component that renders content for floating ui to use)onDragStart
callback from the standalone widget name component.SplitButton.test.tsx
(Low level UI component - child ofAnvilWidgetNameComponent
)E2E
Fixes #33979
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/10349986042
Commit: 8c9e6b9
Cypress dashboard.
Tags:
@tag.All
Spec:
Mon, 12 Aug 2024 11:13:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
AnvilWidgetNameComponent
andSplitButton
components to ensure expected functionality.Bug Fixes
AnvilWidgetName
component to state changes.Documentation