Change the default behaviour of useNewWDA capability #456

mykola-mokhnach · 2017-06-17T11:42:23Z

This PR tries to address real device issues like facebookarchive/WebDriverAgent#507 by keeping WDA session running for as long as possible and reuse it every time the new session is created instead of recompiling/redeploying WDA. Also, it should significantly shorten the duration of driver initialisation stage between multiple test cases.

@imurchie Could you please assist with testing, since it is necessary to significantly change our automation framework to test these changes on Simulator and (especially) on real device? Thanks in advance.

imurchie · 2017-06-19T11:36:33Z

Sure. I have some real device stuff I need to do today, so I'll take a whirl.

imurchie

This seems to work well in local tests. One remark about short-circuiting.

imurchie · 2017-06-20T14:15:24Z

lib/driver.js

        this.logEvent('wdaStartFailed');
+        if (this.isRealDevice() && launchRetriesCount > 1) {


I presume you want to not retry if there is an xcodebuild failure on a real device?

This will not do that. Any error thrown here will trigger a retry at the retryInterval level (on line 427). It would be better to check for real device before the retryInterval, and adjust the startupRetries accordingly.

const startupRetries = this.isRealDevice() ? 1 : (this.opts.wdaStartupRetries || WDA_STARTUP_RETRIES);

It might not be a good idea to implicitly override user-supplied option.

How about this one?

const startupRetries = this.opts.wdaStartupRetries || (this.isRealDevice() ? 1 : WDA_STARTUP_RETRIES);

Good point. Yeah, that would work. Since the number varies, it should probably be logged, as well.

imurchie · 2017-06-20T18:31:28Z

lib/driver.js

@@ -434,7 +461,13 @@ class XCUITestDriver extends BaseDriver {
        });
        this.logEvent('wdaSessionStarted');
      } catch (err) {
-        return await quitAndUninstall(`Unable to start WebDriverAgent session: ${err.message}`);
+        let errorMsg = `Unable to start WebDriverAgent sesssion because of xcodebuild failure: ${err.message}`;


Refactor into a function call to handle the error, so it's not repeated here?

error messages are different for both cases. I separated them intentionally for a case when it is necessary to add any other helpful message details to help user to fix his problem for the particular case.

jlipps

this will be really useful!

jlipps · 2017-06-21T13:47:11Z

README.md

@@ -134,7 +134,7 @@ Differences noted here
 |`usePrebuiltWDA`|Skips the build phase of running the WDA app. Building is then the responsibility of the user. Only works for Xcode 8+. Defaults to `false`.|e.g., `true`|
 |`preventWDAAttachments`|Sets read only permissions to Attachments subfolder of WebDriverAgent root inside Xcode's DerivedData. This is necessary to prevent XCTest framework from creating tons of unnecessary screenshots and logs, which are impossible to turn off using programming interfaces provided by Apple.|Setting the capability to `true` will set Posix permissions of the folder to `555` and `false` will reset them back to `755`. `true` by default|
 |`webDriverAgentUrl`|If provided, Appium will connect to an existing WebDriverAgent instance at this URL instead of starting a new one.|e.g., `http://localhost:8100`|
-|`useNewWDA`|If `true`, forces uninstall of any existing WebDriverAgent app on device. This can provide stability in some situations. Defaults to `false`.|e.g., `true`|
+|`useNewWDA`|If `true`, forces uninstall of any existing WebDriverAgent app on device. Set it to `true` if you want to apply different startup options for WebDriverAgent for each session. Although, it is only guaranteed to work stable on Simulator. Real devices require WebDriverAgent client to run for as long as possible without reinstall/restart to avoid issues like https://github.com/facebook/WebDriverAgent/issues/507. The `false` value (the default behaviour since Appium 1.6.6) will try to detect currently running WDA listener executed by previous testing session(s) and reuse it if possible, which is highly recommended for real device testing and to speed up suites of multiple tests in general. A new WDA session will be triggered at the default URL (http://localhost:8100) if WDA is not listening and `webDriverAgentUrl` capability is not set. The negative/unset value of `useNewWDA` capability has no effect prior to Appium 1.6.6.|e.g., `true`|


probably don't want to refer to appium versions in the xcuitest repo, maybe refer to the xcuitest version? unless the default is set in appium and not here. which might be the case, hmm.

changed to driver version

i don't see a change

jlipps · 2017-06-21T13:52:27Z

lib/driver.js

-        await quitAndUninstall(`Unable to launch WebDriverAgent because of xcodebuild failure: ${err.message}`);
+        let errorMsg = `Unable to launch WebDriverAgent because of xcodebuild failure: "${err.message}".`;
+        if (this.isRealDevice()) {
+          errorMsg += ` Make sure you follow the tutorial at https://github.com/appium/appium-xcuitest-driver/blob/master/docs/real-device-config.md. ` +


formatting looks goofy here, maybe because the lines are too long (we try to stick to 80 chars when possible)

jlipps

n/m saw the change. one more comment.

jlipps · 2017-06-21T15:27:19Z

README.md

@@ -134,7 +134,7 @@ Differences noted here
 |`usePrebuiltWDA`|Skips the build phase of running the WDA app. Building is then the responsibility of the user. Only works for Xcode 8+. Defaults to `false`.|e.g., `true`|
 |`preventWDAAttachments`|Sets read only permissions to Attachments subfolder of WebDriverAgent root inside Xcode's DerivedData. This is necessary to prevent XCTest framework from creating tons of unnecessary screenshots and logs, which are impossible to turn off using programming interfaces provided by Apple.|Setting the capability to `true` will set Posix permissions of the folder to `555` and `false` will reset them back to `755`. `true` by default|
 |`webDriverAgentUrl`|If provided, Appium will connect to an existing WebDriverAgent instance at this URL instead of starting a new one.|e.g., `http://localhost:8100`|
-|`useNewWDA`|If `true`, forces uninstall of any existing WebDriverAgent app on device. This can provide stability in some situations. Defaults to `false`.|e.g., `true`|
+|`useNewWDA`|If `true`, forces uninstall of any existing WebDriverAgent app on device. Set it to `true` if you want to apply different startup options for WebDriverAgent for each session. Although, it is only guaranteed to work stable on Simulator. Real devices require WebDriverAgent client to run for as long as possible without reinstall/restart to avoid issues like https://github.com/facebook/WebDriverAgent/issues/507. The `false` value (the default behaviour since Appium 1.6.6) will try to detect currently running WDA listener executed by previous testing session(s) and reuse it if possible, which is highly recommended for real device testing and to speed up suites of multiple tests in general. A new WDA session will be triggered at the default URL (http://localhost:8100) if WDA is not listening and `webDriverAgentUrl` capability is not set. The negative/unset value of `useNewWDA` capability has no effect prior to Appium 1.6.6.|e.g., `true`|


i don't see a change

jlipps · 2017-06-21T15:28:58Z

lib/driver.js

-        await quitAndUninstall(`Unable to launch WebDriverAgent because of xcodebuild failure: ${err.message}`);
+        let errorMsg = `Unable to launch WebDriverAgent because of xcodebuild failure: "${err.message}".`;
+        if (this.isRealDevice()) {
+          errorMsg += ` Make sure you follow the tutorial at ` +


there's now quite a bit of duplication with this error message; maybe make it a global constant?

What if I only extract the tutorial URL into a constant? I want to keep message texts separated, since it might be necessary to tune them in the future for launch session and wait for status cases

jlipps

fine with me if we think we will likely want to do that

imurchie · 2017-06-24T00:14:31Z

Published in [email protected].

mykola-mokhnach requested review from jlipps and imurchie June 17, 2017 11:42

Try to reuse running WDA instance and tune WDA startup logic

062036b

mykola-mokhnach force-pushed the new_wda_behavior branch from 109210d to 062036b Compare June 17, 2017 14:34

Add more detailed issue description

c254f43

mykola-mokhnach mentioned this pull request Jun 19, 2017

Getting an issue xcodebuild code 65 appium/appium#7742

Closed

imurchie approved these changes Jun 20, 2017

View reviewed changes

Tune retries count

698a79d

imurchie reviewed Jun 20, 2017

View reviewed changes

Avoid error message duplication

9cfcc16

jlipps approved these changes Jun 21, 2017

View reviewed changes

Tune max line lengths

b60f57f

jlipps reviewed Jun 21, 2017

View reviewed changes

Keep real device tutorial URL in a separate constant

0c9a16c

jlipps reviewed Jun 21, 2017

View reviewed changes

imurchie merged commit 8857a2b into appium:master Jun 24, 2017

mykola-mokhnach deleted the new_wda_behavior branch June 26, 2017 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the default behaviour of useNewWDA capability #456

Change the default behaviour of useNewWDA capability #456

mykola-mokhnach commented Jun 17, 2017

imurchie commented Jun 19, 2017

imurchie left a comment

imurchie Jun 20, 2017

mykola-mokhnach Jun 20, 2017 •

edited

Loading

imurchie Jun 20, 2017

imurchie Jun 20, 2017

mykola-mokhnach Jun 20, 2017 •

edited

Loading

jlipps left a comment

jlipps Jun 21, 2017

mykola-mokhnach Jun 21, 2017

jlipps Jun 21, 2017

jlipps Jun 21, 2017

mykola-mokhnach Jun 21, 2017

jlipps left a comment

jlipps Jun 21, 2017

jlipps Jun 21, 2017

mykola-mokhnach Jun 21, 2017 •

edited

Loading

jlipps left a comment

imurchie commented Jun 24, 2017

		this.logEvent('wdaStartFailed');
		if (this.isRealDevice() && launchRetriesCount > 1) {

Change the default behaviour of useNewWDA capability #456

Change the default behaviour of useNewWDA capability #456

Conversation

mykola-mokhnach commented Jun 17, 2017

imurchie commented Jun 19, 2017

imurchie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

jlipps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlipps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

jlipps left a comment

Choose a reason for hiding this comment

imurchie commented Jun 24, 2017

mykola-mokhnach Jun 20, 2017 •

edited

Loading

mykola-mokhnach Jun 20, 2017 •

edited

Loading

mykola-mokhnach Jun 21, 2017 •

edited

Loading