Allows reloadSession to connect to remote driver#12957
Allows reloadSession to connect to remote driver#12957christian-bromann merged 1 commit intowebdriverio:v8from
Conversation
|
@ccharnkij thanks for raising an PR. Would it be possible to provide a reproducible example to validate this bug first? |
|
@christian-bromann Yeah, here is a sample one: https://github.com/ccharnkij/v8-reload-example. When you run npm test, you should see Side note, I'm still not sure why github action would fail on bootstrap part. I have not touched @wdio/types. |
|
Hi @christian-bromann Have you a chance to check this out? |
christian-bromann
left a comment
There was a problem hiding this comment.
Sorry for the delayed review, I had a lot of competing priorities and only got a chance now.
packages/webdriver/src/index.ts
Outdated
| const capabilities = deepmerge(instance.requestedCapabilities, newCapabilities || {}) | ||
| const params: Options.WebDriver = { ...instance.options, capabilities } | ||
| const capabilities = { ...sanitizeCaps(newCapabilities || {}, true), capabilities: sanitizeCaps(newCapabilities || instance.requestedCapabilities) } | ||
| const params: Options.WebDriver = { ...instance.options, ...capabilities } |
There was a problem hiding this comment.
This would merge capabilities into the params object which should be of type Options.WebDriver, can you explain your intention doing this? It feels like this is not what we want, right?
There was a problem hiding this comment.
Yeah, the merging is what I aimed for. Because of this error Error: Invalid or unsupported WebDriver capabilities found ("hostname", "port")., I need a way to separate out fields like host, port, etc.. from an object that is passed into reloadFunction and use them to overwrite the old values existed in instance.options. So, assuming you pass something like this in reloadFunction,
{
host: 'xxxx',
port: 1234,
browserName: 'chrome'
}
That top line would make capabilities field look like this
host: 'xxxx',
port: 1234,
capabilities: {
browserName: 'chrome'
}
And I can just use this to overwrite the corresponding value in instance.options.
Not sure if I answer your question, but let me know.
There was a problem hiding this comment.
I would suggest to just have some simple if statements and move the connection parameters into the params object, e.g.:
for (const prop of ['protocol', 'hostname', 'port', 'path'] as Option.Connection) {
if (prop in capabilities) {
params[prop] = capabilities[prop];
delete capabilities[prop];
}
}
christian-bromann
left a comment
There was a problem hiding this comment.
One more comment, can we also add some docs in the reloadSession command for this?
|
If all is well, let me know if you'd like me to make the same change to main too. |
|
Thanks @ccharnkij , on the documentation side, mind adding a sentence to the command docs, mentioning in which situations people may want to reload a session with different capabilities? |
|
like this? |
| * in a situation, for example, where you start a test in native app and need to verify | ||
| * data in web app. |
There was a problem hiding this comment.
Just for understanding: this would completely get rid of the mobile session so you couldn't continue anything on the native app side, correct?
There was a problem hiding this comment.
Yeah, I tested it out locally. This is not meant to be for concurrent session like multi-remote, if that's what you're asking. It's meant to be used within a single session. Btw, I made a small change again, I removed deepmerge because I don't want caps from original session to be mixed with a new one.
|
Mind raising a PR for |
|
Will do. Thank you |
|
Hey ccharnkij 👋 Thank you for your contribution to WebdriverIO! Your pull request has been marked as an "Expensable" contribution. We've sent you an email with further instructions on how to claim your expenses from our development fund. Please make sure to check your spam folder as well. If you have any questions, feel free to reach out to us at expense@webdriver.io or in the contributing channel on Discord. We are looking forward to more contributions from you in the future 🙌 Have a nice day, |
Proposed changes
This change allows reloadSession to connect to remote driver and vice versa. This is useful for some instances, such as when a test starts in mobile native app and then have to validate data in web.
Types of changes
Checklist
Further comments
Reviewers: @webdriverio/project-committers