borinquenkid commented on PR #16031: URL: https://github.com/apache/grails-core/pull/16031#issuecomment-5148814825
@jdaugherty thanks for the deep read on this one — replied inline on each of the three specific comments, and wanted to close the loop on the top-level review too. **On the diagnosis:** you were right that the original stability-polling premise doesn't hold. Verified the same thing you did against the Testcontainers 2.0.5 source (`saveRecordingToFile`'s `Files.copy` has returned before this feature method ever runs), and reverted that change entirely in 24a0b59c. **On confirming which assertion actually failed:** I wasn't able to pull the original CI/Develocity output — it's a <1% flake and #16030 has no captured failure info attached, so I don't have a build scan to chase. Rather than sit on this without that signal, 24a0b59c hedges across both of the candidate diagnoses you laid out instead of landing a fix confirmed against the real failure: 1. Fixed a real bug in `WebDriverContainerHolder#restartVncRecordingContainer` — the `vncRecordingContainer` field was updated to the new container *before* `start()` succeeded, so a swallowed `start()` failure left it silently pointing at a container that never started. This doesn't cover the "connected but not yet capturing frames" timing gap you described — I don't have a way to force that deterministically without a real repro. 2. Replaced the byte-difference-only check with a minimum-size floor per recording. Kept the byte-mismatch check on top rather than dropping it — it also catches two recordings accidentally resolving to the *same* file, which the size floor alone wouldn't. The floor isn't a guess anymore either: 08800e4 grounds `MIN_MEANINGFUL_RECORDING_BYTES` in two real local runs against the container, where every genuine recording measured 75KB-855KB (5,000 bytes stays more than an order of magnitude below the smallest of those). 3. Added `WebDriverContainerHolderSpec` (08800e4) covering `stop()`'s try/finally reset and `restartVncRecordingContainer()`'s guard clauses / exception-swallowing — no Docker required, since neither had unit coverage before. I'd still take a real repro over this if one turns up — happy to hold or follow up further if you'd rather chase the actual signal first. **On 7.0.x:** agreed the two copies of `waitForRecordingFiles` shouldn't keep diverging. Proposing we land this on 8.0.x first, and I'll follow up with a backport PR to 7.0.x that also picks up the per-run directory scoping from c179aacdc0, since 7.0.x doesn't have that either. Let me know if you'd rather sequence it the other way. -- 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]
