chore(browser): remove timing issues with restart and fork (#5085)

- remove .ready since forking should automatically return a browser
- getNewDriver should return a promised WebDriver that can be awaited
- fix interaction tests and local driver tests
- update unit tests for async await due to getNewDriver fix

closes #5031

Author: cnishina

lib/browser.ts

@@ -238,7 +238,7 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver {
    * Resolved when the browser is ready for use.  Resolves to the browser, so
    * you can do:
    *
-   *   forkedBrowser = await browser.forkNewDriverInstance().ready;
+   *   forkedBrowser = await browser.forkNewDriverInstance();
    *
    * Set by the runner.
    *
@@ -359,22 +359,6 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver {
         ng12Hybrid_ = ng12Hybrid;
       }
     });
-    this.ready = this.angularAppRoot(opt_rootElement || '')
-                     .then(() => {
-                       return this.driver.getSession();
-                     })
-                     .then((session: Session) => {
-                       // Internet Explorer does not accept data URLs, which are the default
-                       // reset URL for Protractor.
-                       // Safari accepts data urls, but SafariDriver fails after one is used.
-                       // PhantomJS produces a "Detected a page unload event" if we use data urls
-                       let browserName = session.getCapabilities().get('browserName');
-                       if (browserName === 'internet explorer' || browserName === 'safari' ||
-                           browserName === 'phantomjs' || browserName === 'MicrosoftEdge') {
-                         this.resetUrl = 'about:blank';
-                       }
-                       return this;
-                     });
 
     this.trackOutstandingTimeouts_ = !opt_untrackOutstandingTimeouts;
     this.mockModules_ = [];
