-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix flaky tests #2812
Fix flaky tests #2812
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.
Seems like its failing CI again
Yep, I ran the CI one more time and it failed, will keep trying to figure this out, I still don't know what's causing the issues. |
test/utils/clickCenter.ts
Outdated
@@ -1,8 +1,6 @@ | |||
import { Page, Locator, expect } from '@playwright/test'; | |||
|
|||
export default async function clickCenter(page: Page, targetLocator: Locator) { | |||
await expect(targetLocator).toBeAttached(); | |||
|
|||
const targetBoundingBox = await targetLocator.boundingBox(); |
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.
maybe try something like
await expect(async () => {
const targetBoundingBox = await targetLocator.boundingBox();
expect(targetBoundingBox).toBeTruthy();
expect(targetBoundingBox.width).toBeGreaterThan(0)
expect(targetBoundingBox.height).toBeGreaterThan(0)
}).toPass();
(writing on my phone)
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.
My latest commit seems to fix the flaky tests (partially if not fully, the only one I'm not sure about is can create new custom components
, we can see after merging).
I had added that line before as an attempt, but I removed it in that last commit because it wasn't doing anything, the problems were unrelated to that logic there it seems.
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 didn't know about that awaitable .toPass
though, actually I'll add it too!
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.
with these fixes, is the container in the datagrid still necessary?
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.
Maybe not, they seemed to make the data providers
test more stable before the other fixes but it could have been just a coincidence. (actually I haven't seen those tests fail again since I changed that)
But these changes will also be needed for the upcoming resizing improvements, that's also why I tried getting them in anyway. And I've seen some issues with the grid height not matching the dotted highlight borders before.
If there's a reason why we're using absolute position I can revert that.
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 just wouldn't expect to find a container component there. feels more like a top level thing, given that its behavior depends on viewport width
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.
Ah, yeah Container
in components isn't the most appropriate, I've changed all components with a Container
to a Box
with 100% width and reverted the DataGrid until if we actually see that we need any changes there.
…t DataGrid component changes
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.
👌Thanks for taking this up!
Includes replacing
Container
withBox
in some components, unrelated but it seems likeContainer
is more appropriate for page-wide wrappers.