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

Move tag list tests to proper browsers interactions #3791

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

bearfriend
Copy link
Contributor

Uses proper browser mouse/keyboard interactions from the testing library instead of javascript methods/events to simulate them.

@bearfriend bearfriend marked this pull request as ready for review June 28, 2023 18:41
@bearfriend bearfriend requested a review from a team as a code owner June 28, 2023 18:41
@github-actions
Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://pr-3791-brightspace-ui-core.d2l.dev/

Note
The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

const button = elem.shadowRoot.querySelector('d2l-button-subtle.d2l-tag-list-clear-button');

setTimeout(() => button.click());
clickElem(button);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want to do this kind of thing in a timeout in case the event gets fired synchronously from the click handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't getting any flake locally without it, and I think it might always be safe to do this without, but it'd be good to know definitively either way before we convert/write more of these. I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if it's not safe, maybe we wrap them so it is always safe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah maybe it makes sense for us to put the setTimeout inside all of these utilities eh? The Promises they return can still resolve after the action has complete if that's what they're going for, but if you don't care you can just call it and then await an event on the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this and discussed offline. There are multiple steps in other processes that make these interactions inherently significantly delayed (in the event loop, not actually time) so for now we're thinking these should be safe to run in this order without intervention, but we can always build in a delay later if needed.

@bearfriend bearfriend merged commit fcc4810 into main Jun 30, 2023
@bearfriend bearfriend deleted the dgleckler/tag-list-browser-interaction branch June 30, 2023 14:52
@ghost
Copy link

ghost commented Jul 7, 2023

🎉 This PR is included in version 2.132.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants