a73x

b22b1de4

Drain the pty, and wait on the screen instead of the clock

a73x   2026-08-13 07:46

Commit message
Drain the pty, and wait on the screen instead of the clock

`tests/tui_review_test.rs` failed intermittently, always on the two
`$EDITOR` tests, always by hanging to the harness deadline rather than by
failing an assertion. It was neither a race against a slow machine nor
anything in the TUI.

**The cause.** `run_dashboard_driven` collected the child's output with
`wait_with_output()`, which reads nothing until the child exits. `script`
copies the pty master to its stdout in a single loop, so when that write
blocks it also stops reading its own stdin — keystrokes stop being
forwarded, the `q` never arrives, and the dashboard sits in `event::poll`
until the deadline kills it. Caught in the act, `script` is in state
`anon_pipe_write`.

What made it intermittent is that a pipe's capacity is not a constant.
Linux gives a new pipe 64 KiB only while the user is under
`fs/pipe-user-pages-soft`; over it, new pipes get one page's worth — 8 KiB.
A machine running a test suite crosses that line continually. Sampling a
fresh pipe every 200ms through one run of this file gave 8 KiB on 71 of
200 samples.

8 KiB is exactly where these tests sit. The dashboard has written ~4.9 KiB
by the time the patch detail pane is up; suspending to `$EDITOR` leaves the
alternate screen and returns to a full repaint, taking it to ~9.3 KiB. Only
the editor tests cross the line, which is why only they failed. The limit
is per-*user*, so `--test-threads=1` does not avoid it and another process
of the same user can push a test over — which is what "order-dependent"
really was.

This also explains the two things that did not fit a timing story: a wider
sleep budget cannot help, because the bytes are already written before the
sleep begins; and an unrelated change to how the pane draws can appear to
fix it, by moving the byte count back under the line until the next change
moves it over again.

Proved by A/B on unmodified `main` with pipe capacity as the only variable:
held over the soft limit, 3/3 runs failed on exactly those two tests; the
fix passed 12/12 under the same pressure.

**The fix.** Read stdout and stderr on background threads for as long as
the child lives, so the pty is never the thing that blocks. That makes the
live screen readable while the program runs, so the tests can wait on what
they actually mean:

- the first frame, rather than 600ms, before typing anything — raw mode is
  enabled before the first draw, so a frame is proof that keys reach the
  program and not the terminal's line editor;
- the frame that shows the last key landed, rather than a few hundred ms;
- the dashboard's own message that the editor round trip finished.

Matching for a wait ignores whitespace, because ratatui writes only the
cells that changed and a blank that was already blank never reaches the
stream. Assertions keep their exact text: no assertion is weakened, and a
wait that gives up only makes the assertion that follows fail sooner, with
the screen attached. Verified by breaking the abort message and watching
the test fail on it.

One wall-clock wait remains, in
`the_dashboards_own_write_does_not_raise_the_banner`. Its claim is an
absence, and an absence has nothing to wait for; it now sleeps one tip-poll
period of the program's own rather than a guess about the machine, and says
so.

