Skip to content
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

Editor integration tests #831

Merged
merged 31 commits into from
Sep 19, 2022
Merged

Editor integration tests #831

merged 31 commits into from
Sep 19, 2022

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Aug 19, 2022

Closes #758

Refactored to use Playright's Page Object Model as we intended.

@render
Copy link

render bot commented Aug 19, 2022

@oliviertassinari oliviertassinari requested a deployment to editor-integration-test - toolpad-db PR #831 August 19, 2022 17:24 — with Render Abandoned
@apedroferreira apedroferreira force-pushed the editor-integration-test branch from ffd1ae5 to c333d82 Compare August 19, 2022 17:32
@apedroferreira apedroferreira force-pushed the editor-integration-test branch from c333d82 to 3b5c0a9 Compare August 19, 2022 17:34
@apedroferreira apedroferreira force-pushed the editor-integration-test branch from 3b5c0a9 to a13efd4 Compare August 19, 2022 17:35
@apedroferreira apedroferreira changed the title Editor integration test Editor integration tests Aug 19, 2022
@apedroferreira apedroferreira self-assigned this Aug 22, 2022
@apedroferreira apedroferreira marked this pull request as ready for review August 22, 2022 18:22
@apedroferreira
Copy link
Member Author

apedroferreira commented Aug 22, 2022

Looks like we have to use the overlay element to capture clicks/drags, as it's the one that captures them. It doesn't work with other elements in the same position.
Also I don't plan to add lots of test ids but I think that for these specific elements it was the best choice.

Let me know if you think anything should be different in terms of how i reorganized some files and their structure a bit too!

@apedroferreira
Copy link
Member Author

Will add a couple of tests for prop controls in a separate PR now too.

@apedroferreira apedroferreira requested a review from Janpot August 23, 2022 16:29
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 24, 2022
* Add prop controls integration test

* Add TOOLPAD_CREATE_WITH_DOM environment variable in CI

* Rebase in editor tests

* Change tests to avoid text field bug

* Another env var attempt (-.-) (last attempt or I'll ask)

* Attempting to fix Firefox tests

* Attempt for Docker test configuration
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 25, 2022
.circleci/config.yml Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari requested a deployment to editor-integration-test - toolpad-db PR #831 September 5, 2022 18:48 — with Render Abandoned
@apedroferreira apedroferreira force-pushed the editor-integration-test branch from d6ddd09 to 1e763da Compare September 5, 2022 20:39
@apedroferreira
Copy link
Member Author

@Janpot
CI passed 3 times in a row, maybe it's ok to merge this now?
i had to dispatch events manually in Firefox, couldn't find a better way.
If these tests start being flaky later though we can skip them temporarily and i can work on them a bit more - locally they always pass for me but CI seems a bit more unpredictable.

@Janpot
Copy link
Member

Janpot commented Sep 9, 2022

Is there a ticket in the playwright repo that we can link to that describes the faulty behavior in firefox? If not, have you tried creating a minimal reproduction? Perhaps you can open a ticket and link to it?

I don't like to add hacks for bugs without adding documentation that can tell us more about the status of said bug. (example: https://github.com/mui/mui-toolpad/blob/4f71c00b28c8e57d9bad5ff99ede85f1d1aecc45/packages/toolpad-app/src/components/SplitPane.tsx#L58)

When there's a proper way forward towards solving the firefox issue in the future, then I'm fine merging this.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2022
@apedroferreira
Copy link
Member Author

apedroferreira commented Sep 9, 2022

Is there a ticket in the playwright repo that we can link to that describes the faulty behavior in firefox? If not, have you tried creating a minimal reproduction? Perhaps you can open a ticket and link to it?

I don't like to add hacks for bugs without adding documentation that can tell us more about the status of said bug. (example:

https://github.com/mui/mui-toolpad/blob/4f71c00b28c8e57d9bad5ff99ede85f1d1aecc45/packages/toolpad-app/src/components/SplitPane.tsx#L58

)
When there's a proper way forward towards solving the firefox issue in the future, then I'm fine merging this.

i couldn't find a proper related issue in the Playwright repo, and yeah i thought about creating one myself, just takes a bit of work to create a reproduction. i'm working on it, there's probably no rush to merge this so we can wait

@render
Copy link

render bot commented Sep 19, 2022

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 19, 2022
@apedroferreira
Copy link
Member Author

Is there a ticket in the playwright repo that we can link to that describes the faulty behavior in firefox? If not, have you tried creating a minimal reproduction? Perhaps you can open a ticket and link to it?

I don't like to add hacks for bugs without adding documentation that can tell us more about the status of said bug. (example:

https://github.com/mui/mui-toolpad/blob/4f71c00b28c8e57d9bad5ff99ede85f1d1aecc45/packages/toolpad-app/src/components/SplitPane.tsx#L58

)
When there's a proper way forward towards solving the firefox issue in the future, then I'm fine merging this.

I've managed to reproduce the bug in a minimal reproduction:
microsoft/playwright#17441

So it's confirmed, seems like there's a specific issue with triggering HTML5 drag & drop events in iFrames, in Firefox only.

I've included a link to the issue I created now, so will merge this PR if the tests seem stable in CI.

@apedroferreira apedroferreira merged commit bbaaec7 into master Sep 19, 2022
@apedroferreira apedroferreira deleted the editor-integration-test branch September 19, 2022 17:02
@Janpot
Copy link
Member

Janpot commented Sep 20, 2022

great 👌

@Janpot
Copy link
Member

Janpot commented Oct 4, 2022

@apedroferreira Following up on the playwright issue, it seems that it's closed due to the closing of microsoft/playwright#17153 (would have been nice of them to reference that issue). Does calling hover() twice fix the issue?

@apedroferreira
Copy link
Member Author

@apedroferreira Following up on the playwright issue, it seems that it's closed due to the closing of microsoft/playwright#17153 (would have been nice of them to reference that issue). Does calling hover() twice fix the issue?

looks like they're supposed to have fixed it, i'll try removing the Firefox specific logic and see if things work now

@apedroferreira
Copy link
Member Author

or i guess they might not have fixed it after all?
i'll try things from that issue then, maybe using hover twice works

@zannager zannager added Toolpad scope: toolpad-studio Abbreviated to "studio" labels Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: toolpad-studio Abbreviated to "studio" Toolpad
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration test for the application editor
4 participants