Improve Appium support in Nightwatch

[garg3133]

This PR does the following:

• Automatically starts the Appium server on the specified port.
• Users need to set use_appium property to true in the selenium config of the Nightwatch configuration file when using Appium server. Ex. config:

  test_settings: {
    app_env: {
      selenium: {
        start_process: true,
        use_appium: true,  // <-- here
        port: 4723,
        // other properties
      },
      webdriver: {
        start_process: false
      },
      desiredCapabilities: {
        browserName: null,
        // other desired capabilities
      },
    }
  }

• Added support for Appium v2 (beta) as well. To use with Appium v2, set default_path_prefix property to '' in the selenium config of your Nightwatch configuration file:

  test_settings: {
   app_env: {
     selenium: {
       start_process: true,
       use_appium: true,
       port: 4723,
       default_path_prefix: '',  // <-- here
       // other properties
     },
     webdriver: {
       start_process: false
     },
     desiredCapabilities: {
       browserName: null,
       // other desired capabilities
     },
   }
  }

• Both local and global installation of appium package can be used. The above configs work with the local installation of appium. To use a global installation of appium, set server_path property of selenium config to 'appium' or any other command you want to start the Appium Server with.

  test_settings: {
   app_env: {
     selenium: {
       start_process: true,
       use_appium: true,
       port: 4723,
       server_path: 'appium',  // <-- here
       // other properties
     },
     webdriver: {
       start_process: false
     },
     desiredCapabilities: {
       browserName: null,
       // other desired capabilities
     },
   }
  }

• Setting just browserName: null to use Nightwatch with the Appium server is now deprecated. You should set use_appium property of selenium config to true to use Nightwatch with Appium Server, as mentioned in the second point above.

[Topperfalkon]

Regarding "why browsername=null was needed earlier", I might be able to provide additional context.

I introduced some checking logic a while back for NW 1.7: #2882

I suspect this was to allow support for older versions of Appium that I guess Browserstack was using by default at the time? It's been a fair while since I've looked at this though, so that might be the limit of my memory :)

[garg3133]

Thanks @Topperfalkon, your comment was helpful. It helped me realize that browserName=null was required earlier to work with Appium just because browserName defaults to firefox if not passed, which causes an error in Appium that both browserName and appPackage cannot be defined together. And if set to something not a browser, Nightwatch used to throw an error.

But with the current change, users will be able to set browserName to null or '' for doing native app testing and to any browser for running tests on mobile browsers. But not passing browserName altogether would still result in it being set to firefox by default, so they would need to set it to something.

[garg3133]

Everything is done in this PR now according to me. There are just two issues with BrowserStack that are left:

• Since browserstack.js extends appium..js, createSessionOptions() is also being overridden for BrowserStack, which shouldn't be the case for BrowserStack Automate atleast.
• isMobile() API method makes BrowserStack use app-automate even for mobile-web testing, while mobile-web testing should be performed on BrowserStack Automate.

[gravityvi]

To support Browserstack mobile and web, we can have two classes that extends Browserstack and we can add in a createDriver of appium a Mixin method to Browserstack Mobile class prototype

[garg3133]

Commit e000786 contains my approach for solving the BrowserStack related issues and commit 1167538 contains Ravi's approach for solving the problem.

I personally like my approach better for its simplicity but if we don't want to have AppiumServer as the parent class for BrowserStack and go forward with Ravi's approach, I'd still prefer to have a single class for BrowserStack and not three classes, because most of the things in Automate and AppAutomate are still the same (we would still want to use Appium's createDriver method in Automate to test on browsers not supported by Selenium, and use part of Appium's createSessionOptions since testing mobile-web on Automate still happens using Appium).

[gravityvi]

It's better to have different classes rather than hiding every functionality under one class. It will help us in the future when adding additional capabilities which are specific to app-automate. Technically we should use selenium driver instance created from the builder for automate sessions in BrowserStack.

[garg3133]

Makes sense!

[gravityvi]

I think for browserstack classes we can create a separate folder and rename it automate and AppAutomate respectively

[beatfactor]

can't we use normal inheritance here?

[garg3133]

We could but then browserstack.js would have to extend to appium.js because we need a few functionalities from the AppiumServer class in automate and app-automate and then to make it work, we'd have to write some additional code in appium.js related to BrowserStack. Commit e000786 does just that.

But then, as I think now, browserstack.js and appium.js should be more or less independent and we shouldn't need to write additional code in appium.js to support browserstack.js, because otherwise when appium.js would grow at some later point of time, we'd always have to do something extra to ensure that browserstack still works fine.

[beatfactor]

or both browserstack and appium could extend a base class.

[garg3133]

Please check 0706064.

[beatfactor]

we should use a factory here.

[garg3133]

You mean factory as variable name or use a TransportFactory class method to decide whether appium is being used or not?

[beatfactor]

use a factory to create the server

[garg3133]

Please check 54587b1 if it is fine.