The file went from 33.8s to 2.8s, and from flaky to 12/12 consecutive green
under the conditions that used to make it fail every time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tests/common/mod.rs
Old New
@@ -6,7 +6,7 @@ use std::io::{Read, Write};
6 use std::net::{SocketAddr, TcpListener, TcpStream}; 6 use std::net::{SocketAddr, TcpListener, TcpStream};
7 use std::path::{Path, PathBuf}; 7 use std::path::{Path, PathBuf};
8 use std::process::{Child, Command, Output, Stdio}; 8 use std::process::{Child, Command, Output, Stdio};
9 use std::sync::{Mutex, MutexGuard, OnceLock}; 9 use std::sync::{Arc, Mutex, MutexGuard, OnceLock};
10 use std::thread; 10 use std::thread;
11 use std::time::{Duration, Instant}; 11 use std::time::{Duration, Instant};
12 12
@@ -66,6 +66,191 @@ pub fn screen_text(bytes: &[u8]) -> String {
66 } 66 }
67 67
68 // =========================================================================== 68 // ===========================================================================
69 // Driving a program that owns a terminal
70 // ===========================================================================
71
72 /// The backstop for every wait below. Nothing is *supposed* to take this long:
73 /// each wait is on something observable and returns the moment it is true, so
74 /// reaching this number means the thing being waited for never happened, and
75 /// the assertion that follows will say which.
76 const PTY_TIMEOUT: Duration = Duration::from_secs(20);
77
78 /// How often an observable condition is re-checked.
79 const PTY_POLL: Duration = Duration::from_millis(20);
80
81 /// Poll `cond` until it holds. Returns whether it held before `timeout`.
82 ///
83 /// The shape every wait in this file has: ask the question again until the
84 /// answer is yes. A `sleep` long enough to cover the same case on this machine
85 /// is not the same thing — it is slower when the answer comes early and wrong
86 /// when it comes late.
87 pub fn wait_until(timeout: Duration, mut cond: impl FnMut() -> bool) -> bool {
88 let deadline = Instant::now() + timeout;
89 loop {
90 if cond() {
91 return true;
92 }
93 if Instant::now() >= deadline {
94 return false;
95 }
96 thread::sleep(PTY_POLL);
97 }
98 }
99
100 /// A child's output stream, read on a background thread for as long as the
101 /// child lives.
102 ///
103 /// This is not a convenience, and reading the output afterwards instead — the
104 /// obvious `wait_with_output` — deadlocks.
105 ///
106 /// `script` copies the pty master to its stdout, and it is a single loop: when
107 /// that write blocks, it also stops reading its own stdin, so keystrokes stop
108 /// being forwarded and the program under test never sees the `q` that would
109 /// end the test. It does not crash and it does not fail an assertion; it sits
110 /// there until the harness deadline kills it.
111 ///
112 /// What makes that fire intermittently is that a pipe's capacity is not a
113 /// constant. Linux gives a new pipe 64 KiB *until the user is over
114 /// `fs/pipe-user-pages-soft`* (16384 pages here), after which new pipes get one
115 /// page's worth — 8 KiB. A machine running a test suite crosses that line
116 /// constantly: sampling a fresh pipe every 200ms during one run of this file
117 /// gave 8 KiB on 71 of 200 samples. So each `script` gets 64 KiB or 8 KiB
118 /// depending on nothing the test can see, and it is a per-*user* limit, so
119 /// `--test-threads=1` does not avoid it and another process of the same user
120 /// can push a test over.
121 ///
122 /// 8 KiB is exactly where the editor tests sit. The dashboard has written
123 /// ~4.9 KiB by the time the patch detail pane is up, which is under the line;
124 /// suspending to `$EDITOR` leaves the alternate screen and comes back to a
125 /// full repaint, which takes it to ~9.3 KiB — over. That, and not any race
126 /// against a slow machine, is why those two tests and no others, why a wider
127 /// sleep budget does not help (the bytes are already written before the sleep
128 /// starts), and why a change to how the pane draws can appear to fix it.
129 struct Drain {
130 buf: Arc<Mutex<Vec<u8>>>,
131 join: thread::JoinHandle<()>,
132 }
133
134 impl Drain {
135 fn start<R: Read + Send + 'static>(mut src: R) -> Drain {
136 let buf: Arc<Mutex<Vec<u8>>> = Arc::new(Mutex::new(Vec::new()));
137 let sink = Arc::clone(&buf);
138 let join = thread::spawn(move || {
139 let mut chunk = [0u8; 8192];
140 loop {
141 match src.read(&mut chunk) {
142 Ok(0) | Err(_) => break,
143 Ok(n) => sink.lock().unwrap().extend_from_slice(&chunk[..n]),
144 }
145 }
146 });
147 Drain { buf, join }
148 }
149
150 /// A handle on the bytes so far, readable while the child still runs.
151 fn live(&self) -> Arc<Mutex<Vec<u8>>> {
152 Arc::clone(&self.buf)
153 }
154
155 /// Everything the child wrote, once its stream has closed.
156 fn finish(self) -> Vec<u8> {
157 let _ = self.join.join();
158 let bytes = self.buf.lock().unwrap().clone();
159 bytes
160 }
161 }
162
163 /// Compare screens ignoring whitespace.
164 ///
165 /// ratatui repaints only the cells that changed, so a blank between two words
166 /// that was already blank may never be written at all: `"Patch Detail"` can
167 /// legitimately arrive as `"PatchDetail"`. Waits are matched this way so they
168 /// cannot hang on a space the terminal was entitled to skip. Assertions keep
169 /// their exact text — a loose *wait* only ever waits less, never asserts less.
170 fn squeezed(text: &str) -> String {
171 text.chars().filter(|c| !c.is_whitespace()).collect()
172 }
173
174 /// A dashboard running in a pty: keys go in, and the screen can be read while
175 /// it is still running.
176 ///
177 /// Implements `Write`, so `stdin.write_all(b"q")` still means what it did when
178 /// this was a bare `ChildStdin`.
179 pub struct Dashboard {
180 stdin: std::process::ChildStdin,
181 screen: Arc<Mutex<Vec<u8>>>,
182 }
183
184 impl Write for Dashboard {
185 fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
186 self.stdin.write(buf)
187 }
188 fn flush(&mut self) -> std::io::Result<()> {
189 self.stdin.flush()
190 }
191 }
192
193 impl Dashboard {
194 /// Everything drawn so far, control sequences stripped.
195 pub fn screen(&self) -> String {
196 screen_text(&self.screen.lock().unwrap())
197 }
198
199 /// Block until `needle` has been drawn. Returns whether it appeared.
200 ///
201 /// The screen accumulates, so this answers "has this been on screen at any
202 /// point", which is what a test that later asserts on the same accumulated
203 /// text wants. A timeout is reported and execution continues, so the
204 /// assertion the test came for is the thing that fails, with the screen
205 /// attached.
206 pub fn wait_for(&self, needle: &str) -> bool {
207 let wanted = squeezed(needle);
208 let found = wait_until(PTY_TIMEOUT, || squeezed(&self.screen()).contains(&wanted));
209 if !found {
210 eprintln!(
211 "wait_for({:?}) timed out after {:?}; screen so far:\n{}",
212 needle,
213 PTY_TIMEOUT,
214 self.screen()
215 );
216 }
217 found
218 }
219
220 /// Type `keys`, then block until `then` has been drawn.
221 pub fn press(&mut self, keys: &str, then: &str) -> bool {
222 let _ = self.write_all(keys.as_bytes());
223 self.wait_for(then)
224 }
225
226 /// Type `keys` with nothing to wait for afterwards.
227 ///
228 /// Safe only because a pty delivers keystrokes in order: the program reads
229 /// them from the terminal's input queue in the order they were written, so
230 /// a later key cannot overtake an earlier one and no pause is needed
231 /// between them. What a pause would buy is a chance to *observe* the
232 /// effect, which is what `press` is for.
233 pub fn send(&mut self, keys: &str) {
234 let _ = self.write_all(keys.as_bytes());
235 }
236
237 /// Block until `cond` holds — for an effect that is not on the screen,
238 /// such as an event reaching the collab DAG.
239 pub fn wait_for_condition(&self, what: &str, cond: impl FnMut() -> bool) -> bool {
240 let found = wait_until(PTY_TIMEOUT, cond);
241 if !found {
242 eprintln!(
243 "waiting for {} timed out after {:?}; screen so far:\n{}",
244 what,
245 PTY_TIMEOUT,
246 self.screen()
247 );
248 }
249 found
250 }
251 }
252
253 // ===========================================================================
69 // Library-level helpers (for collab_test / sync_test) 254 // Library-level helpers (for collab_test / sync_test)
70 // =========================================================================== 255 // ===========================================================================
71 256
@@ -562,16 +747,25 @@ impl TestRepo {
562 .spawn() 747 .spawn()
563 .expect("failed to launch git-collab in a pty"); 748 .expect("failed to launch git-collab in a pty");
564 749
565 let deadline = Instant::now() + Duration::from_secs(20); 750 // Drained from the first instant: see `Drain`.
566 loop { 751 let out = Drain::start(child.stdout.take().expect("pty stdout piped"));
567 if child.try_wait().expect("poll pty process").is_some() { 752 let err = Drain::start(child.stderr.take().expect("pty stderr piped"));
568 return child.wait_with_output().expect("collect pty output"); 753
569 } 754 let exited = wait_until(PTY_TIMEOUT, || {
570 if Instant::now() >= deadline { 755 child.try_wait().expect("poll pty process").is_some()
571 let _ = child.kill(); 756 });
572 panic!("git-collab {:?} hung in a pty (editor path did not return)", args); 757 if !exited {
573 } 758 let _ = child.kill();
574 thread::sleep(Duration::from_millis(20)); 759 panic!(
760 "git-collab {:?} hung in a pty (editor path did not return)",
761 args
762 );
763 }
764 let status = child.wait().expect("collect pty exit status");
765 Output {
766 status,
767 stdout: out.finish(),
768 stderr: err.finish(),
575 } 769 }
576 } 770 }
577 771
@@ -648,24 +842,27 @@ impl TestRepo {
648 .spawn() 842 .spawn()
649 .expect("failed to launch dashboard in pty"); 843 .expect("failed to launch dashboard in pty");
650 844
845 let out = Drain::start(child.stdout.take().expect("dashboard stdout piped"));
846 let err = Drain::start(child.stderr.take().expect("dashboard stderr piped"));
847
651 if let Some(mut stdin) = child.stdin.take() { 848 if let Some(mut stdin) = child.stdin.take() {
652 stdin.write_all(input.as_bytes()).unwrap(); 849 stdin.write_all(input.as_bytes()).unwrap();
653 } 850 }
654 851
655 let deadline = Instant::now() + Duration::from_secs(5); 852 let exited = wait_until(PTY_TIMEOUT, || {
656 loop { 853 child
657 if let Some(_status) = child.try_wait().expect("failed to poll dashboard process") { 854 .try_wait()
658 return child 855 .expect("failed to poll dashboard process")
659 .wait_with_output() 856 .is_some()
660 .expect("failed to collect dashboard output"); 857 });
661 } 858 if !exited {
662 if Instant::now() >= deadline { 859 let _ = child.kill();
663 let _ = child.kill(); 860 }
664 return child 861 let status = child.wait().expect("failed to collect dashboard exit status");
665 .wait_with_output() 862 Output {
666 .expect("failed to collect timed out dashboard output"); 863 status,
667 } 864 stdout: out.finish(),
668 thread::sleep(Duration::from_millis(20)); 865 stderr: err.finish(),
669 } 866 }
670 } 867 }
671 868
@@ -677,6 +874,11 @@ impl TestRepo {
677 /// which cannot express "open the dashboard, let something else move a ref, 874 /// which cannot express "open the dashboard, let something else move a ref,
678 /// then look at the screen". `extra_env` is applied last, so a test can set 875 /// then look at the screen". `extra_env` is applied last, so a test can set
679 /// `EDITOR` to a script and exercise the suspend path with no human. 876 /// `EDITOR` to a script and exercise the suspend path with no human.
877 ///
878 /// The callback gets a [`Dashboard`], which can read the screen while the
879 /// program is still running — so a test waits for the frame that proves
880 /// the last key landed rather than for a number of milliseconds that
881 /// happened to be enough here.
680 pub fn run_dashboard_driven<F>( 882 pub fn run_dashboard_driven<F>(
681 &self, 883 &self,
682 cols: u16, 884 cols: u16,
@@ -685,7 +887,7 @@ impl TestRepo {
685 drive: F, 887 drive: F,
686 ) -> Output 888 ) -> Output
687 where 889 where
688 F: FnOnce(&mut std::process::ChildStdin), 890 F: FnOnce(&mut Dashboard),
689 { 891 {
690 let mut command = Command::new("script"); 892 let mut command = Command::new("script");
691 self.apply_env(&mut command); 893 self.apply_env(&mut command);
@@ -710,29 +912,39 @@ impl TestRepo {
710 .spawn() 912 .spawn()
711 .expect("failed to launch dashboard in pty"); 913 .expect("failed to launch dashboard in pty");
712 914
915 let out = Drain::start(child.stdout.take().expect("dashboard stdout piped"));
916 let err = Drain::start(child.stderr.take().expect("dashboard stderr piped"));
917
713 { 918 {
714 let mut stdin = child.stdin.take().expect("dashboard stdin piped"); 919 let mut dash = Dashboard {
715 // Let the dashboard reach its first draw before anything is typed: 920 stdin: child.stdin.take().expect("dashboard stdin piped"),
716 // keys delivered before the event loop starts are read by the 921 screen: out.live(),
717 // terminal, not by the program under test. 922 };
718 thread::sleep(Duration::from_millis(600)); 923 // Wait for the first frame rather than guessing at how long it
719 drive(&mut stdin); 924 // takes. Raw mode is enabled before the alternate screen is
925 // entered and before anything is drawn, so a frame on screen is
926 // proof that keys typed now reach the program rather than the
927 // terminal's line editor.
928 dash.wait_for("DASHBOARD");
929 drive(&mut dash);
930 // `dash` drops here, closing stdin, which `script` forwards as the
931 // end of input.
720 } 932 }
721 933
722 let deadline = Instant::now() + Duration::from_secs(30); 934 let exited = wait_until(PTY_TIMEOUT, || {
723 loop { 935 child
724 if child.try_wait().expect("failed to poll dashboard process").is_some() { 936 .try_wait()
725 return child 937 .expect("failed to poll dashboard process")
726 .wait_with_output() 938 .is_some()
727 .expect("failed to collect dashboard output"); 939 });
728 } 940 if !exited {
729 if Instant::now() >= deadline { 941 let _ = child.kill();
730 let _ = child.kill(); 942 }
731 return child 943 let status = child.wait().expect("failed to collect dashboard exit status");
732 .wait_with_output() 944 Output {
733 .expect("failed to collect timed out dashboard output"); 945 status,
734 } 946 stdout: out.finish(),
735 thread::sleep(Duration::from_millis(20)); 947 stderr: err.finish(),
736 } 948 }
737 } 949 }
738 950
tests/tui_review_test.rs
Old New
@@ -10,7 +10,6 @@
10 mod common; 10 mod common;
11 11
12 use common::{screen_text, TestRepo}; 12 use common::{screen_text, TestRepo};
13 use std::io::Write;
14 use std::thread; 13 use std::thread;
15 use std::time::Duration; 14 use std::time::Duration;
16 15
@@ -64,16 +63,15 @@ fn an_external_write_raises_the_banner() {
64 let repo = TestRepo::new("Reader", "reader@example.com"); 63 let repo = TestRepo::new("Reader", "reader@example.com");
65 repo.issue_open("the one already on screen"); 64 repo.issue_open("the one already on screen");
66 65
67 let out = repo.run_dashboard_driven(100, 24, &[], |stdin| { 66 let out = repo.run_dashboard_driven(100, 24, &[], |dash| {
68 // The dashboard is up and has folded what exists. Now be the agent 67 // The dashboard is up and has folded what exists. Now be the agent
69 // that lands something while the reviewer is reading. `repo.run_ok` 68 // that lands something while the reviewer is reading. `repo.run_ok`
70 // targets the temp repo and its isolated home — running the binary by 69 // targets the temp repo and its isolated home — running the binary by
71 // hand here would write to whatever directory the test happens to be 70 // hand here would write to whatever directory the test happens to be
72 // started from. 71 // started from.
73 thread::sleep(Duration::from_millis(300));
74 repo.issue_open("landed while you were reading"); 72 repo.issue_open("landed while you were reading");
75 thread::sleep(Duration::from_millis(4000)); 73 dash.wait_for("new event");
76 let _ = stdin.write_all(b"q"); 74 dash.send("q");
77 }); 75 });
78 let text = screen_text(&out.stdout); 76 let text = screen_text(&out.stdout);
79 assert!( 77 assert!(
@@ -90,10 +88,9 @@ fn n_composes_the_issue_body_in_the_editor() {
90 let repo = TestRepo::new("Reader", "reader@example.com"); 88 let repo = TestRepo::new("Reader", "reader@example.com");
91 let editor = editor_writing(&repo, "issue-body.sh", "the long markdown body"); 89 let editor = editor_writing(&repo, "issue-body.sh", "the long markdown body");
92 90
93 repo.run_dashboard_driven(100, 24, &[("EDITOR", &editor)], |stdin| { 91 repo.run_dashboard_driven(100, 24, &[("EDITOR", &editor)], |dash| {
94 let _ = stdin.write_all(b"nA new issue\r"); 92 dash.press("nA new issue\r", "Issue created");
95 thread::sleep(Duration::from_millis(2000)); 93 dash.send("q");
96 let _ = stdin.write_all(b"q");
97 }); 94 });
98 95
99 let listed = repo.run_ok(&["issue", "list"]); 96 let listed = repo.run_ok(&["issue", "list"]);
@@ -117,11 +114,19 @@ fn the_dashboards_own_write_does_not_raise_the_banner() {
117 repo.issue_open("existing"); 114 repo.issue_open("existing");
118 let editor = editor_writing(&repo, "issue-editor.sh", "a body for it"); 115 let editor = editor_writing(&repo, "issue-editor.sh", "a body for it");
119 116
120 let out = repo.run_dashboard_driven(100, 24, &[("EDITOR", &editor)], |stdin| { 117 let out = repo.run_dashboard_driven(100, 24, &[("EDITOR", &editor)], |dash| {
121 // `n`, a title, Enter — which opens the editor for the body. 118 // `n`, a title, Enter — which opens the editor for the body.
122 let _ = stdin.write_all(b"nmine\r"); 119 dash.press("nmine\r", "Issue created");
123 thread::sleep(Duration::from_millis(4000)); 120 // The claim here is an absence, and an absence has nothing to wait
124 let _ = stdin.write_all(b"q"); 121 // for. The dashboard compares collab tips every two seconds, so
122 // sitting still for longer than one of those periods is what makes
123 // "no banner" mean "no banner was ever going to be raised" rather
124 // than "we did not stay long enough to see it". This is the only
125 // wall-clock wait left in the file, and it is bounded by the
126 // program's own poll interval rather than by a guess about the
127 // machine.
128 thread::sleep(Duration::from_millis(2500));
129 dash.send("q");
125 }); 130 });
126 let text = screen_text(&out.stdout); 131 let text = screen_text(&out.stdout);
127 // Vacuously true if nothing was written, so check the write landed first. 132 // Vacuously true if nothing was written, so check the write landed first.
@@ -157,12 +162,13 @@ fn a_diff_line_wider_than_the_pane_is_not_clipped_away() {
157 repo.run_ok(&["patch", "create", "-t", "A wide patch", "-B", "feature/wide"]); 162 repo.run_ok(&["patch", "create", "-t", "A wide patch", "-B", "feature/wide"]);
158 repo.git(&["checkout", "main"]); 163 repo.git(&["checkout", "main"]);
159 164
160 let out = repo.run_dashboard_driven(100, 45, &[], |stdin| { 165 let out = repo.run_dashboard_driven(100, 45, &[], |dash| {
161 let _ = stdin.write_all(b"P"); 166 dash.press("P", "Patches (open)");
162 thread::sleep(Duration::from_millis(400)); 167 // Waiting for the far end of the line is not the assertion: if it
163 let _ = stdin.write_all(b"\r"); 168 // never arrives the wait gives up and the assertion below fails with
164 thread::sleep(Duration::from_millis(1200)); 169 // the screen attached, which is the same failure as before, sooner.
165 let _ = stdin.write_all(b"q"); 170 dash.press("\r", "ENDOFTHEWIDELINE");
171 dash.send("q");
166 }); 172 });
167 let text = screen_text(&out.stdout); 173 let text = screen_text(&out.stdout);
168 assert!( 174 assert!(
@@ -207,14 +213,16 @@ fn no_pane_prints_a_stored_timestamp_in_full() {
207 213
208 // Walk the panes a reader walks: the issue detail, then the patch list, 214 // Walk the panes a reader walks: the issue detail, then the patch list,
209 // then the patch detail with its revisions, comments and diff. 215 // then the patch detail with its revisions, comments and diff.
210 let out = repo.run_dashboard_driven(120, 45, &[], |stdin| { 216 let out = repo.run_dashboard_driven(120, 45, &[], |dash| {
211 let _ = stdin.write_all(b"\t"); 217 // The event loop draws once per key it reads, so every keystroke gets
212 thread::sleep(Duration::from_millis(400)); 218 // a frame of its own and none of these needs a pause behind it. The
213 let _ = stdin.write_all(b"P"); 219 // last pane is the one to wait for: quitting before the patch detail
214 thread::sleep(Duration::from_millis(400)); 220 // is drawn would narrow what the `+00:00` check below covers.
215 let _ = stdin.write_all(b"\r"); 221 dash.send("\t");
216 thread::sleep(Duration::from_millis(800)); 222 dash.press("P", "Patches (open)");
217 let _ = stdin.write_all(b"q"); 223 dash.press("\r", "--- Revisions ---");
224 dash.wait_for(&expected);
225 dash.send("q");
218 }); 226 });
219 let text = screen_text(&out.stdout); 227 let text = screen_text(&out.stdout);
220 228
@@ -241,17 +249,16 @@ fn r_records_a_review_composed_in_the_editor() {
241 let (repo, id) = repo_with_patch(); 249 let (repo, id) = repo_with_patch();
242 let editor = editor_writing(&repo, "review-editor.sh", "this reads well"); 250 let editor = editor_writing(&repo, "review-editor.sh", "this reads well");
243 251
244 repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |stdin| { 252 repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |dash| {
245 // Switch to the patch list, open the patch, review, approve. 253 // Switch to the patch list, open the patch, review, approve.
246 let _ = stdin.write_all(b"P"); 254 dash.press("P", "Patches (open)");
247 thread::sleep(Duration::from_millis(300)); 255 dash.press("\r", "--- Revisions ---");
248 let _ = stdin.write_all(b"\r"); 256 // `R` opens the verdict prompt and `a` answers it. A pty hands
249 thread::sleep(Duration::from_millis(500)); 257 // keystrokes to the program in the order they were written, so
250 let _ = stdin.write_all(b"R"); 258 // nothing has to pause between the two.
251 thread::sleep(Duration::from_millis(300)); 259 dash.send("R");
252 let _ = stdin.write_all(b"a"); 260 dash.press("a", "Review recorded");
253 thread::sleep(Duration::from_millis(2000)); 261 dash.send("q");
254 let _ = stdin.write_all(b"q");
255 }); 262 });
256 263
257 let shown = repo.run_ok(&["patch", "show", &id]); 264 let shown = repo.run_ok(&["patch", "show", &id]);
@@ -271,19 +278,15 @@ fn c_on_a_diff_line_records_an_inline_comment() {
271 let (repo, id) = repo_with_patch(); 278 let (repo, id) = repo_with_patch();
272 let editor = editor_writing(&repo, "comment-editor.sh", "why the rename?"); 279 let editor = editor_writing(&repo, "comment-editor.sh", "why the rename?");
273 280
274 repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |stdin| { 281 repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |dash| {
275 let _ = stdin.write_all(b"P"); 282 dash.press("P", "Patches (open)");
276 thread::sleep(Duration::from_millis(300)); 283 dash.press("\r", "--- Revisions ---");
277 let _ = stdin.write_all(b"\r");
278 thread::sleep(Duration::from_millis(500));
279 // Walk down to a line of the diff body. The detail pane opens with the 284 // Walk down to a line of the diff body. The detail pane opens with the
280 // cursor at the top, and the header, revision list and diff header sit 285 // cursor at the top, and the header, revision list and diff header sit
281 // above the first anchorable line. 286 // above the first anchorable line.
282 let _ = stdin.write_all(b"jjjjjjjjjjjjjjjj"); 287 dash.send("jjjjjjjjjjjjjjjj");
283 thread::sleep(Duration::from_millis(400)); 288 dash.press("c", "Comment added on");
284 let _ = stdin.write_all(b"c"); 289 dash.send("q");
285 thread::sleep(Duration::from_millis(2000));
286 let _ = stdin.write_all(b"q");
287 }); 290 });
288 291
289 let shown = repo.run_ok(&["patch", "show", &id]); 292 let shown = repo.run_ok(&["patch", "show", &id]);
@@ -307,14 +310,15 @@ fn a_comment_only_body_aborts_and_records_nothing() {
307 // An editor that leaves the seeded `#` lines exactly as they were. 310 // An editor that leaves the seeded `#` lines exactly as they were.
308 let editor = repo.write_script("noop-editor.sh", "#!/bin/sh\nexit 0\n"); 311 let editor = repo.write_script("noop-editor.sh", "#!/bin/sh\nexit 0\n");
309 312
310 let out = repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |stdin| { 313 let out = repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |dash| {
311 let _ = stdin.write_all(b"P"); 314 dash.press("P", "Patches (open)");
312 thread::sleep(Duration::from_millis(300)); 315 dash.press("\r", "--- Revisions ---");
313 let _ = stdin.write_all(b"\r"); 316 // The editor runs and returns; the dashboard saying so is what marks
314 thread::sleep(Duration::from_millis(500)); 317 // the round trip finished, and it is the same sentence this test is
315 let _ = stdin.write_all(b"c"); 318 // about. Waiting for it is not the assertion — if it never comes, the
316 thread::sleep(Duration::from_millis(2000)); 319 // assertion below fails with the screen attached.
317 let _ = stdin.write_all(b"q"); 320 dash.press("c", "Aborting");
321 dash.send("q");
318 }); 322 });
319 323
320 let shown = repo.run_ok(&["patch", "show", &id]); 324 let shown = repo.run_ok(&["patch", "show", &id]);
@@ -343,14 +347,11 @@ fn an_editor_killed_mid_write_records_nothing() {
343 "#!/bin/sh\nprintf 'half a thought' > \"$1\"\nkill -TERM $$\n", 347 "#!/bin/sh\nprintf 'half a thought' > \"$1\"\nkill -TERM $$\n",
344 ); 348 );
345 349
346 let out = repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |stdin| { 350 let out = repo.run_dashboard_driven(100, 40, &[("EDITOR", &editor)], |dash| {
347 let _ = stdin.write_all(b"P"); 351 dash.press("P", "Patches (open)");
348 thread::sleep(Duration::from_millis(300)); 352 dash.press("\r", "--- Revisions ---");
349 let _ = stdin.write_all(b"\r"); 353 dash.press("c", "Editor exited");
350 thread::sleep(Duration::from_millis(500)); 354 dash.send("q");
351 let _ = stdin.write_all(b"c");
352 thread::sleep(Duration::from_millis(2000));
353 let _ = stdin.write_all(b"q");
354 }); 355 });
355 356
356 let shown = repo.run_ok(&["patch", "show", &id]); 357 let shown = repo.run_ok(&["patch", "show", &id]);
@@ -392,16 +393,12 @@ fn x_resolves_and_reopens_the_comment_under_the_cursor() {
392 // The detail pane opens with the cursor on row 0. Above the inline comment 393 // The detail pane opens with the cursor on row 0. Above the inline comment
393 // sit six header rows, a blank, the Revisions heading, one revision, a 394 // sit six header rows, a blank, the Revisions heading, one revision, a
394 // blank and the Inline Comments heading — eleven rows. 395 // blank and the Inline Comments heading — eleven rows.
395 repo.run_dashboard_driven(100, 40, &[], |stdin| { 396 repo.run_dashboard_driven(100, 40, &[], |dash| {
396 let _ = stdin.write_all(b"P"); 397 dash.press("P", "Patches (open)");
397 thread::sleep(Duration::from_millis(300)); 398 dash.press("\r", "--- Revisions ---");
398 let _ = stdin.write_all(b"\r"); 399 dash.send("jjjjjjjjjjj");
399 thread::sleep(Duration::from_millis(500)); 400 dash.press("x", "resolved");
400 let _ = stdin.write_all(b"jjjjjjjjjjj"); 401 dash.send("q");
401 thread::sleep(Duration::from_millis(400));
402 let _ = stdin.write_all(b"x");
403 thread::sleep(Duration::from_millis(1500));
404 let _ = stdin.write_all(b"q");
405 }); 402 });
406 403
407 let shown = repo.run_ok(&["patch", "show", &id]); 404 let shown = repo.run_ok(&["patch", "show", &id]);
@@ -413,16 +410,12 @@ fn x_resolves_and_reopens_the_comment_under_the_cursor() {
413 410
414 // The same key withdraws the claim. The row moved into the Resolved 411 // The same key withdraws the claim. The row moved into the Resolved
415 // section, one line further down. 412 // section, one line further down.
416 repo.run_dashboard_driven(100, 40, &[], |stdin| { 413 repo.run_dashboard_driven(100, 40, &[], |dash| {
417 let _ = stdin.write_all(b"P"); 414 dash.press("P", "Patches (open)");
418 thread::sleep(Duration::from_millis(300)); 415 dash.press("\r", "--- Revisions ---");
419 let _ = stdin.write_all(b"\r"); 416 dash.send("jjjjjjjjjjj");
420 thread::sleep(Duration::from_millis(500)); 417 dash.press("x", "reopened");
421 let _ = stdin.write_all(b"jjjjjjjjjjj"); 418 dash.send("q");
422 thread::sleep(Duration::from_millis(400));
423 let _ = stdin.write_all(b"x");
424 thread::sleep(Duration::from_millis(1500));
425 let _ = stdin.write_all(b"q");
426 }); 419 });
427 420
428 let shown = repo.run_ok(&["patch", "show", &id]); 421 let shown = repo.run_ok(&["patch", "show", &id]);
@@ -468,18 +461,14 @@ fn a_shows_what_answered_the_comment_under_the_cursor() {
468 .to_string(); 461 .to_string();
469 repo.run_ok(&["patch", "resolve", &id, &comment_id]); 462 repo.run_ok(&["patch", "resolve", &id, &comment_id]);
470 463
471 let out = repo.run_dashboard_driven(100, 40, &[], |stdin| { 464 let out = repo.run_dashboard_driven(100, 40, &[], |dash| {
472 let _ = stdin.write_all(b"P"); 465 dash.press("P", "Patches (open)");
473 thread::sleep(Duration::from_millis(300)); 466 dash.press("\r", "--- Revisions ---");
474 let _ = stdin.write_all(b"\r");
475 thread::sleep(Duration::from_millis(500));
476 // Six header rows, blank, Revisions heading, two revisions, blank, 467 // Six header rows, blank, Revisions heading, two revisions, blank,
477 // the Resolved-comment heading — twelve rows down to the comment. 468 // the Resolved-comment heading — twelve rows down to the comment.
478 let _ = stdin.write_all(b"jjjjjjjjjjjj"); 469 dash.send("jjjjjjjjjjjj");
479 thread::sleep(Duration::from_millis(400)); 470 dash.press("a", "answered [");
480 let _ = stdin.write_all(b"a"); 471 dash.send("q");
481 thread::sleep(Duration::from_millis(1500));
482 let _ = stdin.write_all(b"q");
483 }); 472 });
484 473
485 // Matched without the interior spaces a redraw is free to skip: ratatui 474 // Matched without the interior spaces a redraw is free to skip: ratatui
@@ -503,11 +492,11 @@ fn o_checks_out_the_revision_even_with_the_branch_gone() {
503 let head = repo.git(&["rev-parse", "feature/x"]).trim().to_string(); 492 let head = repo.git(&["rev-parse", "feature/x"]).trim().to_string();
504 repo.git(&["branch", "-D", "feature/x"]); 493 repo.git(&["branch", "-D", "feature/x"]);
505 494
506 let out = repo.run_dashboard_driven(100, 40, &[], |stdin| { 495 let out = repo.run_dashboard_driven(100, 40, &[], |dash| {
507 let _ = stdin.write_all(b"P"); 496 dash.press("P", "Patches (open)");
508 thread::sleep(Duration::from_millis(300)); 497 // `o` leaves the dashboard for good, so the checkout report it prints
509 let _ = stdin.write_all(b"o"); 498 // on the way out is both the acknowledgement and the exit.
510 thread::sleep(Duration::from_millis(1500)); 499 dash.press("o", "Checked out patch");
511 }); 500 });
512 501
513 let at = repo.git(&["rev-parse", "HEAD"]).trim().to_string(); 502 let at = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
@@ -532,14 +521,11 @@ fn o_checks_out_the_revision_even_with_the_branch_gone() {
532 fn no_editor_configured_says_so() { 521 fn no_editor_configured_says_so() {
533 let (repo, _id) = repo_with_patch(); 522 let (repo, _id) = repo_with_patch();
534 523
535 let out = repo.run_dashboard_driven(100, 40, &[("EDITOR", ""), ("VISUAL", "")], |stdin| { 524 let out = repo.run_dashboard_driven(100, 40, &[("EDITOR", ""), ("VISUAL", "")], |dash| {
536 let _ = stdin.write_all(b"P"); 525 dash.press("P", "Patches (open)");
537 thread::sleep(Duration::from_millis(300)); 526 dash.press("\r", "--- Revisions ---");
538 let _ = stdin.write_all(b"\r"); 527 dash.press("c", "EDITOR");
539 thread::sleep(Duration::from_millis(500)); 528 dash.send("q");
540 let _ = stdin.write_all(b"c");
541 thread::sleep(Duration::from_millis(1000));
542 let _ = stdin.write_all(b"q");
543 }); 529 });
544 let text = screen_text(&out.stdout); 530 let text = screen_text(&out.stdout);
545 assert!( 531 assert!(