matrei opened a new pull request, #16405: URL: https://github.com/apache/grails-core/pull/16405
Follow-up to #16031. ## Problem With `grails.geb.recording.restartRecordingContainerPerTest` enabled, `WebDriverContainerHolder#restartVncRecordingContainer` stops the current VNC recording container before each test and starts a replacement. Since #16031, the stopped container is cleared first, so a failed start no longer leaves a reference to a removed container. But it left two problems: - **One failed restart disabled recording for the rest of the browser container's life.** With nothing left to stop, every later restart skipped entirely. All remaining tests ran unrecorded, and the only trace was a single warning. - **`GebRecordingTestListener` caught `NullPointerException` to detect the missing recording container.** With per-test restart enabled, that swallowed every NPE from `afterIteration` at debug level, including ones unrelated to recording. ## Changes - **Restart recovers.** `restartVncRecordingContainer` now starts a new recording container even when there is none to stop, so recording resumes with the next test after a failure. A replacement that fails to start is still stopped, so it isn't left attached to the browser container. - **Explicit check instead of the NPE catch.** New `WebDriverContainerHolder#isRecordingContainerAvailable()`. The listener skips saving when recording is enabled but no recording container is available. Any other exception now propagates. - **Replacement creation is overridable.** It moves into `protected createVncRecordingContainer()`. - **`PerTestRecordingSpec` asserts the minimum recording size directly.** The recordings are saved synchronously before the comparing feature runs, so polling their size could not change the result. It only delayed the failure by 10 s. ## Tests The specs added in #16031 passed without exercising what they described: - The "replacement fails to start" case failed in the `VncRecordingContainer` constructor (a bare mock returns `null` network aliases), so `start()` and the new cleanup path were never reached. - The listener specs passed a bare `Mock(IterationInfo)`, so `ContainerGebTestDescription` threw an NPE before `afterTest` was called. Both the "swallows" and the "re-throws" cases were asserting against that NPE. Changes to the specs: - `WebDriverContainerHolderSpec` supplies replacement containers through `createVncRecordingContainer()`. It covers a normal restart, a replacement that fails to start and is stopped, and recovery on the next restart. - `GebRecordingTestListenerSpec` passes the running iteration (`specificationContext.currentIteration`) and asserts the recording name passed to the container. - Assertions no longer read the private Testcontainers field. It is still set by reflection in the setup, as the production code does. - Both specs use `@RestoreSystemProperties` instead of hand-rolled save/restore of the `grails.geb.*` properties. I checked each fix by reverting it: the recovery, stopping the failed replacement, and the listener skip. Each revert fails exactly the test written for it. ## Verification - `:grails-geb:test`: 15/15 pass - `:grails-test-examples-geb:integrationTest` against Docker: all specs pass, including `PerTestRecordingSpec` 3/3 - `:grails-geb:codeStyle`: passes ## Backport This will be backported to 7.0.x once merged, together with #16031 and the per-run recording directory scoping from c179aacdc0, which 7.0.x also lacks. A trial cherry-pick applies cleanly and `:grails-geb:test` passes there. With this PR merged first, the backported files match 8.0.x, so the merge-up should not conflict. -- 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]
