-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: When no argument is passed to the .click interaction command, the webdriver command should also have no argument #8937
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
Conversation
0250506 to
4b82d43
Compare
| context, | ||
| selector, | ||
| options = {}, | ||
| options, |
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.
Did you check what is passed here when = {} is removed? I think we always normalize it in the browser/*/locators/index.ts
Maybe a better test could catch 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.
ah good question, I'll double check at least by adding logs :)
I'm not sure how to do a good test, as they should essentially do the same thing in the end ... except with more work (possibly taking longer especially).
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.
or maybe I could try to mock the call to see what is really called...
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.
Yeah you're right of course:
| public click(options: UserEventClickOptions = {}): Promise<void> { |
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.
Playwright defaults to {} too, and the preview one doesn't take any option, so I guess we can remove the default everywhere.
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.
https://bugzilla.mozilla.org/show_bug.cgi?id=1456642 turns out there's a bug open in Firefox, it looks like it's actively worked on these days.
Given there's an easy workaround (passing {} as a parameter to click), I'll leave it up to you to decide if you want to merge 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.
I think if we can add a documentation notice to the click event, it's fine to merge. The impact should not be big
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 pushed some documentation, tell me if that looks like what you expected.
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.
Yeah, looks good to me, just make sure eslint doesn't fail
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.
oh no, should be fixed now :-)
|
FWIW this is the webdriver.io bug I was running into because of that: webdriverio/webdriverio#14840 |
test/browser/test/userEvent.test.ts
Outdated
|
|
||
| onClick.mockClear() | ||
| await userEvent.click(button, { button: 'right' }) | ||
| expect(onClick).toHaveBeenCalled() |
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.
Can you give me some hint about how to run this test only? I couldn't find it :/
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.
in test/core:
PROVIDER=webdriverio pnpm test-fixtures userEvent --run -t "correctly clicks a button"
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 guess you meant test/browser :)
I see my new test wasn't passing either, but I don't see that in the CI, are they running there?
Now I can fix all of that, thanks!
4b82d43 to
c11b560
Compare
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
It's not clear to me why the tests are failing :/ I tested locally the individual files, they pass for me, but they don't seem to pass when testing the runner... Though I'm on Linux, Linux passes too in CI, just not Windows and MacOS, so there might be something different there? |
c11b560 to
3787d5a
Compare
|
I reverted the changes on playwright because this wasn't related to this issue, I just wanted to make things consistent and I thought this would change nothing. But maybe I misread the playwright code. Let's see if this fixes the tests. Update: this didn't fix the test. |
…driver command should also have no argument Indeed webdriver.io has a different behavior when the argument is not undefined. Fixes vitest-dev#8931
3787d5a to
7e89e16
Compare
|
By preventing the context menu from appearing when clicking right, I fixed the tests, to make new failures appear, which make more sense. I wonder if that might be a bug in Firefox... It's still pretty weird that only macos fails, and not linux. (I don't know for windows yet) update: Ah of course, browser tests are not run on Ubuntu. |
…n using the Shift modifier
2515cb7 to
1342ef6
Compare
|
Thank you! |
|
Thanks! |
|
Is there a changelog to update about this, or will you add it to the release notes ? |
release notes are generated based on the squashed commit message |
|
Okay, just want to make sure the folks know what to do in case they see failures because of that. |
|
So this change actually avoided the bug I mentioned earlier webdriverio/webdriverio#14840 in Firefox... but now I get a new webdriver bug in Chrome, related to a web component wrapping a button element. Nothing related to vitest IMO, but I wanted to mention it in case other folks got the issue. |
|
Not related to vitest directly, but mentioning it in case this can be useful to other folks. |
Indeed webdriver.io has a different behavior when the argument is not undefined.
Fixes #8931
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.