From 7d7b39d5d0f612e5cab7459205bcc073f1331f6e Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Tue, 21 Jul 2026 15:54:51 -0500 Subject: [PATCH 1/3] Fix flaky PerTestRecordingSpec by waiting for recordings to stabilize waitForRecordingFiles() polled the recordings directory for matching .mp4/.flv files but only checked that the expected file count existed by name, never that each file's content was fully written. Testcontainers' VncRecordingContainer.saveRecordingToFile() copies the recording with a plain, non-atomic Files.copy(..., REPLACE_EXISTING), so a destination file becomes visible to the directory scan the moment the copy starts - potentially at 0 bytes or mid-write. Two still-partial recordings (e.g. both 0 bytes) can register as byte-identical, intermittently failing the "recordings of the previous two tests are different" assertion (apache/grails-core#16030). Track each candidate file's size across polls and only treat it as ready once its size is non-zero and unchanged from the previous poll, which means the copy has finished. Keeps the existing 10s timeout / 500ms poll interval. Verified with :grails-test-examples-geb:integrationTest (PerTestRecordingSpec) - all three iterations pass, including the comparison assertion. --- .../demo/spock/PerTestRecordingSpec.groovy | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy b/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy index 808d90c9c44..3023fc609e6 100644 --- a/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy +++ b/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy @@ -123,21 +123,36 @@ class PerTestRecordingSpec extends ContainerGebSpec { long pollIntervalMillis = 500L ) { long deadline = System.currentTimeMillis() + timeoutMillis - List recordingFiles = [] + Map previousSizes = [:] + List readyFiles = [] while (System.currentTimeMillis() < deadline) { // Re-scan on every poll: the directory and the files both appear // asynchronously while the recording container flushes videos. - recordingFiles = currentRunRecordingDirs(baseRecordingDir).collectMany { File dir -> + List candidateFiles = currentRunRecordingDirs(baseRecordingDir).collectMany { File dir -> (dir.listFiles({ File file -> isVideoFile(file) && file.name.contains(testClassName) } as FileFilter) ?: new File[0]) as List } + Map currentSizes = candidateFiles.collectEntries { File file -> + [(file.absolutePath): file.length()] + } + + // Testcontainers copies each recording with a plain, non-atomic + // stream copy, so a file can appear in the directory scan above + // while still 0 bytes or only partially written. Only treat a + // recording as ready once its size is non-zero and unchanged + // since the previous poll, which means the copy has finished. + readyFiles = candidateFiles.findAll { File file -> + long size = currentSizes[file.absolutePath] + size > 0 && previousSizes[file.absolutePath] == size + } - if (recordingFiles.size() >= minFileCount) { + if (readyFiles.size() >= minFileCount) { break } + previousSizes = currentSizes sleep(pollIntervalMillis) } - return recordingFiles + return readyFiles } } From 24a0b59c1b3d6916d9c2515b362e7f44d64abcf4 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Sun, 26 Jul 2026 22:24:50 -0500 Subject: [PATCH 2/3] Fix the real causes behind PerTestRecordingSpec's flakiness Review on this PR (apache/grails-core#16031) showed the original fix's diagnosis doesn't hold: GebRecordingTestListener.afterIteration calls BrowserWebDriverContainer.afterTest -> saveRecordingToFile synchronously on the test thread, and saveRecordingToFile's own Files.copy blocks until the copy is done - verified directly against the Testcontainers 2.0.5 source. There is no concurrent writer for a directory scan to race against, so the size-stability polling added here didn't remove a race; it just added latency. The more likely real cause: a VNC recording container that was just restarted (WebDriverContainerHolder#restartVncRecordingContainer) is only guaranteed to have connected, not to have captured meaningful frames, by the time a fast test iteration finishes. Two such near-blank captures can encode to identical, non-zero, size-stable bytes via ffmpeg, passing both the old and the new check without being distinct, meaningful recordings. - PerTestRecordingSpec: revert the stability-polling change, and instead assert each recording independently exceeds a sensible minimum size before asserting the two differ - this is the actual framework contract, not raw byte inequality of ffmpeg output. - WebDriverContainerHolder#restartVncRecordingContainer: fix a real bug found while investigating - the vncRecordingContainer field was set to the new container BEFORE start() was called, so a thrown (and swallowed) start() failure left the field pointing at a container that never actually started. - WebDriverContainerHolder#stop: wrap container?.stop() in try/finally so a thrown stop() also can't skip the state reset, leaving isInitialized() reporting true for a broken container. Verified against the real Testcontainers-backed integration test (PerTestRecordingSpec, 3/3 passing) and repo-wide codeStyle. Co-Authored-By: Claude Sonnet 5 --- .../geb/WebDriverContainerHolder.groovy | 26 ++++++++---- .../demo/spock/PerTestRecordingSpec.groovy | 41 ++++++++++--------- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy b/grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy index 014e61ab176..eace148a1ac 100644 --- a/grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy +++ b/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 + testManager = null + containerConf = null + } } boolean matchesCurrentContainerConfiguration(WebDriverContainerConfiguration specConf) { @@ -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) log.debug('Successfully restarted VNC recording container') } diff --git a/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy b/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy index 3023fc609e6..a9de5fa3a81 100644 --- a/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy +++ b/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy @@ -85,12 +85,28 @@ 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 - well under any real capture observed locally (tens of KB), but + // well above a single near-blank keyframe - asserts the real framework contract. 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 + secondRecording.length() > MIN_MEANINGFUL_RECORDING_BYTES + + and: 'the recording files should have different content' Files.mismatch(firstRecording.toPath(), secondRecording.toPath()) != -1 } + // Comfortably below every real capture observed locally (tens of KB) but well above + // what a single near-blank keyframe from a just-restarted VNC connection would encode to. + private static final long MIN_MEANINGFUL_RECORDING_BYTES = 5_000L + private static final DateTimeFormatter RECORDING_DIR_FORMAT = DateTimeFormatter.ofPattern('yyyyMMdd_HHmmss') private static final LocalDateTime JVM_START = LocalDateTime.ofInstant( @@ -123,36 +139,21 @@ class PerTestRecordingSpec extends ContainerGebSpec { long pollIntervalMillis = 500L ) { long deadline = System.currentTimeMillis() + timeoutMillis - Map previousSizes = [:] - List readyFiles = [] + List recordingFiles = [] while (System.currentTimeMillis() < deadline) { // Re-scan on every poll: the directory and the files both appear // asynchronously while the recording container flushes videos. - List candidateFiles = currentRunRecordingDirs(baseRecordingDir).collectMany { File dir -> + recordingFiles = currentRunRecordingDirs(baseRecordingDir).collectMany { File dir -> (dir.listFiles({ File file -> isVideoFile(file) && file.name.contains(testClassName) } as FileFilter) ?: new File[0]) as List } - Map currentSizes = candidateFiles.collectEntries { File file -> - [(file.absolutePath): file.length()] - } - - // Testcontainers copies each recording with a plain, non-atomic - // stream copy, so a file can appear in the directory scan above - // while still 0 bytes or only partially written. Only treat a - // recording as ready once its size is non-zero and unchanged - // since the previous poll, which means the copy has finished. - readyFiles = candidateFiles.findAll { File file -> - long size = currentSizes[file.absolutePath] - size > 0 && previousSizes[file.absolutePath] == size - } - if (readyFiles.size() >= minFileCount) { + if (recordingFiles.size() >= minFileCount) { break } - previousSizes = currentSizes sleep(pollIntervalMillis) } - return readyFiles + return recordingFiles } } From 08800e41145a919ec2c2cbe9b10d80b23a35b6f1 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Fri, 31 Jul 2026 20:17:08 -0500 Subject: [PATCH 3/3] Add unit coverage for WebDriverContainerHolder and ground the recording size floor in real measurements jdaugherty's review on PR #16031 flagged two gaps in the previous commit: WebDriverContainerHolder's stop()/restartVncRecordingContainer() changes had no test coverage of their own, and MIN_MEANINGFUL_RECORDING_BYTES was justified only by an unquantified "tens of KB" claim. - Add WebDriverContainerHolderSpec: unit-tests stop()'s try/finally state reset (both the happy path and when container.stop() throws), and restartVncRecordingContainer()'s guard clauses plus its swallow-and-log behavior when the current recording container fails to stop. All exercised via Mocks - no Docker required. - PerTestRecordingSpec: replace the vague size justification with numbers from two real local runs against the VNC recording container (75KB-855KB per genuine recording), and document why the byte-mismatch check is kept alongside the size floor rather than replaced by it - the two assertions guard against different failure modes (near-blank captures vs. two recordings accidentally being the same file). Co-Authored-By: Claude Sonnet 5 --- .../geb/WebDriverContainerHolderSpec.groovy | 137 ++++++++++++++++++ .../demo/spock/PerTestRecordingSpec.groovy | 15 +- 2 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 grails-geb/src/test/groovy/grails/plugin/geb/WebDriverContainerHolderSpec.groovy diff --git a/grails-geb/src/test/groovy/grails/plugin/geb/WebDriverContainerHolderSpec.groovy b/grails-geb/src/test/groovy/grails/plugin/geb/WebDriverContainerHolderSpec.groovy new file mode 100644 index 00000000000..df960932d40 --- /dev/null +++ b/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') + vncField.accessible = true + vncField.set(container, vncContainer) + + when: 'restarting the recording container' + holder.restartVncRecordingContainer() + + then: 'the failure is logged and swallowed rather than breaking test execution' + noExceptionThrown() + + and: "the field is left untouched - it still points at the container that just failed to stop, not a container that never started" + vncField.get(container).is(vncContainer) + } +} diff --git a/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy b/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy index a9de5fa3a81..bd22ed58e34 100644 --- a/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy +++ b/grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy @@ -92,19 +92,26 @@ class PerTestRecordingSpec extends ContainerGebSpec { // 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 - well under any real capture observed locally (tens of KB), but - // well above a single near-blank keyframe - asserts the real framework contract. + // 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 secondRecording.length() > MIN_MEANINGFUL_RECORDING_BYTES and: 'the recording files should have different content' + // Kept alongside the size check above rather than dropped in favor of it: the size + // check only rules out near-blank captures, it says nothing about two recordings + // accidentally being the *same* file (e.g. a future regression in how recording + // files are named or matched). The two checks guard against different failure modes. Files.mismatch(firstRecording.toPath(), secondRecording.toPath()) != -1 } - // Comfortably below every real capture observed locally (tens of KB) but well above - // what a single near-blank keyframe from a just-restarted VNC connection would encode to. + // Two local runs against a real VNC recording container measured every genuine, + // played-out recording at 75KB-855KB (org.demo.spock.PerTestRecordingSpec, recorded + // 2026-07-21 and 2026-07-31). 5,000 bytes stays more than an order of magnitude below + // the smallest of those, while still comfortably clearing a single near-blank keyframe + // from a just-restarted VNC connection. private static final long MIN_MEANINGFUL_RECORDING_BYTES = 5_000L private static final DateTimeFormatter RECORDING_DIR_FORMAT = DateTimeFormatter.ofPattern('yyyyMMdd_HHmmss')