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

fix: Properly translate desiredCapabilities into a command line argument #1337

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

mykola-mokhnach
Copy link
Contributor

Types of changes

  • No changes in production code.
  • 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)

Details

Addresses #1336

@mykola-mokhnach
Copy link
Contributor Author

@SrinivasanTarget Could you please rerun the E2E tests? I've just patched the UIA2 driver

@SrinivasanTarget
Copy link
Member

@SrinivasanTarget Could you please rerun the E2E tests? I've just patched the UIA2 driver

Retriggered now

@mykola-mokhnach mykola-mokhnach merged commit 5af08a1 into appium:master Apr 23, 2020
@mykola-mokhnach mykola-mokhnach deleted the args_map branch April 23, 2020 06:49
@root-intruder
Copy link
Contributor

@mykola-mokhnach I also recently stumbled across this. With your fix, it seems that no arguments are escaped with \" which means that it always has to be manually escaped for every capability.

I see why a fix was needed, but that makes the situation worse for all windows users using mostly common simple capabilities such as "app" or "udid".

Is there any mechanism to work against this problem, without now quoting each arguments I pass to the capabilities.

Thanks!

@mykola-mokhnach
Copy link
Contributor Author

@root-intruder Maybe there is, but I don't know about it

@root-intruder
Copy link
Contributor

@mykola-mokhnach thanks for the answer! How about overloading public AppiumServiceBuilder withCapabilities(DesiredCapabilities capabilities)" as public AppiumServiceBuilder withCapabilities(DesiredCapabilities capabilities, boolean quoteAllCapabilities)

and then quote everything in private String capabilitiesToCmdlineArg()

if quoteAllCapabilities is set?!

Then still the user could decide if he want's to quote everything.

Alternatively it would also be possible to make quoting everything the default on windows systems and only disable it if the user sets the flag. This way, most win users wouldn't have to escape by hand and special cases like #1336 can still be addressed.

Cheers!

@mykola-mokhnach
Copy link
Contributor Author

The overload proposal seems reasonable to me. Feel free to create a pull request

@root-intruder
Copy link
Contributor

@mykola-mokhnach would you be fine with the described second option: quoting everything on win systems and the user needs to opt out and then manually quote in cases as described in #1336?

I'd do a pull request in the next days!

@mykola-mokhnach
Copy link
Contributor Author

would you be fine with the described second option: quoting everything on win systems and the user needs to opt out and then manually quote in cases as described in #1336?

I would rather stick to the first option, since changing the default behaviour is a breaking change. Clients must explicitly opt in for "automatic" quoting if they want to

@root-intruder
Copy link
Contributor

you're absolutely right...didn't think of existing users

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants