Merge pull request #130 from Homebrew/fix-flakey-pty-test

Stop the pty tests racing the child they spawn

Author: graeme

Tests/BrewCLITests/AsyncTestExpectations.swift

@@ -0,0 +1,50 @@
+//
+//  AsyncTestExpectations.swift
+//  BrewTests
+//
+
+import Foundation
+import Testing
+
+func waitUntil(
+    _ comment: Comment? = nil,
+    timeout: Duration = .seconds(5),
+    poll: Duration = .milliseconds(10),
+    sourceLocation: SourceLocation = #_sourceLocation,
+    _ condition: @Sendable () async -> Bool,
+) async throws {
+    let deadline = ContinuousClock.now + timeout
+    while ContinuousClock.now < deadline {
+        if await condition() {
+            return
+        }
+        try await Task.sleep(for: poll)
+    }
+    guard await condition() else {
+        Issue.record(comment ?? "Timed out after \(timeout)", sourceLocation: sourceLocation)
+        return
+    }
+}
+
+actor TestGate {
+    private var isOpen = false
+    private var waiters: [CheckedContinuation<Void, Never>] = []
+
+    func wait() async {
+        guard !isOpen else {
+            return
+        }
+        await withCheckedContinuation { continuation in
+            waiters.append(continuation)
+        }
+    }
+
+    func open() {
+        isOpen = true
+        let pending = waiters
+        waiters = []
+        for continuation in pending {
+            continuation.resume()
+        }
+    }
+}

Tests/BrewCLITests/BrewCommandServicePseudoTerminalTests.swift

@@ -38,7 +38,7 @@ struct BrewCommandServicePseudoTerminalTests {
             lineObserver: { collector.append($0) },
         )
 
-        #expect(collector.allLines().map(\.text) == ["one", "two", "three"])
+        #expect(collector.allLines().filter(\.isComplete).map(\.text) == ["one", "two", "three"])
     }
 
     @Test func `pseudo-terminal run merges stderr into the stdout stream`() async throws {
@@ -123,12 +123,11 @@ struct BrewCommandServicePseudoTerminalTests {
         let collector = OutputCollector()
 
         _ = try await run(
-            script: "printf '10%%\\r'; sleep 0.1; printf '50%%\\r'; sleep 0.1; printf '100%%\\n'",
+            script: "printf '10%%\\r'; sleep 0.3; printf '50%%\\r'; sleep 0.3; printf '100%%\\n'",
             channel: .pseudoTerminal,
             lineObserver: { collector.append($0) },
         )
 
-        // Separated by sleeps so they arrive as distinct reads.
         let revisions = collector.allLines().filter { !$0.isComplete }.map(\.text)
         #expect(revisions == ["10%", "50%"])
     }
@@ -171,13 +170,15 @@ struct BrewCommandServicePseudoTerminalTests {
             )
         }
 
-        try await Task.sleep(for: .milliseconds(500))
-        let spawned = Self.grandchildCount()
+        try await waitUntil("the child never spawned a grandchild", poll: .milliseconds(50)) {
+            Self.grandchildCount() > 0
+        }
         task.cancel()
         _ = await task.result
-        try await Task.sleep(for: .milliseconds(1500))
 
-        #expect(spawned > 0 && Self.grandchildCount() == 0)
+        try await waitUntil("the grandchild outlived the cancelled run", poll: .milliseconds(50)) {
+            Self.grandchildCount() == 0
+        }
     }
 }
 

Tests/BrewCLITests/PseudoTerminalTests.swift

@@ -49,13 +49,10 @@ struct PseudoTerminalTests {
     @Test func `read reports end of input once the child and the local replica are gone`() throws {
         let terminal = try PseudoTerminal()
         let process = try startProcess(script: "printf 'done\\n'", terminal: terminal)
-        terminal.closeReplica()
-
-        let output = drainToEndOfInput(terminal)
-        process.waitUntilExit()
-        terminal.closePrimary()
 
         // Terminating at all is the assertion: a leaked replica descriptor would hang this forever.
+        let output = collectOutput(of: process, on: terminal)
+
         #expect(output == "done\n")
     }
 
@@ -96,40 +93,66 @@ struct PseudoTerminalTests {
 }
 
 private extension PseudoTerminalTests {
-    /// Runs `script` with the replica as stdout+stderr and drains the primary to end-of-input.
     func runOnPseudoTerminal(
         columns: UInt16 = PseudoTerminal.defaultColumns,
         rows: UInt16 = PseudoTerminal.defaultRows,
         script: String,
     ) throws -> String {
         let terminal = try PseudoTerminal(columns: columns, rows: rows)
         let process = try startProcess(script: script, terminal: terminal)
-        terminal.closeReplica()
+        let output = collectOutput(of: process, on: terminal)
 
-        let output = drainToEndOfInput(terminal)
-        process.waitUntilExit()
-        terminal.closePrimary()
+        #expect(process.terminationStatus == 0, "the child exited \(process.terminationStatus)")
 
         return output
     }
 
-    /// Ignores idle timeouts; safe because every script under test terminates on its own.
-    func drainToEndOfInput(_ terminal: PseudoTerminal) -> String {
+    func collectOutput(of process: Process, on terminal: PseudoTerminal) -> String {
         var data = Data()
+
+        live: while true {
+            switch terminal.read(timeout: .milliseconds(25)) {
+            case let .data(chunk):
+                data.append(chunk)
+            case .timedOut:
+                if !process.isRunning {
+                    break live
+                }
+            case .endOfInput:
+                Issue.record("end of input arrived while this process still held the replica open")
+                break live
+            case let .failed(code):
+                Issue.record("reading the terminal failed: \(PseudoTerminal.describe(errno: code))")
+                break live
+            }
+        }
+        process.waitUntilExit()
+
+        terminal.closeReplica()
+        drainToEndOfInput(terminal, into: &data)
+        terminal.closePrimary()
+
+        return UTF8StreamDecoder.lossyString(data)
+    }
+
+    func drainToEndOfInput(_ terminal: PseudoTerminal, into data: inout Data) {
+        let deadline = Date().addingTimeInterval(10)
         loop: while true {
             switch terminal.read() {
             case let .data(chunk):
                 data.append(chunk)
             case .timedOut:
-                continue
+                if Date() > deadline {
+                    Issue.record("end of input never arrived: a replica descriptor is still open somewhere")
+                    break loop
+                }
             case .endOfInput:
                 break loop
             case let .failed(code):
                 Issue.record("reading the terminal failed: \(PseudoTerminal.describe(errno: code))")
                 break loop
             }
         }
-        return UTF8StreamDecoder.lossyString(data)
     }
 
     /// `Foundation.Process` deliberately, to exercise ``PseudoTerminal`` independently of the runner.