Skip to content

Commit cb54732

Browse files
committed
fix(tui-e2e): Make E2E snapshot tests more resilient to scroll timing
The TUI E2E tests were flaky due to terminal viewport scroll timing causing the startup header to sometimes be present and sometimes not in snapshots. This made tests non-deterministic. Changes: - Add normalize_for_input_snapshot() function that strips the startup header block (both boxed and plain text forms) for consistent snapshots - Update all E2E tests to use the new normalization function - Regenerate all snapshots without header content for determinism - Keep original normalize_for_snapshot() for cases where header content needs to be tested explicitly The header is detected by looking for box characters, To get started, or Welcome to Codex, then stripping everything up to and including the /review command line.
1 parent f41389e commit cb54732

16 files changed

Lines changed: 274 additions & 123 deletions

codex-rs/acp/src/translator.rs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -180,21 +180,18 @@ fn extract_command_from_tool_call(tool_call: &acp::ToolCallUpdate) -> Vec<String
180180

181181
// Add stringified raw_input if present
182182
if let Some(input) = &tool_call.fields.raw_input
183-
&& let Ok(args_str) = serde_json::to_string(input) {
184-
cmd.push(args_str);
185-
}
183+
&& let Ok(args_str) = serde_json::to_string(input)
184+
{
185+
cmd.push(args_str);
186+
}
186187

187188
cmd
188189
}
189190

190191
/// Extract a human-readable reason from the tool call.
191192
fn extract_reason_from_tool_call(tool_call: &acp::ToolCallUpdate) -> Option<String> {
192193
// Use the title as a basic description, or fall back to ID
193-
let name = tool_call
194-
.fields
195-
.title
196-
.as_deref()
197-
.unwrap_or("unknown tool");
194+
let name = tool_call.fields.title.as_deref().unwrap_or("unknown tool");
198195
Some(format!("ACP agent requests permission to use: {name}"))
199196
}
200197

