-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
feat: upgrade WebdriverIO to v9, drop JWP capabilities #2852
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 7203a0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
fbc632d
to
7203a0e
Compare
7203a0e
to
e755f6b
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.
Seems fine to me. Two questions and then likely good to go.
'@web/test-runner-webdriver': minor | ||
--- | ||
|
||
Upgrade WebdriverIO to v9, drop JWP capabilities |
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.
"Drop"ing something feels like it should be more than a minor
release, but I don't know Webdriver that well...
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.
Sure, but we are still in 0.x so technically every minor can contain breaking changes.
I wasn't sure whether I should use major
for this in the changeset.
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.
minor in 0.x is practically major, so is good
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.
Agreed. Thanks for clarifying.
|
||
function getPlatform(c: WebDriver.DesiredCapabilities): string | undefined { | ||
return c.platformName || c.platform; | ||
function getPlatform(c: WebdriverIO.Capabilities): string | undefined { |
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.
Where does this file get this type value from? If it works, it works, but confused that there is no import. The main question is if we're doing something special somewhere else, or relying on something that I can't see, then is it visible to a more experienced WebdriverIO user in a way that would prevent their consumption of this from causing them issues?
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 tried to use this but got error "Cannot use namespace 'Capabilities' as a type".
import type { Capabilities } from '@wdio/types';
The namespace is defined here, seems like it's done for supporting custom capabilities.
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.
Does it run fine in other projects this way. I'm fine with whatever works, it was just surprising to see and would be annoying if this caused some undocumented requirements on consumers.
@@ -0,0 +1,6 @@ | |||
--- | |||
'@web/test-runner-saucelabs': minor | |||
'@web/test-runner-webdriver': minor |
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.
shouldn't other packages be updated too?
from the top of my head it's the browserstack one
https://github.com/modernweb-dev/web/blob/master/packages/test-runner-browserstack/package.json
maybe some others, good to check where else the @web/test-runner-webdriver
is in deps
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.
Good catch, added @web/test-runner-browserstack
. This is the only remaining package that needs updating (other packages only use webdriver launcher as dev dependency).
What I did
webdriverio
to v9 and changed the saucelabs launcher accordinglyNote
Tested locally by running tests for
@web/test-runner-webdriver
package. These don't currently run in CI as they were skipped long ago, in particular due to webdriverio/selenium-standalone#788 - that issue is now fixed but unfortunately the fix landed in a version that dropped Node 18 support.