When building a AppiumDriverLocalService, there is a Port number missmatch when providing --port argument #521

Activity

1. alexander-poulikakos commented on Nov 23, 2016

   

   alexander-poulikakos

   on Nov 23, 2016

   Last edited by alexander-poulikakos

   ContributorAuthor

   More actions

   It seems as these server flags gets a special treatment: --log, --port and --address. I think the withArgument(...) method should be able to handle those as well. The usecase in my case is I'm reading the appium server flags from a properties file and would prefer to treat them in the same generic way.

   Would modifying the withArgument(...) method to something like the below proposal be ok? If so, I can provide a pull request (with unit tests etc.) and we can continue discussing it from there.

       /**
        * @param argument is an instance which contains the argument name.
        * @param value    A non null string value. (Warn!!!) Boolean arguments have a special moment:
        *                 the presence of an arguments means "true". At this case an empty string
        *                 should be defined.
        * @return the self-reference.
        */
   	public AppiumServiceBuilder withArgument(ServerArgument argument, String value) {
   		String argName = argument.getArgument().trim();
   		if (argName.equals("--port") || argName.equals("-p")) {
   			usingPort(Integer.valueOf(value));
   		} else if (argName.equals("--address") || argName.equals("-a")) {
   			withIPAddress(value);
   		} else if (argName.equals("--log") || argName.equals("-g")) {
   			withLogFile(new File(value));
   		} else {
   			serverArguments.put(argName, value);
   		}
   		return this;
       }
2. TikhomirovSergey

   added

   invalid

   on Nov 23, 2016
3. TikhomirovSergey commented on Nov 23, 2016

   

   TikhomirovSergey

   on Nov 23, 2016

   Last edited by TikhomirovSergey

   Contributor

   More actions

   @alexander-poulikakos
   Please just use these methods

   usingPort()
   withIPAddress()
   withLogFile()
   

   Thse is the set of methods provided by DriverService (except withIPAddress)

   As I can read the log there is another problem

   [Appium] Welcome to Appium v1.6.1-beta
   [Appium] Non-default server args:
   [Appium]   port: 8998
   [Appium] Appium REST http interface listener started on 0.0.0.0:8998
   

   That means that the service was started successfully (!!!).
   But

   aused by: org.openqa.selenium.net.UrlChecker$TimeoutException: Timed out waiting for [http://0.0.0.0:4723/wd/hub/status] to be available after 120003 ms
   	at org.openqa.selenium.net.UrlChecker.waitUntilAvailable(UrlChecker.java:107)
   

   Do you see the difference? This is because it considers that the port was not provided so it uses the default value (4723)

   Actually current implementation does same things as you are going to propose. It adds port, log file and ip to arguments.
4. alexander-poulikakos commented on Nov 24, 2016

   

   alexander-poulikakos

   on Nov 24, 2016

   ContributorAuthor

   More actions

   thanks for your reply @TikhomirovSergey.

   My use case is that I am reading the arguments from a properties file, something like this:

   argument.--port = 8998
   argument.--log-level = info
   argument.--log = /path/to/log
   argument.--log-timestamp=
   

   and I would like to set them in a generic way for all arguments. Not make special cases for some of the arguments.

   Since the API allows any argument to be set through the ´withArgument(...)´ method, then it should work for all arguments, including --port.

   For example, a unit test like below should pass. Currently it is not.

       @Test public void checkAbilityToStartServiceWithPortUsingFlags() throws Exception {
       	String port = "8996";
       	String expectedUrl = String.format("http://0.0.0.0:%s/wd/hub", port);
       	
       	AppiumDriverLocalService service = new AppiumServiceBuilder()
       			.withArgument(() -> "--port", port)
       			.build();
   	String actualUrl = service.getUrl().toString();
   	assertEquals(expectedUrl, actualUrl);
       }

   The problem is that the ´DriverService´ does not get aware of the new port when using ´withArgument(() -> "--port", "...")´ method.

   Actually current implementation does same things as you are going to propose. It adds port, log file and ip to arguments.

   No, that is not the same thing. What I propose is to make ´DriverService´ aware of the new port when using ´withArgument(...)´ method.
5. TikhomirovSergey

   added

   enhancement

   and removed

   invalid

   on Nov 24, 2016
6. TikhomirovSergey commented on Nov 24, 2016

   

   TikhomirovSergey

   on Nov 24, 2016

   Last edited by TikhomirovSergey

   Contributor

   More actions

   @alexander-poulikakos Now I got your problem. Ok. You can propose the PR.

   My remark: Please use equalsIgnoreCase instead of equals
   Also don't forget to provide some tests.
7. TikhomirovSergey commented on Nov 25, 2016

   

   TikhomirovSergey

   on Nov 25, 2016

   Last edited by TikhomirovSergey

   Contributor

   More actions

   It was improved. It is going to be published soon.
8. alexander-poulikakos commented on Nov 25, 2016

   

   alexander-poulikakos

   on Nov 25, 2016

   ContributorAuthor

   More actions

   Just a minor comment: This calls the toLowerCase() method.

    String argName = argument.getArgument().trim().toLowerCase();

   So no need to call equalsIgnoreCase multiple (6) times in below code.

           String argName = argument.getArgument().trim().toLowerCase();
           if ("--port".equalsIgnoreCase(argName) || "-p".equalsIgnoreCase(argName)) {
               usingPort(Integer.valueOf(value));
           } else if ("--address".equalsIgnoreCase(argName) || "-a".equalsIgnoreCase(argName)) {
                withIPAddress(value);
           } else if ("--log".equalsIgnoreCase(argName) || "-g".equalsIgnoreCase(argName)) {
               withLogFile(new File(value));
           } else {
               serverArguments.put(argName, value);
           }

   Would be cleaner if equals is used :)

           String argName = argument.getArgument().trim().toLowerCase();
           if ("--port".equals(argName) || "-p".equals(argName)) {
               usingPort(Integer.valueOf(value));
           } else if ("--address".equals(argName) || "-a".equals(argName)) {
                withIPAddress(value);
           } else if ("--log".equals(argName) || "-g".equals(argName)) {
               withLogFile(new File(value));
           } else {
               serverArguments.put(argName, value);
           }
9. TikhomirovSergey commented on Nov 25, 2016

   

   TikhomirovSergey

   on Nov 25, 2016

   Contributor

   More actions

   ok. Will improve that.
10. TikhomirovSergey

    added a commit that references this issue on Nov 25, 2016

    Code optimization #521. README update.

    406da1f
11. TikhomirovSergey commented on Nov 25, 2016

    

    TikhomirovSergey

    on Nov 25, 2016

    Contributor

    More actions

    I'm closing this ticket.
12. TikhomirovSergey

    closed this as completedon Nov 25, 2016