codex-rs/tui-pty-e2e/docs.md

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ impl Drop for TuiSession {
4747

4848
The crate exports helper functions for consistent test patterns:
4949
- `TIMEOUT: Duration` - Standard 5-second timeout constant for use across all tests
50+
- `TIMEOUT_INPUT: Duration` - 300ms timeout for input stabilization before snapshots
5051
- `normalize_for_snapshot(contents: String) -> String` - Normalizes dynamic content for snapshot testing (see below)
52+
- `normalize_for_input_snapshot(contents: String) -> String` - Extends normalization by stripping the startup header block (see below)
5153

5254
**Automatic Test Isolation:**
5355

@@ -142,6 +144,7 @@ This delay allows the PTY subprocess time to process input and update the displa
142144
| `@/codex-rs/tui-pty-e2e/tests/prompt_flow.rs` | Prompt submission and agent responses |
143145
| `@/codex-rs/tui-pty-e2e/tests/input_handling.rs` | Text editing, backspace, Ctrl-C clearing, arrow key navigation with snapshot testing |
144146
| `@/codex-rs/tui-pty-e2e/tests/streaming.rs` | Prompt submission with timing delays, agent response streaming |
147+
| `@/codex-rs/tui-pty-e2e/tests/acp_mode.rs` | ACP mode startup and response flow - validates TUI works with ACP wire API and mock agent; includes TDD marker test for approval bridging (`#[ignore]`) |
145148
| `@/codex-rs/tui-pty-e2e/tests/live_acp.rs` | Live authenticated ACP tests for Gemini and Claude with real API connections (opt-in, marked `#[ignore]`) |
146149

147150
**Snapshot Files:**
@@ -151,6 +154,7 @@ This delay allows the PTY subprocess time to process input and update the displa
151154
| `@/codex-rs/tui-pty-e2e/tests/snapshots/startup__*.snap` | Various startup screen scenarios (welcome, dimensions, temp directory, trust screen) |
152155
| `@/codex-rs/tui-pty-e2e/tests/snapshots/input_handling__*.snap` | Input handling scenarios (ctrl-c clear, typing/backspace, model changed) |
153156
| `@/codex-rs/tui-pty-e2e/tests/snapshots/streaming__submit_input.snap` | Prompt submission and streaming response |
157+
| `@/codex-rs/tui-pty-e2e/tests/snapshots/acp_mode__*.snap` | ACP mode startup screen |
154158

155159
**Snapshot Testing with Insta:**
156160

@@ -163,24 +167,27 @@ Snapshots stored in `@/codex-rs/tui-pty-e2e/tests/snapshots/*.snap` for regressi
163167

164168
**Snapshot Normalization:**
165169

166-
The `normalize_for_snapshot()` helper function exported from `@/codex-rs/tui-pty-e2e/src/lib.rs` ensures stable snapshots across test runs by replacing dynamic content:
170+
Two normalization helpers in `@/codex-rs/tui-pty-e2e/src/lib.rs` ensure stable snapshots:
167171

168-
Normalization rules:
172+
| Function | Use Case |
173+
|----------|----------|
174+
| `normalize_for_snapshot()` | General snapshots that should include the startup header |
175+
| `normalize_for_input_snapshot()` | Input-focused tests where header visibility varies with scroll timing |
176+
177+
**`normalize_for_snapshot()`** - Base normalization rules:
169178
1. Temp directory paths (`/tmp/.tmpXXXXXX`) → `[TMP_DIR]` placeholder
170179
2. Random default prompts on lines starting with ```[DEFAULT_PROMPT]` placeholder
171180
- Detects specific default prompt patterns: "Find and fix a bug", "Explain this codebase", "Write tests for", etc.
172181
- Preserves user-entered prompts and UI text like "? for shortcuts"
173182

174-
Implementation in `@/codex-rs/tui-pty-e2e/src/lib.rs:456-488`:
175-
```rust
176-
pub fn normalize_for_snapshot(contents: String) -> String {
177-
// Replace /tmp/.tmpXXXXXX with [TMP_DIR]
178-
// Replace known default prompts with [DEFAULT_PROMPT]
179-
// Preserves UI structure and user input
180-
}
181-
```
183+
**`normalize_for_input_snapshot()`** - Extends base normalization by stripping the startup header block:
184+
- Detects the header block (lines containing `╭──` through the `/review` and `/model` command list)
185+
- Removes the entire header section to prevent flaky snapshots
186+
- Used by input handling tests in `@/codex-rs/tui-pty-e2e/tests/input_handling.rs`
187+
188+
**Why Two Functions:** Terminal render timing can cause the startup header block to scroll partially in or out of the viewport before a snapshot is taken. For tests focused on input handling, the header presence is irrelevant - only the input area matters. By stripping the header, `normalize_for_input_snapshot()` produces deterministic snapshots regardless of scroll state.
182189

183-
This normalization allows snapshot assertions to focus on UI structure and static content rather than ephemeral runtime values. All tests import and use this function consistently: `use tui_pty_e2e::{normalize_for_snapshot, ...};`
190+
This normalization allows snapshot assertions to focus on UI structure and static content rather than ephemeral runtime values.
184191

185192
**PTY Implementation Details:**
186193

codex-rs/tui-pty-e2e/src/lib.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,3 +581,49 @@ pub fn normalize_for_snapshot(contents: String) -> String {
581581

582582
lines.join("\n")
583583
}
584+
585+
/// Normalize for input tests - strips header for consistent snapshot regardless of scroll state
586+
pub fn normalize_for_input_snapshot(contents: String) -> String {
587+
let normalized = normalize_for_snapshot(contents);
588+
589+
// Strip startup header block if present (prevents flaky snapshots due to scroll timing)
590+
// The header can appear in two forms:
591+
// 1. Boxed header with "╭──" border
592+
// 2. Plain text "To get started, describe a task..."
593+
// Both end with a list of commands like /init, /status, /approvals, /model, /review
594+
let lines: Vec<&str> = normalized.lines().collect();
595+
596+
// Detect if header is present (either boxed or plain text form)
597+
let has_header = lines.iter().any(|l| {
598+
l.contains("╭──")
599+
|| l.contains("To get started, describe a task")
600+
|| l.contains("Welcome to Codex")
601+
});
602+
603+
if has_header {
604+
// Find where the header ends (after the /review command line)
605+
let mut skip_until = 0;
606+
for (i, line) in lines.iter().enumerate() {
607+
// The /review line marks the end of the command list
608+
if line.contains("/review") {
609+
skip_until = i + 1;
610+
break;
611+
}
612+
}
613+
// Skip empty lines after the header block
614+
while skip_until < lines.len() && lines[skip_until].trim().is_empty() {
615+
skip_until += 1;
616+
}
617+
if skip_until > 0 {
618+
lines
619+
.into_iter()
620+
.skip(skip_until)
621+
.collect::<Vec<_>>()
622+
.join("\n")
623+
} else {
624+
normalized
625+
}
626+
} else {
627+
normalized
628+
}
629+
}
Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
//! E2E tests for ACP mode startup and approval bridging
2+
//!
3+
//! These tests verify that:
4+
//! 1. ACP mode starts correctly when configured via wire_api = "acp"
5+
//! 2. The approval bridging infrastructure works correctly
6+
//! 3. Permission requests from ACP agents are properly displayed in the TUI
7+
8+
use std::time::Duration;
9+
use tui_pty_e2e::normalize_for_input_snapshot;
10+
use tui_pty_e2e::Key;
11+
use tui_pty_e2e::SessionConfig;
12+
use tui_pty_e2e::TuiSession;
13+
use tui_pty_e2e::TIMEOUT;
14+
use tui_pty_e2e::TIMEOUT_INPUT;
15+
16+
/// Test that ACP mode starts successfully with mock-model
17+
#[test]
18+
fn test_acp_mode_startup_with_mock_agent() {
19+
let config = SessionConfig::new().with_model("mock-model".to_owned());
20+
21+
let mut session =
22+
TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode");
23+
24+
// Wait for the main prompt to appear (indicated by the chevron character)
25+
session
26+
.wait_for_text("›", TIMEOUT)
27+
.expect("ACP mode TUI should start successfully with mock agent");
28+
29+
std::thread::sleep(TIMEOUT_INPUT);
30+
31+
let contents = session.screen_contents();
32+
33+
// Verify we're in the TUI and not stuck at an error screen
34+
assert!(
35+
contents.contains("›") && contents.contains("context left"),
36+
"Should show main prompt with context indicator in ACP mode, got: {}",
37+
contents
38+
);
39+
40+
// Should NOT show any ACP-related errors
41+
assert!(
42+
!contents.contains("Error") && !contents.contains("error"),
43+
"ACP mode should start without errors, got: {}",
44+
contents
45+
);
46+
}
47+
48+
/// Test that ACP mode can send a prompt and receive a response from mock agent
49+
#[test]
50+
fn test_acp_mode_prompt_response_flow() {
51+
let config = SessionConfig::new()
52+
.with_model("mock-model".to_owned())
53+
.with_mock_response("Hello from ACP mock agent!");
54+
55+
let mut session =
56+
TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode");
57+
58+
// Wait for startup
59+
session
60+
.wait_for_text("›", TIMEOUT)
61+
.expect("ACP mode should start");
62+
63+
std::thread::sleep(TIMEOUT_INPUT);
64+
65+
// Send a test prompt
66+
session.send_str("Test ACP prompt").unwrap();
67+
std::thread::sleep(TIMEOUT_INPUT);
68+
session.send_key(Key::Enter).unwrap();
69+
70+
// Wait for the mock response
71+
session
72+
.wait_for_text("Hello from ACP mock agent!", TIMEOUT)
73+
.expect("Should receive response from mock ACP agent");
74+
}
75+
76+
/// Test that ACP approval requests are displayed in the TUI
77+
///
78+
/// This test verifies the approval bridging infrastructure by:
79+
/// 1. Configuring the mock agent to request permission
80+
/// 2. Verifying the permission request appears in the TUI
81+
/// 3. Verifying user can respond to the permission request
82+
///
83+
/// ## Prerequisites for this test to pass:
84+
/// 1. Mock agent must support MOCK_AGENT_REQUEST_PERMISSION env var
85+
/// 2. TUI must listen to AcpConnection::take_approval_receiver()
86+
/// 3. TUI must display ExecApprovalRequestEvent and send ReviewDecision back
87+
///
88+
/// This test is marked #[ignore] until approval bridging is integrated.
89+
/// Run with: cargo test -p tui-pty-e2e -- --ignored
90+
#[test]
91+
#[ignore]
92+
fn test_acp_approval_request_displayed_in_tui() {
93+
let config = SessionConfig::new()
94+
.with_model("mock-model".to_owned())
95+
// Configure mock agent to request permission before responding
96+
.with_agent_env("MOCK_AGENT_REQUEST_PERMISSION", "1");
97+
98+
let mut session =
99+
TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode");
100+
101+
// Wait for startup
102+
session
103+
.wait_for_text("›", TIMEOUT)
104+
.expect("ACP mode should start");
105+
106+
std::thread::sleep(TIMEOUT_INPUT);
107+
108+
// Send a prompt that triggers a permission request
109+
session.send_str("Run a shell command").unwrap();
110+
std::thread::sleep(TIMEOUT_INPUT);
111+
session.send_key(Key::Enter).unwrap();
112+
113+
// Wait for the approval request to appear
114+
// The TUI should display something like "ACP agent requests permission"
115+
// or show the approval popup from ExecApprovalRequestEvent
116+
let approval_appeared = session.wait_for(
117+
|screen| {
118+
screen.contains("permission")
119+
|| screen.contains("approve")
120+
|| screen.contains("allow")
121+
|| screen.contains("deny")
122+
|| screen.contains("ACP agent requests")
123+
},
124+
Duration::from_secs(10),
125+
);
126+
127+
match approval_appeared {
128+
Ok(()) => {
129+
// Approval UI appeared - verify we can see the request
130+
let contents = session.screen_contents();
131+
eprintln!("Approval request displayed:\n{}", contents);
132+
133+
// The approval UI should show options to approve or deny
134+
assert!(
135+
contents.contains("allow")
136+
|| contents.contains("approve")
137+
|| contents.contains("deny")
138+
|| contents.contains("reject"),
139+
"Approval UI should show allow/deny options, got: {}",
140+
contents
141+
);
142+
}
143+
Err(e) => {
144+
// Expected to fail until approval bridging is integrated
145+
panic!(
146+
"Approval request not displayed in TUI. \
147+
This is expected until approval bridging is integrated. \
148+
Error: {}. Screen contents:\n{}",
149+
e,
150+
session.screen_contents()
151+
);
152+
}
153+
}
154+
}
155+
156+
/// Test snapshot of ACP mode startup screen
157+
#[test]
158+
fn test_acp_mode_startup_snapshot() {
159+
let config = SessionConfig::new().with_model("mock-model".to_owned());
160+
161+
let mut session =
162+
TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode");
163+
164+
session
165+
.wait_for_text("›", TIMEOUT)
166+
.expect("ACP mode should start");
167+
168+
std::thread::sleep(TIMEOUT_INPUT);
169+
170+
insta::assert_snapshot!(
171+
"acp_mode_startup",
172+
normalize_for_input_snapshot(session.screen_contents())
173+
);
174+
}

codex-rs/tui-pty-e2e/tests/input_handling.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use insta::assert_snapshot;
22
use std::time::Duration;
3-
use tui_pty_e2e::normalize_for_snapshot;
3+
use tui_pty_e2e::normalize_for_input_snapshot;
44
use tui_pty_e2e::Key;
55
use tui_pty_e2e::TuiSession;
66
use tui_pty_e2e::TIMEOUT;
@@ -26,7 +26,7 @@ fn test_ctrl_c_clears_input() {
2626
std::thread::sleep(TIMEOUT_INPUT);
2727
assert_snapshot!(
2828
"ctrl_c_clears",
29-
normalize_for_snapshot(session.screen_contents())
29+
normalize_for_input_snapshot(session.screen_contents())
3030
);
3131
}
3232

@@ -49,7 +49,7 @@ fn test_backspace() {
4949
std::thread::sleep(TIMEOUT_INPUT);
5050
assert_snapshot!(
5151
"typing_and_backspace",
52-
normalize_for_snapshot(session.screen_contents())
52+
normalize_for_input_snapshot(session.screen_contents())
5353
);
5454
}
5555

@@ -71,6 +71,6 @@ fn test_arrows() {
7171
std::thread::sleep(TIMEOUT_INPUT);
7272
assert_snapshot!(
7373
"model_changed",
74-
normalize_for_snapshot(session.screen_contents())
74+
normalize_for_input_snapshot(session.screen_contents())
7575
);
7676
}

codex-rs/tui-pty-e2e/tests/prompt_flow.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use insta::assert_snapshot;
2-
use tui_pty_e2e::normalize_for_snapshot;
2+
use tui_pty_e2e::normalize_for_input_snapshot;
33
use tui_pty_e2e::Key;
44
use tui_pty_e2e::SessionConfig;
55
use tui_pty_e2e::TuiSession;
@@ -33,7 +33,7 @@ fn test_submit_prompt_default_response() {
3333
std::thread::sleep(TIMEOUT_INPUT);
3434
assert_snapshot!(
3535
"prompt_submitted",
36-
normalize_for_snapshot(session.screen_contents())
36+
normalize_for_input_snapshot(session.screen_contents())
3737
);
3838
}
3939

@@ -67,7 +67,7 @@ fn test_submit_prompt_missing_model() {
6767
std::thread::sleep(TIMEOUT_INPUT);
6868
assert_snapshot!(
6969
"missing_model",
70-
normalize_for_snapshot(session.screen_contents())
70+
normalize_for_input_snapshot(session.screen_contents())
7171
);
7272
}
7373

@@ -92,7 +92,7 @@ fn test_submit_prompt_custom_response() {
9292
std::thread::sleep(TIMEOUT_INPUT);
9393
assert_snapshot!(
9494
"custom_response",
95-
normalize_for_snapshot(session.screen_contents())
95+
normalize_for_input_snapshot(session.screen_contents())
9696
);
9797
}
9898

@@ -112,6 +112,6 @@ fn test_multiline_input() {
112112
std::thread::sleep(TIMEOUT_INPUT);
113113
assert_snapshot!(
114114
"multiline_input",
115-
normalize_for_snapshot(session.screen_contents())
115+
normalize_for_input_snapshot(session.screen_contents())
116116
);
117117
}

0 commit comments

Comments
 (0)