Skip to content

[EVI-11]: fix capture-pr video artifacts #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
riddim-developer-bot merged 1 commit into from
May 19, 2026
Merged

[EVI-11]: fix capture-pr video artifacts #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions Sources/EvidenceCLIKit/CommandRunner.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,50 @@ public struct ProcessCommandRunner: CommandRunning {
process.standardOutput = stdout
process.standardError = stderr

var stdoutData = Data()
var stderrData = Data()
let stdoutLock = NSLock()
let stderrLock = NSLock()

stdout.fileHandleForReading.readabilityHandler = { handle in
let data = handle.availableData
guard !data.isEmpty else { return }
stdoutLock.lock()
stdoutData.append(data)
stdoutLock.unlock()
}
stderr.fileHandleForReading.readabilityHandler = { handle in
let data = handle.availableData
guard !data.isEmpty else { return }
stderrLock.lock()
stderrData.append(data)
stderrLock.unlock()
}
defer {
stdout.fileHandleForReading.readabilityHandler = nil
stderr.fileHandleForReading.readabilityHandler = nil
}

try process.run()
process.waitUntilExit()

stdout.fileHandleForReading.readabilityHandler = nil
stderr.fileHandleForReading.readabilityHandler = nil
let remainingStdout = stdout.fileHandleForReading.readDataToEndOfFile()
let remainingStderr = stderr.fileHandleForReading.readDataToEndOfFile()
stdoutLock.lock()
stdoutData.append(remainingStdout)
let finalStdout = stdoutData
stdoutLock.unlock()
stderrLock.lock()
stderrData.append(remainingStderr)
let finalStderr = stderrData
stderrLock.unlock()

return CommandResult(
exitCode: process.terminationStatus,
stdout: String(data: stdout.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "",
stderr: String(data: stderr.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? ""
stdout: String(data: finalStdout, encoding: .utf8) ?? "",
stderr: String(data: finalStderr, encoding: .utf8) ?? ""
)
}
}
65 changes: 51 additions & 14 deletions Sources/EvidenceCLIKit/EvidencePlanExecution.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,17 +236,38 @@ public struct XcodeTestPlanExecutor: EvidencePlanExecuting {
guard let session, let videoPlan else { return nil }
try videoRecorder.stop(session)
let timestamp = timestamp()
result.artifacts.append(artifactWriter.artifact(
let artifact = try videoArtifact(
kind: .video,
phase: phase,
path: session.outputPath,
stepName: videoPlan.artifactStepName,
mediaType: mediaType(for: URL(fileURLWithPath: session.outputPath)),
capturedAt: timestamp
))
)
result.artifacts.append(artifact)
return session.outputPath
}

private func videoArtifact(
kind: CapturedArtifact.Kind,
phase: PRChangeEvidencePhase,
path: String,
stepName: String,
mediaType: String,
capturedAt: String
) throws -> CapturedArtifact {
let artifact = artifactWriter.artifact(
kind: kind,
phase: phase,
path: path,
stepName: stepName,
mediaType: mediaType,
capturedAt: capturedAt
)
try validateVideoArtifact(artifact, phase: phase)
return artifact
}