@@ -423,7 +407,7 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver {
    * Fork another instance of browser for use in interactive tests.
    *
    * @example
-   * var forked = await browser.forkNewDriverInstance().ready;
+   * var forked = await browser.forkNewDriverInstance();
    * await forked.get('page1'); // 'page1' gotten by forked browser
    *
    * @param {boolean=} useSameUrl Whether to navigate to current url on creation
@@ -433,8 +417,9 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver {
    *
    * @returns {ProtractorBrowser} A browser instance.
    */
-  forkNewDriverInstance(useSameUrl?: boolean, copyMockModules?: boolean, copyConfigUpdates = true):
-      ProtractorBrowser {
+  async forkNewDriverInstance(
+      useSameUrl?: boolean, copyMockModules?: boolean,
+      copyConfigUpdates = true): Promise<ProtractorBrowser> {
     return null;
   }
 
@@ -456,7 +441,7 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver {
    * await browser.get('page2'); // 'page2' gotten by restarted browser
    *
    * // Running against forked browsers
-   * var forked = await browser.forkNewDriverInstance().ready;
+   * var forked = await browser.forkNewDriverInstance();
    * await fork.get('page1');
    * fork = await fork.restart();
    * await fork.get('page2'); // 'page2' gotten by restarted fork

lib/driverProviders/attachSession.ts

@@ -35,10 +35,11 @@ export class AttachSession extends DriverProvider {
    * @public
    * @return {WebDriver} webdriver instance
    */
-  getNewDriver(): WebDriver {
+  async getNewDriver(): Promise<WebDriver> {
     const httpClient = new http.HttpClient(this.config_.seleniumAddress);
     const executor = new http.Executor(httpClient);
-    const newDriver = WebDriver.attachToSession(executor, this.config_.seleniumSessionId, null);
+    const newDriver =
+        await WebDriver.attachToSession(executor, this.config_.seleniumSessionId, null);
     this.drivers_.push(newDriver);
     return newDriver;
   }

lib/driverProviders/direct.ts

@@ -49,7 +49,7 @@ export class Direct extends DriverProvider {
    * @override
    * @return webdriver instance
    */
-  getNewDriver(): WebDriver {
+  async getNewDriver(): Promise<WebDriver> {
     let driver: WebDriver;
 
     switch (this.config_.capabilities.browserName) {
@@ -74,7 +74,7 @@ export class Direct extends DriverProvider {
         }
 
         let chromeService = new ChromeServiceBuilder(chromeDriverFile).build();
-        driver = DriverForChrome.createSession(
+        driver = await DriverForChrome.createSession(
             new Capabilities(this.config_.capabilities), chromeService);
         break;
       case 'firefox':
@@ -98,7 +98,7 @@ export class Direct extends DriverProvider {
         }
 
         let firefoxService = new FirefoxServiceBuilder(geckoDriverFile).build();
-        driver = DriverForFirefox.createSession(
+        driver = await DriverForFirefox.createSession(
             new Capabilities(this.config_.capabilities), firefoxService);
         break;
       default:

lib/driverProviders/driverProvider.ts

@@ -7,7 +7,10 @@ import {Builder, WebDriver} from 'selenium-webdriver';
 
 import {BlockingProxyRunner} from '../bpRunner';
 import {Config} from '../config';
+import {BrowserError} from '../exitCodes';
+import {Logger} from '../logger';
 
+let logger = new Logger('driverProvider');
 export abstract class DriverProvider {
   drivers_: WebDriver[];
   config_: Config;
@@ -42,7 +45,7 @@ export abstract class DriverProvider {
    * @public
    * @return webdriver instance
    */
-  getNewDriver() {
+  async getNewDriver() {
     let builder: Builder;
     if (this.config_.useBlockingProxy) {
       builder =
@@ -56,7 +59,12 @@ export abstract class DriverProvider {
     if (this.config_.disableEnvironmentOverrides === true) {
       builder.disableEnvironmentOverrides();
     }
-    let newDriver = builder.build() as WebDriver;
+    let newDriver: WebDriver;
+    try {
+      newDriver = await builder.build();
+    } catch (e) {
+      throw new BrowserError(logger, (e as Error).message);
+    }
     this.drivers_.push(newDriver);
     return newDriver;
   }
@@ -72,6 +80,7 @@ export abstract class DriverProvider {
     if (driverIndex >= 0) {
       this.drivers_.splice(driverIndex, 1);
       try {
+        await driver.close();
         await driver.quit();
       } catch (err) {
         // This happens when Protractor keeps track of all the webdrivers

lib/driverProviders/mock.ts

@@ -38,7 +38,7 @@ export class Mock extends DriverProvider {
    * @override
    * @return webdriver instance
    */
-  getNewDriver(): WebDriver {
+  async getNewDriver(): Promise<WebDriver> {
     let mockSession = new Session('test_session_id', {});
     let newDriver = new WebDriver(mockSession, new MockExecutor());
     this.drivers_.push(newDriver);

lib/runner.ts

@@ -1,6 +1,6 @@
 import {EventEmitter} from 'events';
 // TODO(cnishina): remove when selenium webdriver is upgraded.
-import {promise as wdpromise} from 'selenium-webdriver';
+import {promise as wdpromise, WebDriver} from 'selenium-webdriver';
 import * as util from 'util';
 
 import {ProtractorBrowser} from './browser';
@@ -54,9 +54,6 @@ export class Runner extends EventEmitter {
       nodedebug.on('exit', () => {
         process.exit(1);
       });
-      this.ready_ = new Promise(resolve => {
-        setTimeout(resolve, 1000);
-      });
     }
 
     if (config.capabilities && config.capabilities.seleniumAddress) {
@@ -206,9 +203,9 @@ export class Runner extends EventEmitter {
    * @return {Protractor} a protractor instance.
    * @public
    */
-  createBrowser(plugins: any, parentBrowser?: ProtractorBrowser): any {
+  async createBrowser(plugins: any, parentBrowser?: ProtractorBrowser): Promise<any> {
     let config = this.config_;
-    let driver = this.driverprovider_.getNewDriver();
+    let driver = await this.driverprovider_.getNewDriver();
 
     let blockingProxyUrl: string;
     if (config.useBlockingProxy) {
@@ -258,58 +255,40 @@ export class Runner extends EventEmitter {
       browser_.ng12Hybrid = initProperties.ng12Hybrid;
     }
 
-    browser_.ready =
-        browser_.ready
-            .then(() => {
-              return browser_.waitForAngularEnabled(initProperties.waitForAngularEnabled);
-            })
-            .then(() => {
-              return driver.manage().timeouts().setScriptTimeout(
-                  initProperties.allScriptsTimeout || 0);
-            })
-            .then(() => {
-              return browser_;
-            });
+    await browser_.waitForAngularEnabled(initProperties.waitForAngularEnabled);
+    await driver.manage().timeouts().setScriptTimeout(initProperties.allScriptsTimeout || 0);
 
     browser_.getProcessedConfig = () => {
       return Promise.resolve(config);
     };
 
-    browser_.forkNewDriverInstance =
-        (useSameUrl: boolean, copyMockModules: boolean, copyConfigUpdates = true) => {
-          let newBrowser = this.createBrowser(plugins);
-          if (copyMockModules) {
-            newBrowser.mockModules_ = browser_.mockModules_;
-          }
-          if (useSameUrl) {
-            newBrowser.ready = newBrowser.ready
-                                   .then(() => {
-                                     return browser_.driver.getCurrentUrl();
-                                   })
-                                   .then((url: string) => {
-                                     return newBrowser.get(url);
-                                   })
-                                   .then(() => {
-                                     return newBrowser;
-                                   });
-          }
-          return newBrowser;
-        };
-
-    let replaceBrowser = () => {
-      let newBrowser = browser_.forkNewDriverInstance(false, true);
+    browser_.forkNewDriverInstance = async(
+        useSameUrl: boolean, copyMockModules: boolean, copyConfigUpdates = true): Promise<any> => {
+      let newBrowser = await this.createBrowser(plugins);
+      if (copyMockModules) {
+        newBrowser.mockModules_ = browser_.mockModules_;
+      }
+      if (useSameUrl) {
+        const currentUrl = await browser_.driver.getCurrentUrl();
+        await newBrowser.get(currentUrl);
+      }
+      return newBrowser;
+    };
+
+    let replaceBrowser = async () => {
+      let newBrowser = await browser_.forkNewDriverInstance(false, true);
       if (browser_ === protractor.browser) {
         this.setupGlobals_(newBrowser);
       }
       return newBrowser;
     };
 
-    browser_.restart = () => {
+    browser_.restart = async () => {
       // Note: because tests are not paused at this point, any async
       // calls here are not guaranteed to complete before the tests resume.
-      return this.driverprovider_.quitDriver(browser_.driver)
-          .then(replaceBrowser)
-          .then(newBrowser => newBrowser.ready);
+      const restartedBrowser = await replaceBrowser();
+      await this.driverprovider_.quitDriver(browser_.driver);
+      return restartedBrowser;
     };
 
     return browser_;
@@ -352,18 +331,15 @@ export class Runner extends EventEmitter {
       this.config_.useBlockingProxy = true;
     }
 
-    // 0) Wait for debugger
-    await Promise.resolve(this.ready_);
-
     // 1) Setup environment
     // noinspection JSValidateTypes
     await this.driverprovider_.setupEnv();
 
     // 2) Create a browser and setup globals
-    browser_ = this.createBrowser(plugins);
+    browser_ = await this.createBrowser(plugins);
     this.setupGlobals_(browser_);
     try {
-      const session = await browser_.ready.then(browser_.getSession);
+      const session = await browser_.getSession();
       logger.debug(
           'WebDriver session successfully started with capabilities ' +
           util.inspect(session.getCapabilities()));
@@ -403,9 +379,9 @@ export class Runner extends EventEmitter {
 
     if (this.config_.restartBrowserBetweenTests) {
       // TODO(sjelin): replace with warnings once `afterEach` support is required
-      let restartDriver = () => {
+      let restartDriver = async () => {
         if (!this.frameworkUsesAfterEach) {
-          this.restartPromise = Promise.resolve(browser_.restart());
+          this.restartPromise = await browser_.restart();
         }
       };
       this.on('testPass', restartDriver);