Do not persist a driver instance in the configuration object but in the browser

Author: Vampire

doc/manual-snippets/src/test/groovy/configuration/DriverConfigSpec.groovy

@@ -54,7 +54,7 @@ class DriverConfigSpec extends Specification implements InlineConfigurationLoade
         """
 
         then:
-        config.driver instanceof FirefoxDriver
+        config.createDriver() instanceof FirefoxDriver
 
         cleanup:
         CachingDriverFactory.clearCacheAndQuitDriver()
@@ -72,7 +72,7 @@ class DriverConfigSpec extends Specification implements InlineConfigurationLoade
         """
 
         then:
-        config.driver instanceof FirefoxDriver
+        config.createDriver() instanceof FirefoxDriver
 
         cleanup:
         CachingDriverFactory.clearCacheAndQuitDriver()
@@ -90,7 +90,7 @@ class DriverConfigSpec extends Specification implements InlineConfigurationLoade
         """
 
         then:
-        config.driver instanceof FirefoxDriver
+        config.createDriver() instanceof FirefoxDriver
 
         cleanup:
         CachingDriverFactory.clearCacheAndQuitDriver()
@@ -130,7 +130,7 @@ class DriverConfigSpec extends Specification implements InlineConfigurationLoade
         """)
 
         then:
-        config.driver in driverClass
+        config.createDriver() in driverClass
 
         cleanup:
         CachingDriverFactory.clearCacheAndQuitDriver()
@@ -140,4 +140,4 @@ class DriverConfigSpec extends Specification implements InlineConfigurationLoade
         null     | HtmlUnitDriver
         "remote" | RemoteWebDriver
     }
-}
+}

module/geb-core/src/main/groovy/geb/Browser.groovy

