test(modals): fix flaky e2e #1366
Conversation
Summary of ChangesHello @spike-rabbit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the stability of end-to-end tests for modal dialogs, particularly resolving flakiness encountered during drag-and-drop interactions. The changes involve refining the test steps to ensure more robust and consistent simulation of user actions, leading to more reliable test outcomes and updated snapshots. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix a flaky end-to-end test for modal dialogs by adjusting the mouse movements during a drag-and-drop operation and adding an assertion to verify the element's final position. While the added assertion is a good step towards making the test more robust, the use of mouse 'jiggling' is a workaround that should be avoided. My feedback focuses on improving the test's stability and maintainability by removing this workaround and addressing the use of magic numbers.
|
Documentation. Coverage Reports: |
spike-rabbit
force-pushed
the
test/modals/flaky-e2e-fix
branch
2 times, most recently
from
c8a129f to
0de65e2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix a flaky E2E test for modals by ensuring elements are in their correct positions before interactions and assertions. The approach involves adding a new custom Playwright matcher, toHaveBoundingBox, to poll for an element's position. The changes are logical and should resolve the flakiness. I've provided a couple of suggestions to improve the new test helper's robustness and to address the maintainability of using hardcoded coordinates in the test.
spike-rabbit
force-pushed
the
test/modals/flaky-e2e-fix
branch
from
0de65e2 to
727e006
Compare
The root problem is, that after creation the modal is not immediately at the correct location. Probably due CDK that needs to calculate first. By adding an expect that ensures that the modal is at the correct location we solve the issue. In addition, we also need to verify the location of the cdk-drag-preview to make sure it is updated before the screenshot is taken.
spike-rabbit
force-pushed
the
test/modals/flaky-e2e-fix
branch
from
727e006 to
c0d8846
Compare
|
|
| await page.getByText('Many columns').first().click(); | ||
| await expect(page.locator('.modal-body')).toBeVisible(); | ||
| // Ensure everything is in place for the mouse interactions. | ||
| await expect(page.locator('.modal-body')).toHaveBoundingBox({ x: 274, y: 116 }); |
There was a problem hiding this comment.
this is a pain to maintain and write tests for as you need to know the exact box. So this is a no-no for me
Can't this be instead something more generic? I.e. what is the initial incorrect position so we can test for not being that
There was a problem hiding this comment.
Not really. On my machine the initial coordinates were varying.
In the end we need to wait here until the modal reached its position.
Usually I would say just disabling the animation and checking for visibility should also be fine. But I did not get this working. Maybe you find something better?
There was a problem hiding this comment.
Playwright let you know the correct numbers in case those are off. So maintenance is not hard.
Unless you find better way, I would be in favor of just merging it. In my opinion this is at least an improvement to before.
The root problem is, that after creation the modal is not immediately at the correct location.
Probably due CDK that needs to calculate first.
By adding an expect that ensures that the modal is at the correct location we solve the issue.
In addition we also need to verify the location of the cdk-drag-preview to make sure it is updated before
the screenshot is taken.