-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: add cypress tests for drag and drop building blocks #33036
feat: add cypress tests for drag and drop building blocks #33036
Conversation
…o feat/cypress-tests-for-drag-drop-building-blocks
…o feat/cypress-tests-for-drag-drop-building-blocks
…o feat/cypress-tests-for-drag-drop-building-blocks
WalkthroughWalkthroughThe changes focus on enhancing the drag-and-drop functionality for building blocks in the client-side application. This includes adding Cypress tests for automation, updating Redux actions and reducers to handle skeleton loaders, and modifying saga functions to manage widget additions and deletions. The updates ensure smoother interactions and better state management for building blocks. Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Warning Review ran into problemsProblems (1)
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 Configration File (
|
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: 2
it("2. If widgets are more than 9, see more button should be visible", () => { | ||
featureFlagIntercept({ release_drag_drop_building_blocks_enabled: true }); | ||
let widgetsInThisTag: string[] = []; | ||
|
||
agHelper | ||
.GetElement(`${entityExplorer._widgetTagBuildingBlocks}`) | ||
.each(($widgetTag) => { | ||
cy.wrap($widgetTag) | ||
.find(entityExplorer._widgetCardTitle) | ||
.each(($widgetName) => { | ||
const value = $widgetName.text(); | ||
widgetsInThisTag.push(value); | ||
}); | ||
}); | ||
|
||
if (widgetsInThisTag.length > MAX_BUILDING_BLOCKS_TO_DISPLAY) { | ||
cy.get(entityExplorer._widgetSeeMoreButton).should("be.visible"); | ||
} |
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.
Address the variable scope issue to prevent potential test failures.
The variable widgetsInThisTag
is defined outside the .each()
block but is used inside it, which might lead to incorrect behavior due to asynchronous execution. Consider defining it inside the .each()
block or using a different approach to ensure correct behavior.
it("3. Should not show the 'See More' button if widgets are less than 9", () => { | ||
featureFlagIntercept({ release_drag_drop_building_blocks_enabled: true }); | ||
|
||
// Array to store the names of widgets found | ||
const widgetsInThisTag: string[] = []; | ||
|
||
// Find and iterate over each widget card title within the specified tag | ||
cy.get(entityExplorer._widgetTagBuildingBlocks) | ||
.find(entityExplorer._widgetCardTitle) | ||
.each(($widgetName) => { | ||
// Extract the text of each widget title | ||
const value = $widgetName.text().trim(); | ||
widgetsInThisTag.push(value); | ||
}) | ||
.then(() => { | ||
// After collecting widget names, assert based on the count | ||
if (widgetsInThisTag.length < MAX_BUILDING_BLOCKS_TO_DISPLAY) { | ||
// If less than 9 widgets, ensure the 'See More' button does not exist | ||
cy.get(entityExplorer._widgetSeeMoreButton).should("not.exist"); | ||
} else { | ||
// If 9 or more widgets, ensure the 'See More' button exists | ||
cy.get(entityExplorer._widgetSeeMoreButton).should("exist"); | ||
} |
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.
Refactor to remove redundant conditions.
The conditions in lines 73 and 77 are redundant since they are mutually exclusive and handled in the same .then()
block. Consider simplifying the logic to enhance readability and maintainability.
app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Building_Blocks.ts
Outdated
Show resolved
Hide resolved
…o feat/cypress-tests-for-drag-drop-building-blocks
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.
@jacquesikot We are again mixing scopes of 2 prs into single one.
Split this one into 2:
- just the cypress test.
- performance enhancement by not saving skeleton loader.
Also checkout Code rabbits suggestions.. they are good.
@@ -84,7 +84,8 @@ function WidgetCardComponent({ | |||
onDragStart?: (e: any) => void; | |||
}) { | |||
const type = `${details.type.split("_").join("").toLowerCase()}`; | |||
const className = `t--widget-card-draggable t--widget-card-draggable-${type}`; | |||
const displayName = details.displayName.split(" ").join("").toLowerCase(); | |||
const className = `t--widget-card-draggable t--widget-card-draggable-${type} ${details.type === BUILDING_BLOCK_EXPLORER_TYPE ? `t--widget-card-draggable-${type}-${displayName}` : ""}`; |
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.
I know the current implementation uses class names, however can we pls pass the selector only needed for testing as a data attribute instead of a class name.
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.
This change has been made
…o feat/cypress-tests-for-drag-drop-building-blocks
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.
since we've added a new spec, ran ok-to-test
once again to make sure this passes.
Description
Tip
Adds the following cypress tests suite and cases for building blocks drag and drop
Building_Blocks.ts
Fixes #32961
Automation
/ok-to-test tags="@tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9153757681
Commit: 07eaa5e
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit