-
Notifications
You must be signed in to change notification settings - Fork 246
fix(e2e-tests): wait for diagram nodes to be visible COMPASS-9627 #7141
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
Conversation
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.
Pull Request Overview
This PR fixes a drag-and-drop operation in the e2e tests for the data modeling tab by changing how the pointer moves to the target node position.
- Simplifies the node positioning logic by using the element reference directly instead of calculating absolute coordinates
- Removes manual coordinate calculations that may have been causing positioning inaccuracies
| x: Math.round(startPosition.x + nodeSize.width / 2), | ||
| y: Math.round(startPosition.y + nodeSize.height / 2), | ||
| }) | ||
| .move({ origin: node, x: nodeSize.width / 2, y: nodeSize.height / 2 }) |
Copilot
AI
Jul 24, 2025
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.
The division operations nodeSize.width / 2 and nodeSize.height / 2 could result in fractional pixel values. Consider using Math.round() to ensure precise positioning, similar to the original implementation.
| .move({ origin: node, x: nodeSize.width / 2, y: nodeSize.height / 2 }) | |
| .move({ origin: node, x: Math.round(nodeSize.width / 2), y: Math.round(nodeSize.height / 2) }) |
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.
Copilot has a point here, I think I've seen errors before I added the rounding
|
The change makes sense to me, but I don't think this could've been causing the failure in Were there other failures? |
paula-stacho
left a comment
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.
Thank you! I honestly thought I'd already done this, must've removed it by accident
Ticket: COMPASS-9627
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes