cbmarcum commented on code in PR #313:
URL: https://github.com/apache/groovy-geb/pull/313#discussion_r3005341372


##########
integration/geb-testcontainers/geb-testcontainers.gradle:
##########
@@ -103,9 +103,6 @@ tasks.register('integrationTest', Test) {
     systemProperty('geb.env', System.getProperty('geb.env'))

Review Comment:
   add a default environment in case one isn't passed in since integrationTest 
is being ran during check instead of firefoxTest.
   ```suggestion
       systemProperty('geb.env', System.getProperty('geb.env') ?: 'firefox')
   ```
   



##########
integration/geb-testcontainers/src/main/groovy/geb/testcontainers/WebDriverContainerHolder.groovy:
##########
@@ -176,29 +161,21 @@ class WebDriverContainerHolder {
         }
 
         // Ensure that the driver points to the re-initialized container with 
the correct host.
-        // The driver is explicitly quit by us in stop() method, to fulfill 
our resulting responsibility.
         gebConf.cacheDriver = false
-
-        // As we don't cache, this will have been defaulted to true. We 
override to false.
         gebConf.quitDriverOnBrowserReset = false
-
         gebConf.baseUrl = container.seleniumAddress
+
         if (containerConf.reporting) {
             gebConf.reportsDir = settings.reportingDirectory
             gebConf.reporter = (methodInvocation.sharedInstance as 
ContainerGebSpec).createReporter()
         }
 
-        if (gebConf.driverConf) {
-            // As a custom `GebConfig` cannot know the `remoteAddress` of the 
container beforehand,
-            // the `RemoteWebDriver` will be instantiated using the 
`webdriver.remote.server`
-            // system property. We set that property to inform the driver of 
the container address.
-            gebConf.driverConf = ClosureDecorators.withSystemProperty(
-                gebConf.driverConf as Closure,
-                REMOTE_ADDRESS_PROPERTY,
-                container.seleniumAddress
-            )
-        } else {
-            // If no driver was set in GebConfig, create a Firefox driver
+        // Set the selenium address as a system property so that 
RemoteWebDriver
+        // constructors (without explicit URL) can find it. Tests run serially
+        // (enforced by ExclusiveResource) so this is safe.
+        System.setProperty(REMOTE_ADDRESS_PROPERTY, 
container.seleniumAddress.toString())
+
+        if (!gebConf.driverConf) {

Review Comment:
   codenarc wants this if block to use an elvis operator instead
   ```suggestion
           gebConf.driverConf = gebConf.driverConf ?: { ->
               log.info('Using default Firefox RemoteWebDriver for {}', 
container.seleniumAddress)
               new RemoteWebDriver(container.seleniumAddress, new 
FirefoxOptions())
           }
   ```



##########
integration/geb-testcontainers/src/main/groovy/geb/testcontainers/ContainerGebSpec.groovy:
##########
@@ -18,20 +18,22 @@
  */
 package geb.testcontainers
 
-import geb.Page
+import geb.download.DownloadSupport
+import geb.report.CompositeReporter
+import geb.report.PageSourceReporter
+import geb.report.Reporter
+import geb.report.ScreenshotReporter
 import geb.test.GebTestManager
-import geb.testcontainers.support.ContainerSupport
-import geb.testcontainers.support.ReportingSupport
-import geb.testcontainers.support.delegate.BrowserDelegate
-import geb.testcontainers.support.delegate.DownloadSupportDelegate
-import geb.testcontainers.support.delegate.DriverDelegate
-import geb.testcontainers.support.delegate.PageDelegate
-import groovy.transform.CompileStatic
+import geb.transform.DynamicallyDispatchesToBrowser
+import geb.testcontainers.support.ContainerGebFileInputSource
+import org.testcontainers.containers.BrowserWebDriverContainer
+import org.testcontainers.containers.GenericContainer

Review Comment:
   GenericContainer import is unused.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to