borinquenkid commented on code in PR #16031:
URL: https://github.com/apache/grails-core/pull/16031#discussion_r3694227419
##########
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<File> recordingFiles = []
+ Map<String, Long> previousSizes = [:]
+ List<File> 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<File> candidateFiles =
currentRunRecordingDirs(baseRecordingDir).collectMany { File dir ->
(dir.listFiles({ File file ->
isVideoFile(file) && file.name.contains(testClassName)
} as FileFilter) ?: new File[0]) as List<File>
}
+ Map<String, Long> 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
+ }
Review Comment:
You're right, and thanks for walking through the source — confirmed the same
thing against Testcontainers 2.0.5: `saveRecordingToFile`'s `Files.copy` has
already returned by the time this feature method runs, so there's no concurrent
writer for a directory scan to race against.
Reverted the stability-polling entirely in 24a0b59c and reworked the
diagnosis around your more likely explanation (a VNC container that's connected
but hasn't captured meaningful frames yet after a restart). That also surfaced
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 pointing at a
container that never actually started. Fixed alongside.
I want to flag honestly: I didn't manage to pull the original CI/Develocity
failure output to confirm which assertion actually broke (it's a <1% flake with
no captured output in #16030). 24a0b59c hedges across both of your candidate
diagnoses rather than landing a fix confirmed against the real failure — happy
to hold this for a repro if you'd rather chase the actual signal first.
##########
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<File> recordingFiles = []
+ Map<String, Long> previousSizes = [:]
Review Comment:
Good catch, and moot now — 24a0b59c reverts the stability-polling entirely
(`previousSizes` and all), so `waitForRecordingFiles` is back to its original
form and there's no first-poll penalty to worry about. Thanks for spotting it
regardless.
##########
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<File> recordingFiles = []
+ Map<String, Long> previousSizes = [:]
+ List<File> 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<File> candidateFiles =
currentRunRecordingDirs(baseRecordingDir).collectMany { File dir ->
(dir.listFiles({ File file ->
isVideoFile(file) && file.name.contains(testClassName)
} as FileFilter) ?: new File[0]) as List<File>
}
+ Map<String, Long> 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
Review Comment:
Also moot after 24a0b59c reverted the stability-polling —
`waitForRecordingFiles` returns the raw scan result again, so a timeout still
reports whatever files were actually found, same as before this PR. Good
instinct on wanting failure diagnostics preserved, though; I'll keep it in mind
if we ever add readiness logic here again.
--
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]
