Code Review Feedback

grails · Nov 24, 2024 · 8a8a851 · 8a8a851
1 parent 55abfd7
commit 8a8a851
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 29 deletions.
diff --git a/src/testFixtures/groovy/grails/plugin/geb/ContainerGebRecordingExtension.groovy b/src/testFixtures/groovy/grails/plugin/geb/ContainerGebRecordingExtension.groovy
@@ -33,6 +33,7 @@ import java.time.LocalDateTime
 @Slf4j
 @CompileStatic
 class ContainerGebRecordingExtension implements IGlobalExtension {
+
     WebDriverContainerHolder holder
 
     @Override
@@ -51,7 +52,7 @@ class ContainerGebRecordingExtension implements IGlobalExtension {
             ContainerGebTestListener listener = new ContainerGebTestListener(holder, spec, LocalDateTime.now())
             spec.addSetupInterceptor {
                 holder.reinitialize(it)
-                (it.sharedInstance as ContainerGebSpec).webDriverContainer = holder.current
+                (it.sharedInstance as ContainerGebSpec).webDriverContainer = holder.currentContainer
             }
 
             spec.addListener(listener)
@@ -71,7 +72,7 @@ class ContainerGebRecordingExtension implements IGlobalExtension {
 
     private static void validateContainerGebSpec(SpecInfo specInfo) {
         if (!specInfo.annotations.find { it.annotationType() == Integration }) {
-            throw new IllegalArgumentException("ContainerGebSpec classes must be annotated with @Integration")
+            throw new IllegalArgumentException('ContainerGebSpec classes must be annotated with @Integration')
         }
     }
 }

diff --git a/src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestListener.groovy b/src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestListener.groovy
@@ -20,7 +20,6 @@ import org.spockframework.runtime.AbstractRunListener
 import org.spockframework.runtime.model.ErrorInfo
 import org.spockframework.runtime.model.IterationInfo
 import org.spockframework.runtime.model.SpecInfo
-import org.testcontainers.containers.BrowserWebDriverContainer
 
 import java.time.LocalDateTime
 
@@ -49,7 +48,7 @@ class ContainerGebTestListener extends AbstractRunListener {
 
     @Override
     void afterIteration(IterationInfo iteration) {
-        containerHolder.current.afterTest(
+        containerHolder.currentContainer.afterTest(
                 new ContainerGebTestDescription(iteration, runDate),
                 Optional.ofNullable(errorInfo?.exception)
         )

diff --git a/src/testFixtures/groovy/grails/plugin/geb/RecordingSettings.groovy b/src/testFixtures/groovy/grails/plugin/geb/RecordingSettings.groovy
@@ -21,6 +21,9 @@ import groovy.util.logging.Slf4j
 import org.testcontainers.containers.BrowserWebDriverContainer
 import org.testcontainers.containers.VncRecordingContainer
 
+import static org.testcontainers.containers.BrowserWebDriverContainer.*
+import static org.testcontainers.containers.VncRecordingContainer.*
+
 /**
  * Handles parsing various recording configuration used by {@link grails.plugin.geb.ContainerGebRecordingExtension}
  *
@@ -31,28 +34,35 @@ import org.testcontainers.containers.VncRecordingContainer
 @CompileStatic
 class RecordingSettings {
 
+    private static VncRecordingMode DEFAULT_RECORDING_MODE = VncRecordingMode.SKIP
+    private static VncRecordingFormat DEFAULT_RECORDING_FORMAT = VncRecordingFormat.MP4
+
     String recordingDirectoryName
-    BrowserWebDriverContainer.VncRecordingMode recordingMode
-    VncRecordingContainer.VncRecordingFormat recordingFormat
+	VncRecordingMode recordingMode
+	VncRecordingFormat recordingFormat
 
     RecordingSettings() {
         recordingDirectoryName = System.getProperty('grails.geb.recording.directory', 'build/recordings')
-        recordingMode = BrowserWebDriverContainer.VncRecordingMode.valueOf(System.getProperty('grails.geb.recording.mode', BrowserWebDriverContainer.VncRecordingMode.RECORD_FAILING.name()))
-        recordingFormat = VncRecordingContainer.VncRecordingFormat.valueOf(System.getProperty('grails.geb.recording.format', VncRecordingContainer.VncRecordingFormat.MP4.name()))
+        recordingMode = VncRecordingMode.valueOf(System.getProperty('grails.geb.recording.mode', DEFAULT_RECORDING_MODE.name()))
+        recordingFormat = VncRecordingFormat.valueOf(System.getProperty('grails.geb.recording.format', DEFAULT_RECORDING_FORMAT.name()))
+    }
+
+    boolean isRecordingEnabled() {
+        recordingMode != VncRecordingMode.SKIP
     }
 
     @Memoized
     File getRecordingDirectory() {
-        if (recordingMode == BrowserWebDriverContainer.VncRecordingMode.SKIP) {
+        if (!recordingEnabled) {
             return null
         }
 
         File recordingDirectory = new File(recordingDirectoryName)
         if (!recordingDirectory.exists()) {
-            log.info("Could not find `{}` directory for recording.  Creating...", recordingDirectoryName)
+            log.info('Could not find `{}` Directory for recording. Creating...', recordingDirectoryName)
             recordingDirectory.mkdirs()
         } else if (!recordingDirectory.isDirectory()) {
-            throw new IllegalStateException("Recording Directory name expected to be `${recordingDirectoryName}, but found file instead.")
+            throw new IllegalStateException("Configured recording directory '${recordingDirectory}' is expected to be a directory, but found file instead.")
         }
 
         return recordingDirectory

diff --git a/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy b/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy
@@ -36,24 +36,25 @@ import java.time.Duration
  * @since 5.0
  */
 class WebDriverContainerHolder {
+
     RecordingSettings recordingSettings
     WebDriverContainerConfiguration configuration
-    BrowserWebDriverContainer current
+    BrowserWebDriverContainer currentContainer
 
     WebDriverContainerHolder(RecordingSettings recordingSettings) {
         this.recordingSettings = recordingSettings
     }
 
     boolean isInitialized() {
-        current != null
+        currentContainer != null
     }
 
     boolean stop() {
-        if (!current) {
+        if (!currentContainer) {
             return false
         }
-        current.stop()
-        current = null
+        currentContainer.stop()
+        currentContainer = null
         configuration = null
         return true
     }
@@ -71,36 +72,39 @@ class WebDriverContainerHolder {
     }
 
     boolean reinitialize(IMethodInvocation invocation) {
-        WebDriverContainerConfiguration specConfiguration = new WebDriverContainerConfiguration(getPort(invocation), invocation.getSpec())
+        WebDriverContainerConfiguration specConfiguration = new WebDriverContainerConfiguration(
+                getPort(invocation),
+                invocation.getSpec()
+        )
         if (matchesCurrentContainerConfiguration(specConfiguration)) {
             return false
         }
 
-        if (isInitialized()) {
+        if (initialized) {
             stop()
         }
 
         configuration = specConfiguration
-        current = new BrowserWebDriverContainer()
+        currentContainer = new BrowserWebDriverContainer()
         Testcontainers.exposeHostPorts(configuration.port)
-        current.tap {
+        currentContainer.tap {
             addExposedPort(configuration.port)
             withAccessToHost(true)
             start()
         }
-        if (!isDefaultHostname()) {
-            current.execInContainer('/bin/sh', '-c', "echo '$hostIp\t${configuration.hostName}' | sudo tee -a /etc/hosts")
+        if (hostnameChanged) {
+            currentContainer.execInContainer('/bin/sh', '-c', "echo '$hostIp\t${configuration.hostName}' | sudo tee -a /etc/hosts")
         }
 
-        if (recordingSettings.recordingMode != BrowserWebDriverContainer.VncRecordingMode.SKIP) {
-            current = current.withRecordingMode(
+        if (recordingSettings.recordingEnabled) {
+            currentContainer = currentContainer.withRecordingMode(
                     recordingSettings.recordingMode,
                     recordingSettings.recordingDirectory,
                     recordingSettings.recordingFormat
             )
         }
 
-        WebDriver driver = new RemoteWebDriver(current.seleniumAddress, new ChromeOptions())
+        WebDriver driver = new RemoteWebDriver(currentContainer.seleniumAddress, new ChromeOptions())
         driver.manage().timeouts().implicitlyWait(Duration.ofSeconds(30))
 
         // Update the browser to use this container
@@ -111,22 +115,25 @@ class WebDriverContainerHolder {
         return true
     }
 
-    private boolean isDefaultHostname() {
-        configuration.hostName == ContainerGebConfiguration.DEFAULT_HOSTNAME_FROM_CONTAINER
+    private boolean getHostnameChanged() {
+        configuration.hostName != ContainerGebConfiguration.DEFAULT_HOSTNAME_FROM_CONTAINER
     }
 
     private String getHostIp() {
-        current.getContainerInfo().getNetworkSettings().getNetworks().entrySet().first().value.ipAddress
+        currentContainer.getContainerInfo().getNetworkSettings().getNetworks().entrySet().first().value.ipAddress
     }
 
     @EqualsAndHashCode
     private static class WebDriverContainerConfiguration {
+
         String protocol
         String hostName
         int port
 
         WebDriverContainerConfiguration(int port, SpecInfo spec) {
-            ContainerGebConfiguration configuration = spec.annotations.find { it.annotationType() == ContainerGebConfiguration } as ContainerGebConfiguration
+            ContainerGebConfiguration configuration = spec.annotations.find {
+                it.annotationType() == ContainerGebConfiguration
+            } as ContainerGebConfiguration
 
             protocol = configuration?.protocol() ?: ContainerGebConfiguration.DEFAULT_PROTOCOL
             hostName = configuration?.hostName() ?: ContainerGebConfiguration.DEFAULT_HOSTNAME_FROM_CONTAINER