Skip to content
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

when if connectExisting passed to driver then args shouldn't include websocketPort #509

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

b68h4
Copy link
Contributor

@b68h4 b68h4 commented Aug 11, 2024

./geckodriver: error: the argument '--websocket-port <PORT>' cannot be used with '--connect-existing'

If connectExisting and websocketPort passed same time to geckodriver cause crash because driver can't initialize websocket socket on existing browser

It's a minimal fix. I'm open to typo commits or other suggestions

@b68h4 b68h4 changed the title when if connectExisting passed to args then args shouldn't include websocketPort when if connectExisting passed to driver then args shouldn't include websocketPort Aug 11, 2024
@christian-bromann
Copy link
Contributor

I am not sure if it is a good idea to just omit the parameter. I would be open to throw a better error message to help the user understand why both parameters can not be set together.

@b68h4
Copy link
Contributor Author

b68h4 commented Aug 15, 2024

I am not sure if it is a good idea to just omit the parameter. I would be open to throw a better error message to help the user understand why both parameters can not be set together.

I pushed new version of fix

@christian-bromann
Copy link
Contributor

christian-bromann commented Aug 15, 2024

How is this now different from the current behavior? In both cases it would fail with an error warning saying that these two parameters can not be used together, right?

@b68h4
Copy link
Contributor Author

b68h4 commented Aug 16, 2024

How is this now different from the current behavior? In both cases it would fail with an error warning saying that these two parameters can not be used together, right?

No, if 2 parameters are given at the same time, an error is given, but if 2 parameters are not given, it automatically assigns 0 to the websocketPort. The problem was that even if connectExisting was given, geckodriver was crashing because 0 was assigned to the websocketPort.

This is a throws an error for existing browser connection:

const geckoDri = await start({
connectExisting:true,
websocketPort: 5961,
marionettePort: 5960,
port: 5959,
});

Or (problem is here):

const geckoDri = await start({
connectExisting:true,
marionettePort: 5960,
port: 5959,
});

It is a right usage for new browser launching:

const geckoDri = await start({
websocketPort: 5961,
marionettePort: 5960,
port: 5959,
})

If you connecting to existing browser, library shouldnt pass automatically websocketPort 0

Copy link
Contributor

@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.

Thanks for clarifying and providing the patch

LGTM 👍

@christian-bromann christian-bromann merged commit a4acdea into webdriverio-community:main Aug 16, 2024
5 checks passed
@wdio-bot
Copy link
Contributor

Hey b68h4 👋

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants