-
Notifications
You must be signed in to change notification settings - Fork 418
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
test(core): add test to ensures that only one listener is open when navigating list #6569
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
No changes to documentation |
Component Testing Report Updated May 6, 2024 2:50 PM (UTC)
|
|
||
// We change the sort order to not be default, to ensure that the listener is not re-created | ||
await page.getByTestId('pane-context-menu-button').click() | ||
await page.getByRole('menuitem', {name: 'Sort by Name'}).click() |
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.
nit: do we have ids that we can use instead of names? just worry about maintenance of this
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.
It doesn't currently exists and it's probably little bit out of scope to do it as part of this PR, there is another task to add tests for this menu in general and it's probably better to refactor it part of that.
The only thing I would also say is when using getByRole
vs getByTestId
is that the role ensures that in future changes the a11y rules of a menu are not broken which will get obfuscated by using testid. Since these are default sort options and chances of them changing is less, IMO it's a balance of when to use testId vs using helpers that test functionality as well other HTML spec rules
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.
👍 LGTM
Description
Adds a test that only one listener is open when navigating document lists
What to review
Changes makes sense
Testing
I did write a test to assert that only one connection is kept open but it is not very ideal way but this is a limitation of playwright that it does not support connection close event for long polling request. Fortunately they get marked as failed requests when they get closed which helps us write some assertions around it.
Notes for release