@@ -60,6 +60,7 @@ class Browser {
 
     private Page page
     private final Configuration config
+    private WebDriver driver
     private final RemoteDriverOperations remoteDriverOperations = new RemoteDriverOperations(this.class.classLoader)
     private String reportGroup = null
     private NavigatorFactory navigatorFactory = null
@@ -68,7 +69,7 @@ class Browser {
      * If the driver is remote, this object allows access to its capabilities (users of Geb should not access this object, it is used internally).
      */
     @Lazy
-    WebDriver augmentedDriver = remoteDriverOperations.getAugmentedDriver(driver)
+    WebDriver augmentedDriver = remoteDriverOperations.getAugmentedDriver(getDriver())
 
     /**
      * Create a new browser with a default configuration loader, loading the default configuration file.
@@ -164,10 +165,14 @@ class Browser {
      * <p>
      * The driver implementation to use is determined by the configuration.
      *
-     * @see geb.Configuration#getDriver()
+     * @see geb.Configuration#createDriver()
      */
     WebDriver getDriver() {
-        config.driver
+        if (driver == null) {
+            driver = createDriver()
+        }
+
+        driver
     }
 
     /**
@@ -189,11 +194,9 @@ class Browser {
      * This should only be called before making any requests as a means to override the driver instance
      * that would be created from the configuration. Where possible, prefer using the configuration mechanism
      * to specify the driver implementation to use.
-     * <p>
-     * This method delegates to {@link geb.Configuration#setDriver(org.openqa.selenium.WebDriver)}.
      */
     void setDriver(WebDriver driver) {
-        config.driver = driver
+        this.driver = driver
     }
 
     /**
@@ -223,7 +226,7 @@ class Browser {
      * @see org.openqa.selenium.WebDriver#getCurrentUrl()
      */
     String getCurrentUrl() {
-        driver.currentUrl
+        getDriver().currentUrl
     }
 
     /**
@@ -509,9 +512,9 @@ class Browser {
     void go(Map params = [:], String url = null, UrlFragment fragment = null) {
         def newUri = calculateUri(url, params, fragment)
         def currentUri = retrieveCurrentUri()
-        driver.get(newUri.toString())
+        getDriver().get(newUri.toString())
         if (sameUrlWithDifferentFragment(currentUri, newUri)) {
-            driver.navigate().refresh()
+            getDriver().navigate().refresh()
         }
         if (!page) {
             page(Page)
@@ -630,7 +633,7 @@ class Browser {
      * @see org.openqa.selenium.WebDriver.Options#deleteAllCookies()
      */
     void clearCookies() {
-        driver?.manage()?.deleteAllCookies()
+        getDriver()?.manage()?.deleteAllCookies()
     }
 
     /**
@@ -669,7 +672,7 @@ class Browser {
      * @see org.openqa.selenium.WebDriver#quit()
      */
     void quit() {
-        driver.quit()
+        getDriver().quit()
     }
 
     /**
@@ -678,7 +681,7 @@ class Browser {
      * @see org.openqa.selenium.WebDriver#close()
      */
     void close() {
-        driver.close()
+        getDriver().close()
     }
 
     /**
@@ -687,7 +690,7 @@ class Browser {
      * @see org.openqa.selenium.WebDriver#getWindowHandle()
      */
     String getCurrentWindow() {
-        driver.windowHandle
+        getDriver().windowHandle
     }
 
     /**
@@ -696,7 +699,7 @@ class Browser {
      * @see org.openqa.selenium.WebDriver#getWindowHandles()
      */
     Set<String> getAvailableWindows() {
-        driver.windowHandles
+        getDriver().windowHandles
     }
 
     /**
@@ -812,7 +815,7 @@ class Browser {
             cloned.call()
         } finally {
             if ((!options.containsKey(CLOSE_OPTION) && config.withNewWindowConfig.close.orElse(true)) || options.close) {
-                driver.close()
+                getDriver().close()
             }
             switchToWindow(originalWindow)
             page originalPage
@@ -996,8 +999,8 @@ class Browser {
     }
 
     public <T> Optional<T> driverAs(Class<T> castType) {
-        if (castType.isInstance(driver)) {
-            Optional.of(driver as T)
+        if (castType.isInstance(getDriver())) {
+            Optional.of(getDriver() as T)
         } else if (castType.isInstance(augmentedDriver)) {
             Optional.of(augmentedDriver as T)
         } else {
@@ -1006,7 +1009,7 @@ class Browser {
     }
 
     protected switchToWindow(String window) {
-        driver.switchTo().window(window)
+        getDriver().switchTo().window(window)
     }
 
     protected <T> T doWithWindow(Map options, Closure<T> block) {
@@ -1021,7 +1024,7 @@ class Browser {
             cloned.call()
         } finally {
             if (options.close || (!options.containsKey(CLOSE_OPTION) && config.withWindowConfig.close)) {
-                driver.close()
+                getDriver().close()
             }
         }
     }
@@ -1035,6 +1038,15 @@ class Browser {
         config.createNavigatorFactory(this)
     }
 
+    /**
+     * Called to create the driver, the first time it is requested.
+     *
+     * @return The driver
+     */
+    protected WebDriver createDriver() {
+        config.createDriver()
+    }
+
     private WebDriver getDriverWithNetworkConditions() {
         optionalHasNetworkConditionsClass.map { hasNetworkConditionsClass ->
             driverAs(hasNetworkConditionsClass).orElse(null)
@@ -1230,7 +1242,7 @@ class Browser {
     private URI retrieveCurrentUri() {
         def currentUri = null
         try {
-            def currentUrl = driver.currentUrl
+            def currentUrl = getDriver().currentUrl
             currentUri = currentUrl ? new URI(currentUrl) : null
         } catch (NullPointerException npe) {
         } catch (URISyntaxException use) {

module/geb-core/src/main/groovy/geb/Configuration.groovy

@@ -256,7 +256,7 @@ class Configuration {
      * This may be the class name of a driver implementation, a driver short name or a closure
      * that when invoked with no arguments returns a driver implementation.
      *
-     * @see #getDriver()
+     * @see #createDriver()
      */
     void setDriverConf(value) {
         rawConfig.driver = value
@@ -268,7 +268,7 @@ class Configuration {
      * This may be the class name of a driver implementation, a short name, or a closure
      * that when invoked returns an actual driver.
      *
-     * @see #getDriver()
+     * @see #createDriver()
      */
     def getDriverConf() {
         def value = properties.getProperty("geb.driver") ?: readValue("driver", null)
@@ -376,16 +376,22 @@ class Configuration {
         rawConfig.pageEventListener = pageEventListener
     }
 
+    /**
+     * @deprecated As of 8.0, replaced by {@link #createDriver()}, the configuration does
+     *             no longer carry a driver instance.
+     */
+    @Deprecated
     WebDriver getDriver() {
-        if (driver == null) {
-            driver = createDriver()
-        }
-
-        driver
+        createDriver()
     }
 
+    /**
+     * @deprecated As of 8.0, the configuration does no longer carry a driver
+     *             instance, but only create new driver instances, the driver
+     *             instance used is stored in the browser instance.
+     */
+    @Deprecated
     void setDriver(WebDriver driver) {
-        this.driver = driver
     }
 
     /**
@@ -700,7 +706,7 @@ class Configuration {
         throw new InvalidGebConfiguration("Configuration for bounds and 'required' template options is conflicting")
     }
 
-    protected WebDriver createDriver() {
+    WebDriver createDriver() {
         wrapDriverFactoryInCachingIfNeeded(getDriverFactory(getDriverConf())).driver
     }
 

module/geb-core/src/test/groovy/geb/PauseSpec.groovy

@@ -56,7 +56,7 @@ class PauseSpec extends GebSpecWithCallbackServer {
     @InheritConstructors
     private static class ThreadSafeScriptExecutionDriverConfiguration extends Configuration {
         @Override
-        protected WebDriver createDriver() {
+        WebDriver createDriver() {
             def driver = super.createDriver()
             driver.javascriptEnabled = true
             ThreadSafeScriptExecutionDriver.of(driver)

module/geb-core/src/test/groovy/geb/conf/ConfigurationDriverCreationSpec.groovy

@@ -168,7 +168,7 @@ class ConfigurationDriverCreationSpec extends Specification {
         def config = new ConfigObject()
         config.cacheDriver = false
         config.driver = { new HtmlUnitDriver() }
-        d = new Configuration(config, p()).driver
+        d = new Configuration(config, p()).createDriver()
 
         then:
         d instanceof HtmlUnitDriver
@@ -182,7 +182,7 @@ class ConfigurationDriverCreationSpec extends Specification {
         config.driver = { 'not a driver' }
 
         when:
-        new Configuration(config).driver
+        new Configuration(config).createDriver()
 
         then:
         thrown(DriverCreationException)

module/geb-core/src/test/groovy/geb/conf/DriverCachingSpec.groovy

@@ -35,13 +35,11 @@ class DriverCachingSpec extends Specification {
         File getReportsDir() { null }
     }
 
-    def conf(cacheDriverPerThread = false) {
+    def conf(cacheDriverPerThread = false, cacheDriver = true) {
         def conf = new Configuration(new ConfigObject(), new Properties(), new NullBuildAdapter())
+        conf.cacheDriver = cacheDriver
         conf.cacheDriverPerThread = cacheDriverPerThread
         conf.driverConf = { new HtmlUnitDriver() }
-
-        assert conf.cacheDriver
-
         conf
     }
 
@@ -58,23 +56,23 @@ class DriverCachingSpec extends Specification {
         def holder = new BlockingVariable()
 
         when:
-        Thread.start { holder.set(conf2.driver) }
+        Thread.start { holder.set(conf2.createDriver()) }
 
         and:
-        def driver1 = conf1.driver
+        def driver1 = conf1.createDriver()
         def driver2 = holder.get()
 
         then:
         !driver1.is(driver2)
 
         and:
-        driver1.is conf(true).driver
+        driver1.is conf(true).createDriver()
 
         when:
         CachingDriverFactory.clearCacheAndQuitDriver()
 
         then:
-        !driver1.is(conf(true).driver)
+        !driver1.is(conf(true).createDriver())
 
         cleanup:
         driver1.quit()
@@ -90,23 +88,39 @@ class DriverCachingSpec extends Specification {
         def holder = new BlockingVariable()
 
         when:
-        Thread.start { holder.set(conf2.driver) }
+        Thread.start { holder.set(conf2.createDriver()) }
 
         and:
-        def driver1 = conf1.driver
+        def driver1 = conf1.createDriver()
         def driver2 = holder.get()
 
         then:
         driver1.is(driver2)
 
         and:
-        driver1.is conf().driver
+        driver1.is conf().createDriver()
 
         when:
         CachingDriverFactory.clearCacheAndQuitDriver()
 
         then:
-        !driver1.is(conf(true).driver)
+        !driver1.is(conf(true).createDriver())
+
+        cleanup:
+        driver1.quit()
+        driver2.quit()
+    }
+
+    def "disabled caching yields different drivers on each call"() {
+        given:
+        def conf = conf(false, false)
+
+        when:
+        def driver1 = conf.createDriver()
+        def driver2 = conf.createDriver()
+
+        then:
+        !driver1.is(driver2)
 
         cleanup:
         driver1.quit()

module/geb-core/src/test/groovy/geb/test/GebTestManagerBrowserResetSpec.groovy

@@ -48,7 +48,7 @@ class GebTestManagerBrowserResetSpec extends Specification {
         driver = new RemoteWebDriverWithExpectations(
             webDriverServer.url, DEFAULT_IGNORED_COMMANDS - 'quit'
         )
-        configuration.driver = driver
+        configuration.driverConf = { driver }
         gebTestManager = new GebTestManagerBuilder()
             .withBrowserCreator {
                 new Browser(configuration)