jdaugherty commented on code in PR #16031:
URL: https://github.com/apache/grails-core/pull/16031#discussion_r3652244018
##########
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:
`previousSizes` starts empty, so on the first poll `previousSizes[path] ==
size` is `null == size` for every candidate and `readyFiles` is always empty —
no matter how long the recordings have been sitting complete on disk.
That means every run of this spec now sleeps at least one full
`pollIntervalMillis` (500ms) before it can succeed, where previously the first
poll could break immediately. Seeding `previousSizes` from an initial scan
before entering the loop would avoid the unconditional penalty if this check is
kept.
##########
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:
The premise in this comment doesn't hold, so this check is guarding against
something that can't happen.
`GebRecordingTestListener.afterIteration` calls `container.afterTest(...)`
synchronously on the test thread. That reaches
`BrowserWebDriverContainer.retainRecordingIfNeeded`, which calls
`vncRecordingContainer.saveRecordingToFile(recordingFile)`:
```java
try (InputStream inputStream = streamRecording()) {
Files.copy(inputStream, file.toPath(),
StandardCopyOption.REPLACE_EXISTING);
}
```
and `streamRecording()` blocks on `execInContainer("ffmpeg", ...)` before
the copy starts. Because the spec is `@Stepwise`, features 1 and 2 have both
returned from `afterIteration` — and therefore from `Files.copy` — before this
feature method runs. `Files.copy` lacking a temp-file+rename only matters when
a reader can race a live writer; there is no live writer here.
Separately, even on its own terms "size unchanged since the previous poll"
isn't proof a copy finished — a docker tar stream that stalls for more than the
500ms poll interval under CI I/O contention gives two equal readings mid-copy.
If a readiness check is genuinely needed, it would want N consecutive stable
polls rather than one.
##########
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:
On timeout this returns `readyFiles`, and `candidateFiles` is discarded.
That makes a real failure harder to diagnose than before: previously the
returned list held whatever was on disk, so the `recordingFiles.size() >=
minFileCount` failure message showed the files that were found. Now a timeout
typically reports an empty list, with no signal distinguishing "no recording
was ever written" from "two recordings existed but never satisfied the
stability check" — and those point at different bugs.
If the check stays, please surface the candidates on timeout (return them,
or at minimum log their names and sizes) so the next flaky run leaves behind
something actionable.
--
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]
