Skip to content

Allows reloadSession to connect to remote driver#12957

Merged
christian-bromann merged 1 commit intowebdriverio:v8from
ccharnkij:reloadsession
Jun 17, 2024
Merged

Allows reloadSession to connect to remote driver#12957
christian-bromann merged 1 commit intowebdriverio:v8from
ccharnkij:reloadsession

Conversation

@ccharnkij
Copy link
Copy Markdown
Contributor

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

  • Polish (an improvement for an existing feature)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (improvements to the project's docs)
  • Specification changes (updates to WebDriver command specifications)
  • Internal updates (everything related to internal scripts, governance documentation and CI files)

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

@christian-bromann
Copy link
Copy Markdown
Member

@ccharnkij thanks for raising an PR. Would it be possible to provide a reproducible example to validate this bug first?

@ccharnkij
Copy link
Copy Markdown
Contributor Author

@christian-bromann Yeah, here is a sample one: https://github.com/ccharnkij/v8-reload-example. When you run npm test, you should see Error: Invalid or unsupported WebDriver capabilities found ("hostname", "port").

Side note, I'm still not sure why github action would fail on bootstrap part. I have not touched @wdio/types.

@ccharnkij
Copy link
Copy Markdown
Contributor Author

Hi @christian-bromann Have you a chance to check this out?

Copy link
Copy Markdown
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed review, I had a lot of competing priorities and only got a chance now.

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 }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];
    }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better?

Copy link
Copy Markdown
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more comment, can we also add some docs in the reloadSession command for this?

@ccharnkij
Copy link
Copy Markdown
Contributor Author

If all is well, let me know if you'd like me to make the same change to main too.

@christian-bromann
Copy link
Copy Markdown
Member

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?

@ccharnkij
Copy link
Copy Markdown
Contributor Author

like this?

Comment on lines +16 to +17
* in a situation, for example, where you start a test in native app and need to verify
* data in web app.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for understanding: this would completely get rid of the mobile session so you couldn't continue anything on the native app side, correct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@christian-bromann
Copy link
Copy Markdown
Member

Mind raising a PR for main?

@ccharnkij
Copy link
Copy Markdown
Contributor Author

Will do. Thank you

@christian-bromann christian-bromann added the PR: Polish 💅 PRs that contain improvements on existing features label Jun 17, 2024
@christian-bromann christian-bromann merged commit 8f7720f into webdriverio:v8 Jun 17, 2024
@wdio-bot
Copy link
Copy Markdown
Contributor

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,
The WebdriverIO Team 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Expensable $35 💸 PR: Polish 💅 PRs that contain improvements on existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants