-
Notifications
You must be signed in to change notification settings - Fork 42
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
fixes panic for the selectOption #1552
Conversation
common/tests/cmd_test.go
Outdated
const page = await browser.newPage(); | ||
await page.goto('https://test.k6.io/browser.php'); | ||
const options = page.locator('#numbers-options'); | ||
await options.selectOption({label:'Five'}); |
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.
@ankur22 @inancgumus any tips on how to better test this keeping in mind the current test structure?
I took a look into the tests
package and not sure what could be the best way to emulate input like that (and below).
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.
Basically, I've created this to be like the ones that k6-core uses.
My predominant concern is why it should start from JS is because partially the panic
was caused because of some "magic" sobek type transitioning, and I'd like to keep it as part of the test, to ensure that it works
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 really like how you wrote the test, it's much more succinct to how we do it.
If we can't sort the race condition out, then take a look at a test that works with startIteration
, such as TestPageOnMetric.
407f2ef
to
cdc2413
Compare
8cbfc57
to
2223948
Compare
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 🚀
What?
This PR aims to fix the panic from #1415, by trying to stick with the types that we explicitly mention in our documentation https://grafana.com/docs/k6/latest/javascript-api/k6-browser/locator/selectoption/
Why?
We shouldn't panic
Checklist
Related PR(s)/Issue(s)
Closes #1415
Fixes #grafana/k6#4071