private func artifactPath(
for step: PRChangeEvidenceStep,
defaultPath: String?,
Expand Down Expand Up @@ -373,7 +394,7 @@ public final class SimctlVideoRecorder: VideoRecording {
public func stop(_ session: VideoRecordingSession) throws {
guard let process = session.process else { return }
if process.isRunning {
process.terminate()
process.interrupt()
}
process.waitUntilExit()
}
Expand Down Expand Up @@ -440,25 +461,27 @@ public struct SimctlPlanExecutor: EvidencePlanExecuting {
didLaunch = true
}

func finishActiveVideo(recordingStepName: String? = nil, capturedAt: String? = nil) {
func finishActiveVideo(recordingStepName: String? = nil, capturedAt: String? = nil) throws {
if let session = activeVideo {
try? videoRecorder.stop(session)
try videoRecorder.stop(session)
activeVideo = nil
if let recordingStepName, let capturedAt {
result.artifacts.append(artifactWriter.artifact(
let artifact = artifactWriter.artifact(
kind: .video,
phase: build.phase,
path: session.outputPath,
stepName: recordingStepName,
mediaType: mediaType(for: URL(fileURLWithPath: session.outputPath)),
capturedAt: capturedAt
))
)
try validateVideoArtifact(artifact, phase: build.phase)
result.artifacts.append(artifact)
}
}
}

defer {
finishActiveVideo()
try? finishActiveVideo()
try? simulator.terminate(bundleID: bundleID, selection: selection)
}

Expand Down Expand Up @@ -487,14 +510,16 @@ public struct SimctlPlanExecutor: EvidencePlanExecuting {
if shouldRecordWholeFlow, step == steps.last, let session = activeVideo {
try videoRecorder.stop(session)
activeVideo = nil
result.artifacts.append(artifactWriter.artifact(
let artifact = artifactWriter.artifact(
kind: .video,
phase: build.phase,
path: session.outputPath,
stepName: request.plan.video.name?.nonEmpty ?? "flow",
mediaType: mediaType(for: URL(fileURLWithPath: session.outputPath)),
capturedAt: completedAt
))
)
try validateVideoArtifact(artifact, phase: build.phase)
result.artifacts.append(artifact)
}
result.stepResults.append(CaptureStepResult(
phase: build.phase,
Expand All @@ -515,18 +540,20 @@ public struct SimctlPlanExecutor: EvidencePlanExecuting {
capturedAt: completedAt
))
} else if step.kind == .stopVideo, let artifactPath {
result.artifacts.append(artifactWriter.artifact(
let artifact = artifactWriter.artifact(
kind: .video,
phase: build.phase,
path: artifactPath,
stepName: step.name,
mediaType: mediaType(for: URL(fileURLWithPath: artifactPath)),
capturedAt: completedAt
))
)
try validateVideoArtifact(artifact, phase: build.phase)
result.artifacts.append(artifact)
}
} catch let error as CLIError {
let completedAt = timestamp()
finishActiveVideo(recordingStepName: step.name, capturedAt: completedAt)
try? finishActiveVideo(recordingStepName: step.name, capturedAt: completedAt)
let message = "\(build.phase.rawValue) step '\(step.name)' failed. \(error.errorDescription ?? String(describing: error))"
result.failures.append(PRChangeEvidenceFailureSummary(stepName: step.name, message: message))
result.stepResults.append(CaptureStepResult(
Expand All @@ -541,7 +568,7 @@ public struct SimctlPlanExecutor: EvidencePlanExecuting {
return result
} catch {
let completedAt = timestamp()
finishActiveVideo(recordingStepName: step.name, capturedAt: completedAt)
try? finishActiveVideo(recordingStepName: step.name, capturedAt: completedAt)
let message = "\(build.phase.rawValue) step '\(step.name)' failed. \(String(describing: error))"
result.failures.append(PRChangeEvidenceFailureSummary(stepName: step.name, message: message))
result.stepResults.append(CaptureStepResult(
Expand Down Expand Up @@ -769,6 +796,16 @@ private func mediaType(for url: URL) -> String {
}
}

private func validateVideoArtifact(
_ artifact: CapturedArtifact,
phase: PRChangeEvidencePhase
) throws {
guard artifact.kind == .video else { return }
guard let fileSize = artifact.fileSize, fileSize > 0 else {
throw CLIError.commandFailed("\(phase.rawValue) video artifact missing or empty: \(artifact.path)")
}
}

private func fileSafeName(for name: String) -> String {
let allowed = CharacterSet.alphanumerics.union(CharacterSet(charactersIn: "-_"))
let scalars = name.unicodeScalars.map { scalar in
Expand Down
14 changes: 12 additions & 2 deletions Sources/EvidenceCLIKit/PullRequestEvidenceReport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ public struct RenderPullRequestEvidenceReport: PullRequestEvidenceReporting {
outputDirectory: URL
) -> [ReportFailureSection] {
var sections: [ReportFailureSection] = []
let mediaSteps = plan.steps.filter { $0.kind == .screenshot || $0.kind == .stopVideo }
let mediaSteps = expectedMediaSteps(in: plan)

let missingBefore = missingArtifacts(
phase: .before,
Expand Down Expand Up @@ -740,7 +740,7 @@ public struct RenderPullRequestEvidenceReport: PullRequestEvidenceReporting {
plan: PRChangeEvidencePlan,
manifest: PRChangeEvidenceManifest
) -> Bool {
for step in plan.steps where step.kind == .screenshot || step.kind == .stopVideo {
for step in expectedMediaSteps(in: plan) {
let kind: CapturedArtifact.Kind = step.kind == .screenshot ? .screenshot : .video
if artifact(kind: kind, phase: .before, stepName: step.name, manifest: manifest) == nil
|| artifact(kind: kind, phase: .after, stepName: step.name, manifest: manifest) == nil {
Expand All @@ -750,6 +750,16 @@ public struct RenderPullRequestEvidenceReport: PullRequestEvidenceReporting {
return false
}

private func expectedMediaSteps(in plan: PRChangeEvidencePlan) -> [PRChangeEvidenceStep] {
var mediaSteps = plan.steps.filter { $0.kind == .screenshot || $0.kind == .stopVideo }
if plan.video.enabled,
!plan.steps.contains(where: { $0.kind == .startVideo || $0.kind == .stopVideo }) {
let name = plan.video.name?.nonEmpty ?? "flow"
mediaSteps.append(PRChangeEvidenceStep(name: name, kind: .stopVideo, path: "\(name).mov"))
}
return mediaSteps
}

private func shortSHA(_ sha: String) -> String {
String(sha.prefix(7))
}
Expand Down
20 changes: 20 additions & 0 deletions Tests/EvidenceCLIKitTests/EvidenceCLIKitTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,26 @@ import Foundation
import XCTest

final class EvidenceCLIKitTests: XCTestCase {
func testProcessCommandRunnerDrainsLargeStdoutAndStderrWhileProcessRuns() throws {
try XCTSkipUnless(
FileManager.default.isExecutableFile(atPath: "/usr/bin/python3"),
"python3 is not available"
)
let script = """
import sys
sys.stdout.write("out" * 100000)
sys.stdout.flush()
sys.stderr.write("err" * 100000)
sys.stderr.flush()
"""

let result = try ProcessCommandRunner().run("/usr/bin/python3", ["-c", script])

XCTAssertEqual(result.exitCode, 0)
XCTAssertEqual(result.stdout.count, 300_000)
XCTAssertEqual(result.stderr.count, 300_000)
}

func testConfigParsingRequiresNamedFields() throws {
let document = try TOMLDocument.parse("""
scheme = "Example"
Expand Down
64 changes: 64 additions & 0 deletions Tests/EvidenceCLIKitTests/IOSRevisionAdaptersTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,58 @@ final class IOSRevisionAdaptersTests: XCTestCase {
])
}

func testSimctlPlanExecutorFailsWhenDeclaredWholeFlowVideoIsMissing() throws {
let directory = try temporaryDirectory()
let output = directory.appendingPathComponent("proof/pr-55", isDirectory: true)
let plan = PRChangeEvidencePlan(
repo: "RiddimSoftware/example",
pr: 55,
platform: .ios,
runner: .simctl,
ios: PRChangeEvidenceIOSSettings(
scheme: "Example",
bundleID: "com.example.app",
simulatorUDID: "SIM-123"
),
steps: [
PRChangeEvidenceStep(name: "launch app", kind: .launch),
PRChangeEvidenceStep(name: "settle", kind: .wait, seconds: 0),
PRChangeEvidenceStep(name: "capture home", kind: .screenshot, path: "home.png")
],
video: PRChangeEvidenceVideo(enabled: true, name: "home-flow")
)
let simulator = FakeSimulatorController()
let videoRecorder = FakeVideoRecorder(missingOutputPhases: [.after])
let executor = SimctlPlanExecutor(
simulator: simulator,
videoRecorder: videoRecorder,
artifactWriter: FileArtifactWriter(),
clock: IOSFixedEvidenceClock(date: Date(timeIntervalSince1970: 1_714_000_000))
)

let result = try executor.execute(EvidencePlanExecutionRequest(
plan: plan,
planURL: directory.appendingPathComponent(".evidence/pr-home.json"),
outputDirectory: output,
worktrees: [
ComparisonWorktree(label: .before, sha: "before", path: directory.appendingPathComponent("before-worktree").path),
ComparisonWorktree(label: .after, sha: "after", path: directory.appendingPathComponent("after-worktree").path)
],
revisionBuilds: [
revisionBuild(.before, output: output),
revisionBuild(.after, output: output)
],
ios: plan.ios!,
launch: plan.launch
))

XCTAssertFalse(result.succeeded)
XCTAssertEqual(result.artifacts.filter { $0.kind == .video }.map(\.phase), [.before])
XCTAssertTrue(result.failures.contains { $0.message.contains("after") && $0.message.contains("home-flow.mov") })
XCTAssertEqual(result.stepResults.last?.phase, .after)
XCTAssertEqual(result.stepResults.last?.status, .failed)
}

func testCapturePRBuildsBothRevisionsWithIsolatedDerivedDataAndManifestRecords() throws {
let directory = try temporaryDirectory()
let output = directory.appendingPathComponent("proof/pr-44", isDirectory: true)
Expand Down Expand Up @@ -860,6 +912,11 @@ private final class FakeSimulatorController: SimulatorControlling {
private final class FakeVideoRecorder: VideoRecording {
private(set) var startedPaths: [String] = []
private(set) var stoppedPaths: [String] = []
var missingOutputPhases: Set<PRChangeEvidencePhase>

init(missingOutputPhases: Set<PRChangeEvidencePhase> = []) {
self.missingOutputPhases = missingOutputPhases
}

func start(udid: String, outputURL: URL) throws -> VideoRecordingSession {
startedPaths.append(outputURL.path)
Expand All @@ -868,8 +925,15 @@ private final class FakeVideoRecorder: VideoRecording {

func stop(_ session: VideoRecordingSession) throws {
stoppedPaths.append(session.outputPath)
if missingOutputPhases.contains(phase(forArtifactPath: session.outputPath)) {
return
}
let url = URL(fileURLWithPath: session.outputPath)
try FileManager.default.createDirectory(at: url.deletingLastPathComponent(), withIntermediateDirectories: true)
try Data("mov".utf8).write(to: url)
}

private func phase(forArtifactPath path: String) -> PRChangeEvidencePhase {
path.contains("/after/") ? .after : .before
}
}
24 changes: 24 additions & 0 deletions Tests/EvidenceCLIKitTests/PullRequestEvidenceReportTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,30 @@ final class PullRequestEvidenceReportTests: XCTestCase {
XCTAssertTrue(report.markdown.contains("- Overall status: **failed**"))
}

func testRendererNamesMissingWholeFlowVideoWhenPlanVideoIsEnabled() throws {
let output = try outputDirectory()
var manifest = successfulManifest(output: output)
manifest.artifacts = manifest.artifacts.filter { artifact in
artifact.kind != .video || artifact.phase == .before
}

var plan = comparisonPlan()
plan.video = PRChangeEvidenceVideo(enabled: true, name: "home-flow")
plan.steps = [
PRChangeEvidenceStep(name: "Launch app", kind: .launch),
PRChangeEvidenceStep(name: "Home screen", kind: .screenshot, path: "home.png")
]

let report = try RenderPullRequestEvidenceReport(
comparisonRenderer: RecordingComparisonImageRenderer(),
fileManager: .default
).writeReport(manifest: manifest, plan: plan, outputDirectory: output)

XCTAssertTrue(report.markdown.contains("### Missing After Artifact"))
XCTAssertTrue(report.markdown.contains("home-flow: expected `after/home-flow.mov`"))
XCTAssertTrue(report.markdown.contains("- Overall status: **partial**"))
}

func testRendererNamesBuildFailureAndReportOnlyPartialOutputWithoutRawLogs() throws {
let output = try outputDirectory()
var manifest = successfulManifest(output: output)
Expand Down
8 changes: 5 additions & 3 deletions docs/architecture/use-case-catalog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,11 @@ screenshot/openURL adapter, simctl video recorder, filesystem artifact writer.
Current implementation: `Sources/EvidenceCLIKit/EvidencePlanExecution.swift`
dispatches `runner = "xctest"` to revision-scoped `xcodebuild test` calls with
Evidence environment values, and `runner = "simctl"` to launch/wait/screenshot
and video steps. `CapturePullRequestEvidence` records returned artifacts and
step results in `manifest.json`; `EvidenceCLI.swift` remains a thin command
router.
and video steps. Video artifacts are validated for presence and non-zero file
size before they are added to the manifest; missing or empty recordings are
returned as capture failures. `CapturePullRequestEvidence` records returned
artifacts and step results in `manifest.json`; `EvidenceCLI.swift` remains a
thin command router.

### RenderPullRequestEvidenceReport

Expand Down
Loading