feedback - formating, use existing enum instead of recording flag, an…

…d clean-up

README.md

@@ -57,22 +57,18 @@ Just run `./gradlew integrationTest` and a container will be started and configu
 By default, all failed tests will generate a video recording of the test execution. These recordings are saved to a `recordings` directory under the project's `build` directory.  
 
 The following system properties can be change to configure the recording behavior:
-* `grails.geb.recording.enabled`
-  * purpose: toggle for recording
-  * possible values: `true` or `false`
+
+* `grails.geb.recording.mode`
+  * purpose: which tests to record
+  * possible values: `SKIP`, `RECORD_ALL`, or `RECORD_FAILING`
+  * defaults to `RECORD_FAILING`
 
 
 * `grails.geb.recording.directory`
     * purpose: the directory to save the recordings relative to the project directory
     * defaults to `build/recordings`
 
 
-* `grails.geb.recording.mode`
-    * purpose: which tests to record via the enum `VncRecordingMode`
-    * possible values: `RECORD_ALL` or `RECORD_FAILING`
-    * defaults to `RECORD_FAILING`
-
-
 * `grails.geb.recording.format`
     * purpose: sets the format of the recording
     * possible values are `FLV` or `MP4`

src/testFixtures/groovy/grails/plugin/geb/ContainerAwareDownloadSupport.groovy

@@ -35,7 +35,7 @@ import java.util.regex.Pattern
  * setups, ensuring the host network context is used for download requests.</p>
  *
  * @author Mattias Reichel
- * @since 5.0.0
+ * @since 5.0
  */
 @CompileStatic
 @SelfType(ContainerGebSpec)
