jdaugherty commented on code in PR #16031:
URL: https://github.com/apache/grails-core/pull/16031#discussion_r3821956670
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -440,13 +447,18 @@ class WebDriverContainerHolder {
if (vncContainer) {
// Stop the current VNC recording container
vncContainer.stop()
- // Create and start a new VNC recording container for the next
test
+ // Create and start a new VNC recording container for the next
test.
+ // start() must succeed BEFORE the field is updated: if it
throws (e.g. the
+ // "Connected" wait strategy times out), the exception below
is deliberately
+ // swallowed to avoid breaking test execution - so if the
field were already
+ // pointing at newVncContainer by then, every subsequent
saveRecordingToFile()
+ // would silently target a container that never actually
started.
def newVncContainer = new VncRecordingContainer(container)
.withVncPassword('secret')
.withVncPort(5900)
.withVideoFormat(settings.recordingFormat)
- field.set(container, newVncContainer)
newVncContainer.start()
+ field.set(container, newVncContainer)
Review Comment:
If `newVncContainer.start()` throws, the swallow below leaves this field
still pointing at the **old** container — which `vncContainer.stop()` above has
already stopped *and removed* (testcontainers' `GenericContainer.stop()`
removes the container). The next `afterIteration` → `saveRecordingToFile()`
then hits a docker `NotFoundException` whose message names the missing
container id, not `/newScreen.mp4`, so `GebRecordingTestListener`'s guard
rethrows it and a passing test gets reported as an error — the exact outcome
the swallow is meant to prevent.
Consider clearing the field (`field.set(container, null)`) in the failure
path so the save is skipped cleanly instead of throwing.
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -440,13 +447,18 @@ class WebDriverContainerHolder {
if (vncContainer) {
// Stop the current VNC recording container
vncContainer.stop()
- // Create and start a new VNC recording container for the next
test
+ // Create and start a new VNC recording container for the next
test.
+ // start() must succeed BEFORE the field is updated: if it
throws (e.g. the
+ // "Connected" wait strategy times out), the exception below
is deliberately
+ // swallowed to avoid breaking test execution - so if the
field were already
+ // pointing at newVncContainer by then, every subsequent
saveRecordingToFile()
+ // would silently target a container that never actually
started.
def newVncContainer = new VncRecordingContainer(container)
.withVncPassword('secret')
.withVncPort(5900)
.withVideoFormat(settings.recordingFormat)
- field.set(container, newVncContainer)
newVncContainer.start()
Review Comment:
Related: when `start()` throws after the docker container has already been
created (e.g. the `Connected` wait strategy times out), nothing references or
stops `newVncContainer` anymore — a live ffmpeg recorder stays attached to the
browser container's VNC port until Ryuk reaps it at JVM exit. The previous
ordering self-healed: the field held the failed container, so the next
restart's `vncContainer.stop()` cleaned it up.
A `try { newVncContainer.start(); field.set(container, newVncContainer) }
catch (Exception e) { newVncContainer.stop(); throw e }` around just the start
would cover both this and the stale-field issue above.
##########
grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy:
##########
@@ -85,12 +85,35 @@ class PerTestRecordingSpec extends ContainerGebSpec {
names.contains('setup_running_a_test_to_create_a_recording')
names.contains('setup_running_a_second_test_to_create_another')
- and: 'the recording files should have different content'
+ and: 'each recording captured meaningful content, not just a
near-blank connection handshake'
+ // A VNC recording container that was only just restarted (see
+ // WebDriverContainerHolder#restartVncRecordingContainer) is
guaranteed to have
+ // connected, but not to have captured more than a frame or two by the
time a fast
+ // iteration finishes. Two such near-blank captures can encode to
identical,
+ // non-zero, stable-sized bytes via ffmpeg - passing a raw
byte-difference check
+ // without actually being distinct, meaningful recordings. Requiring a
sensible
+ // minimum size asserts the real framework contract - a real,
played-out recording -
+ // rather than raw byte inequality of whatever ffmpeg happened to
produce.
def firstRecording = recordingFiles.find {
it.name.contains('setup_running_a_test_to_create_a_recording') }
def secondRecording = recordingFiles.find {
it.name.contains('setup_running_a_second_test_to_create_another') }
+ firstRecording.length() > MIN_MEANINGFUL_RECORDING_BYTES
Review Comment:
Two concerns with the size check and its justification:
1. The comment misstates the old check's semantics: two identical near-blank
captures make `Files.mismatch` return `-1`, which *failed* the pre-existing `!=
-1` assertion — they couldn't "pass a raw byte-difference check".
2. In the exact near-blank scenario the comment describes (recorder just
restarted, fast iteration finishes before more than a frame or two is
captured), these size assertions also fail — so that flake is relabeled rather
than eliminated. And the 5,000-byte floor is calibrated from only two local
runs; a short, low-motion encode on a slow-to-restart CI iteration could
legitimately land below it.
If the real contract is "a played-out recording exists per test", something
like waiting for the file to stop growing / reach the floor (with a timeout)
before asserting would be less flaky than a static minimum on whatever ffmpeg
had flushed at iteration end.
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -88,11 +88,18 @@ class WebDriverContainerHolder {
}
void stop() {
- container?.stop()
- container = null
- browser = null
- testManager = null
- containerConf = null
+ try {
+ container?.stop()
+ } finally {
+ // Reset state even if stop() throws - otherwise isInitialized()
keeps reporting
+ // true for a container that's actually broken, and a later
reinitialize() call
+ // would see matchesCurrentContainerConfiguration() as a false
positive without
+ // ever attempting to recover.
+ container = null
+ browser = null
Review Comment:
While touching `stop()`: the comment in `reinitialize()` says "The driver is
explicitly quit by us in stop() method", but `stop()` never calls
`browser?.driver?.quit()` — it only nulls the reference. Since
`cacheDriver=false` and `quitDriverOnBrowserReset=false` were set precisely on
that promise (disabling Geb's own cleanup), every container recycle abandons a
`RemoteWebDriver` and its HTTP client/connection pool. Worth either quitting
the driver here (inside the `try`, before `container?.stop()`) or correcting
the comment.
##########
grails-geb/src/test/groovy/grails/plugin/geb/WebDriverContainerHolderSpec.groovy:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.plugin.geb
+
+import java.time.LocalDateTime
+
+import org.testcontainers.containers.BrowserWebDriverContainer
+import org.testcontainers.containers.VncRecordingContainer
+
+import geb.Browser
+import geb.test.GebTestManager
+import spock.lang.Specification
+
+import static
org.testcontainers.containers.BrowserWebDriverContainer.VncRecordingMode
+
+class WebDriverContainerHolderSpec extends Specification {
+
+ WebDriverContainerHolder holder = new WebDriverContainerHolder(new
GrailsGebSettings(LocalDateTime.now()))
Review Comment:
`GrailsGebSettings` reads live JVM system properties, and
`gradle/test-config.gradle` forwards every `grails.geb.*` project property into
test JVMs — so an externally supplied value (e.g. a malformed
`grails.geb.recording.mode` or `.format` from a developer's gradle properties)
makes this field initializer throw in `VncRecordingMode.valueOf` and errors all
five features before any assertion runs. Constructing the settings hermetically
here (explicit values, or saving/clearing the relevant system properties around
the spec) would isolate the tests from the environment.
##########
grails-geb/src/test/groovy/grails/plugin/geb/WebDriverContainerHolderSpec.groovy:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.plugin.geb
+
+import java.time.LocalDateTime
+
+import org.testcontainers.containers.BrowserWebDriverContainer
+import org.testcontainers.containers.VncRecordingContainer
+
+import geb.Browser
+import geb.test.GebTestManager
+import spock.lang.Specification
+
+import static
org.testcontainers.containers.BrowserWebDriverContainer.VncRecordingMode
+
+class WebDriverContainerHolderSpec extends Specification {
+
+ WebDriverContainerHolder holder = new WebDriverContainerHolder(new
GrailsGebSettings(LocalDateTime.now()))
+
+ void 'stop() resets container, browser and testManager on the happy
path'() {
+ given: 'a holder with an initialized container'
+ def container = Mock(BrowserWebDriverContainer)
+ holder.container = container
+ holder.browser = Mock(Browser)
+ holder.testManager = Mock(GebTestManager)
+
+ when: 'the holder is stopped'
+ holder.stop()
+
+ then: 'the underlying container is stopped'
+ 1 * container.stop()
+
+ and: 'all held state is cleared'
+ holder.container == null
+ holder.browser == null
+ holder.testManager == null
+ !holder.initialized
+ }
+
+ void 'stop() still resets all held state when container.stop() throws'() {
+ given: 'a holder whose container fails to stop cleanly'
+ def container = Mock(BrowserWebDriverContainer)
+ container.stop() >> { throw new IllegalStateException('boom') }
+ holder.container = container
+ holder.browser = Mock(Browser)
+ holder.testManager = Mock(GebTestManager)
+
+ when: 'the holder is stopped'
+ holder.stop()
+
+ then: 'the exception from stop() propagates'
+ thrown(IllegalStateException)
+
+ and: 'held state is still cleared, so a broken container is never
reported as initialized'
+ holder.container == null
+ holder.browser == null
+ holder.testManager == null
+ !holder.initialized
+ }
+
+ void 'restartVncRecordingContainer() does nothing when recording is
disabled'() {
+ given:
+ holder.settings.recordingMode = VncRecordingMode.SKIP
+ holder.settings.restartRecordingContainerPerTest = true
+ def container = Mock(BrowserWebDriverContainer)
+ holder.container = container
+
+ when:
+ holder.restartVncRecordingContainer()
+
+ then:
+ 0 * container._
+ }
+
+ void 'restartVncRecordingContainer() does nothing when per-test restart is
disabled'() {
+ given:
+ holder.settings.recordingMode = VncRecordingMode.RECORD_ALL
+ holder.settings.restartRecordingContainerPerTest = false
+ def container = Mock(BrowserWebDriverContainer)
+ holder.container = container
+
+ when:
+ holder.restartVncRecordingContainer()
+
+ then:
+ 0 * container._
+ }
+
+ void 'restartVncRecordingContainer() does nothing when no container has
been initialized'() {
+ given:
+ holder.settings.recordingMode = VncRecordingMode.RECORD_ALL
+ holder.settings.restartRecordingContainerPerTest = true
+ holder.container = null
+
+ expect: 'no exception is thrown even though there is nothing to
restart'
+ holder.restartVncRecordingContainer()
+ }
+
+ void 'restartVncRecordingContainer() swallows a failure from the current
recording container instead of propagating it'() {
+ given: 'a container whose active VNC recording container fails to stop'
+ holder.settings.recordingMode = VncRecordingMode.RECORD_ALL
+ holder.settings.restartRecordingContainerPerTest = true
+ def container = Mock(BrowserWebDriverContainer)
+ holder.container = container
+
+ def vncContainer = Mock(VncRecordingContainer)
+ vncContainer.stop() >> { throw new IllegalStateException('vnc
container refused to stop') }
+ def vncField =
BrowserWebDriverContainer.getDeclaredField('vncRecordingContainer')
Review Comment:
This test injects and asserts state through testcontainers' private
`vncRecordingContainer` field via reflection, which runs against the repo test
conventions (CLAUDE.md rule 9: test via public APIs, never bypass the public
surface). It also pins a third-party private implementation detail — a field
rename in a testcontainers upgrade breaks this test with no public-API contract
explaining why. The production code admittedly uses the same reflection, but
the assertion here could target observable behavior instead (e.g. that a
subsequent recording save/restart still behaves after a failed stop) rather
than re-reading the private field.
--
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]
