-
Notifications
You must be signed in to change notification settings - Fork 189
feat: Migrate AppiumLocalService to Appium 2 #529
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
Conversation
@@ -58,7 +58,7 @@ public class AppiumLocalService : ICommandServer | |||
/// <summary> | |||
/// The base URL for the managed appium server. | |||
/// </summary> | |||
public Uri ServiceUrl => new Uri($"http://{IP.ToString()}:{Convert.ToString(Port)}/wd/hub"); | |||
public Uri ServiceUrl => new Uri($"http://{IP}:{Convert.ToString(Port)}"); |
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.
this will make the service fully incompatible with Appium 1. Also, one could provide --base-path/-bp argument to change this path. We should be able to support such approach. At least Python and Java clients do
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.
@mykola-mokhnach , already discussed the fact it will be incompatible with appium 1 with @akinsolb, this is the reason we will merge to release 5.0 branch.
I will try to look what I can do with the --base-path/-bp argument in order to support it fully.
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 didn't know the other bindings supported v1. It would be ideal if we could support both, where users can build their own client
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.
@akinsolb /@mykola-mokhnach Do you prefer I add the base path support as part of the arguments in the AppiumLocalService or it's better to create a dedicated var for it? just like we have IP and Port
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 don't have any strong pref there. Do it the way which is closer/more straightforward to dotnet client users. In java and python service implementations we fetch base path value from the corresponding CLI argument. /
is used by default.
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've pushed some changes to this PR so we will support base-path, but I still need to work on the ipv4 status issue.
Add Support for basePath argument Add BasePath to common Appium server arguments Add StatusUrl const to appiumService Add CreateStatusUrl function Update link to list of common Appium server arguments
Remove unused Exceptions variables Fix unused span parameter
Change list
-Removed the usage of /wd/hub which is no longer in use starting from appium2.0 server
-Minor code improvements at AppiumLocalService.cs
-Removal of redundant 'using' in test/integration/Properties/Resources.Designer.cs
Types of changes
What types of changes are you proposing/introducing to .NET client?
Put an
x
in the boxes that applyDocumentation
This can be done by navigating to the documentation section on http://appium.io selecting the appropriate command/endpoint and clicking the 'Edit this doc' link to update the C# example
Integration tests
Details
Please provide more details about changes if it is necessary. If there are new features you can provide code samples which show the way they
work and possible use cases. Also you can create gists with pasted C# code samples or put them here using markdown.
About markdown please read Mastering markdown and Writing on GitHub