@@ -45,6 +45,7 @@ trait ContainerAwareDownloadSupport implements DownloadSupport {
     private final DownloadSupport downloadSupport = new LocalhostDownloadSupport(browser, this)
 
     abstract Browser getBrowser()
+
     abstract String getHostNameFromHost()
 
     private static class LocalhostDownloadSupport extends DefaultDownloadSupport {

src/testFixtures/groovy/grails/plugin/geb/ContainerGebConfiguration.groovy

@@ -17,50 +17,44 @@ package grails.plugin.geb
 
 import groovy.transform.CompileStatic
 import groovy.transform.Memoized
-import groovy.transform.PackageScope
 import groovy.util.logging.Slf4j
 import org.testcontainers.containers.BrowserWebDriverContainer
 import org.testcontainers.containers.VncRecordingContainer
 
 /**
- * A container class to parse the configuration used by {@link grails.plugin.geb.ContainerGebRecordingExtension}
+ * Handles parsing various configuration used by {@link grails.plugin.geb.ContainerGebRecordingExtension}
  *
  * @author James Daugherty
- * @since 5.0.0
+ * @since 5.0
  */
 @Slf4j
 @CompileStatic
 class ContainerGebConfiguration {
-    String recordingDirectoryName
-
-    boolean recording
 
+    String recordingDirectoryName
     BrowserWebDriverContainer.VncRecordingMode recordingMode
-
     VncRecordingContainer.VncRecordingFormat recordingFormat
 
     ContainerGebConfiguration() {
-        recording = Boolean.parseBoolean(System.getProperty('grails.geb.recording.enabled', true as String))
         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()))
     }
 
     @Memoized
     File getRecordingDirectory() {
-        if(!recording) {
+        if (recordingMode == BrowserWebDriverContainer.VncRecordingMode.SKIP) {
             return null
         }
 
         File recordingDirectory = new File(recordingDirectoryName)
-        if(!recordingDirectory.exists()) {
-            log.info("Could not find `${recordingDirectoryName}` directory for recording.  Creating...")
-            recordingDirectory.mkdir()
-        }
-        else if(!recordingDirectory.isDirectory()) {
+        if (!recordingDirectory.exists()) {
+            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.")
         }
 
-        recordingDirectory
+        return recordingDirectory
     }
 }

src/testFixtures/groovy/grails/plugin/geb/ContainerGebRecordingExtension.groovy

@@ -20,14 +20,15 @@ import groovy.transform.TailRecursive
 import groovy.util.logging.Slf4j
 import org.spockframework.runtime.extension.IGlobalExtension
 import org.spockframework.runtime.model.SpecInfo
+import org.testcontainers.containers.BrowserWebDriverContainer
 
 import java.time.LocalDateTime
 
 /**
  * A Spock Extension that manages the Testcontainers lifecycle for a {@link grails.plugin.geb.ContainerGebSpec}
  *
  * @author James Daugherty
- * @since 5.0.0
+ * @since 5.0
  */
 @Slf4j
 @CompileStatic
@@ -41,14 +42,19 @@ class ContainerGebRecordingExtension implements IGlobalExtension {
 
     @Override
     void visitSpec(SpecInfo spec) {
-        if (isContainerizedGebSpec(spec)) {
+        if (isContainerGebSpec(spec)) {
             ContainerGebTestListener listener = new ContainerGebTestListener(spec, LocalDateTime.now())
             // TODO: We should initialize the web driver container once for all geb tests so we don't have to spin it up & down.
             spec.addSetupInterceptor {
                 ContainerGebSpec gebSpec = it.instance as ContainerGebSpec
                 gebSpec.initialize()
-                if(configuration.recording) {
-                    listener.webDriverContainer = gebSpec.webDriverContainer.withRecordingMode(configuration.recordingMode, configuration.recordingDirectory, configuration.recordingFormat)
+                if (configuration.recordingMode != BrowserWebDriverContainer.VncRecordingMode.SKIP) {
+                    listener.webDriverContainer = gebSpec.webDriverContainer
+                            .withRecordingMode(
+                                    configuration.recordingMode,
+                                    configuration.recordingDirectory,
+                                    configuration.recordingFormat
+                            )
                 }
             }
             spec.addCleanupInterceptor {
@@ -61,13 +67,12 @@ class ContainerGebRecordingExtension implements IGlobalExtension {
     }
 
     @TailRecursive
-    private boolean isContainerizedGebSpec(SpecInfo spec) {
-        if(spec != null) {
-            if(spec.filename.startsWith('ContainerGebSpec.')) {
+    private boolean isContainerGebSpec(SpecInfo spec) {
+        if (spec != null) {
+            if (spec.filename.startsWith('ContainerGebSpec.')) {
                 return true
             }
-
-            return isContainerizedGebSpec(spec.superSpec)
+            return isContainerGebSpec(spec.superSpec)
         }
         return false
     }

src/testFixtures/groovy/grails/plugin/geb/ContainerGebSpec.groovy

@@ -46,7 +46,7 @@ import java.time.Duration
  *
  * @author Søren Berg Glasius
  * @author Mattias Reichel
- * @since 5.0.0
+ * @since 5.0
  */
 @DynamicallyDispatchesToBrowser
 abstract class ContainerGebSpec extends Specification implements ManagedGebTest, ContainerAwareDownloadSupport {
@@ -72,7 +72,7 @@ abstract class ContainerGebSpec extends Specification implements ManagedGebTest,
 
     @PackageScope
     void initialize() {
-        if(webDriverContainer) {
+        if (webDriverContainer) {
             return
         }
 

src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestDescription.groovy

@@ -26,19 +26,18 @@ import java.time.format.DateTimeFormatter
  * Implements {@link org.testcontainers.lifecycle.TestDescription} to customize recording names.
  *
  * @see org.testcontainers.lifecycle.TestDescription
- *
  * @author James Daugherty
  * @since 5.0
  */
 @CompileStatic
 class ContainerGebTestDescription implements TestDescription {
+
     String testId
     String filesystemFriendlyName
 
     ContainerGebTestDescription(IterationInfo testInfo, LocalDateTime runDate) {
         testId = testInfo.displayName
-
-        String safeName = testId.replaceAll("\\W+", "")
+        String safeName = testId.replaceAll('\\W+', '_')
         filesystemFriendlyName = "${DateTimeFormatter.ofPattern("yyyyMMdd_HHmmss").format(runDate)}_${safeName}"
     }
 }

src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestListener.groovy

@@ -35,10 +35,11 @@ import java.time.LocalDateTime
  */
 @CompileStatic
 class ContainerGebTestListener extends AbstractRunListener {
+
     BrowserWebDriverContainer webDriverContainer
-	ErrorInfo errorInfo
-	SpecInfo spec
-	LocalDateTime runDate
+    ErrorInfo errorInfo
+    SpecInfo spec
+    LocalDateTime runDate
 
     ContainerGebTestListener(SpecInfo spec, LocalDateTime runDate) {
         this.spec = spec
@@ -47,7 +48,10 @@ class ContainerGebTestListener extends AbstractRunListener {
 
     @Override
     void afterIteration(IterationInfo iteration) {
-        webDriverContainer.afterTest(new ContainerGebTestDescription(iteration, runDate), Optional.of(errorInfo?.exception))
+        webDriverContainer.afterTest(
+                new ContainerGebTestDescription(iteration, runDate),
+                Optional.ofNullable(errorInfo?.exception)
+        )
         errorInfo = null
     }