Skip to content

Commit 23a44dd

Browse files
aibrahim-oaicodex
andauthored
Stabilize permissions popup selection tests (#14966)
## What is flaky The permissions popup tests in the TUI are flaky, especially on Windows. They assume the popup opens on a specific row and that a fixed number of `Up` or `Down` keypresses will land on a specific preset. They also match popup text too loosely, so a non-selected row can satisfy the assertion. ## Why it was flaky These tests were asserting incidental rendering details rather than the actual selected permission preset. On Windows, the initial selection can differ from non-Windows runs. Some tests also searched the entire popup for text like `Guardian Approvals` or `(current)`, which can match a row that is visible but not selected. Once the popup order or current preset shifted slightly, a test could fail even though the UI behavior was still correct. ## How this PR fixes it This PR adds helpers that identify the selected popup row and selected preset name directly. The tests now assert the current selection by name, navigate to concrete target presets instead of assuming a fixed number of keypresses, and explicitly set the reviewer state in the cases that require `Guardian Approvals` to be current. ## Why this fix fixes the flakiness The assertions now track semantic state, not fragile text placement. Navigation is target-based instead of order-based, so Windows/non-Windows row differences and harmless popup layout changes no longer break the tests. That removes the scheduler- and platform-sensitive assumptions that made the popup suite intermittent. Co-authored-by: Ahmed Ibrahim <219906144+aibrahim-oai@users.noreply.github.com> Co-authored-by: Codex <noreply@openai.com>
1 parent b023886 commit 23a44dd

1 file changed

Lines changed: 113 additions & 19 deletions

File tree

codex-rs/tui/src/chatwidget/tests.rs

Lines changed: 113 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6604,6 +6604,50 @@ fn render_bottom_popup(chat: &ChatWidget, width: u16) -> String {
66046604
lines.join("\n")
66056605
}
66066606

6607+
fn selected_permissions_popup_line(popup: &str) -> &str {
6608+
popup
6609+
.lines()
6610+
.find(|line| {
6611+
line.contains('›')
6612+
&& (line.contains("Default")
6613+
|| line.contains("Read Only")
6614+
|| line.contains("Guardian Approvals")
6615+
|| line.contains("Full Access"))
6616+
})
6617+
.unwrap_or_else(|| {
6618+
panic!("expected permissions popup to have a selected preset row: {popup}")
6619+
})
6620+
}
6621+
6622+
fn selected_permissions_popup_name(popup: &str) -> &'static str {
6623+
selected_permissions_popup_line(popup)
6624+
.trim_start()
6625+
.strip_prefix('›')
6626+
.map(str::trim_start)
6627+
.and_then(|line| line.split_once(". ").map(|(_, rest)| rest))
6628+
.and_then(|line| {
6629+
["Read Only", "Default", "Guardian Approvals", "Full Access"]
6630+
.into_iter()
6631+
.find(|label| line.starts_with(label))
6632+
})
6633+
.unwrap_or_else(|| {
6634+
panic!("expected permissions popup row to start with a preset label: {popup}")
6635+
})
6636+
}
6637+
6638+
fn move_permissions_popup_selection_to(chat: &mut ChatWidget, label: &str, direction: KeyCode) {
6639+
for _ in 0..4 {
6640+
let popup = render_bottom_popup(chat, 120);
6641+
if selected_permissions_popup_name(&popup) == label {
6642+
return;
6643+
}
6644+
chat.handle_key_event(KeyEvent::from(direction));
6645+
}
6646+
6647+
let popup = render_bottom_popup(chat, 120);
6648+
panic!("expected permissions popup to select {label}: {popup}");
6649+
}
6650+
66076651
#[tokio::test]
66086652
async fn apps_popup_stays_loading_until_final_snapshot_updates() {
66096653
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await;
@@ -8333,7 +8377,25 @@ async fn permissions_selection_emits_history_cell_when_selection_changes() {
83338377
chat.config.notices.hide_full_access_warning = Some(true);
83348378

83358379
chat.open_permissions_popup();
8380+
let popup = render_bottom_popup(&chat, 120);
8381+
#[cfg(target_os = "windows")]
8382+
let expected_initial = "Read Only";
8383+
#[cfg(not(target_os = "windows"))]
8384+
let expected_initial = "Default";
8385+
assert!(
8386+
selected_permissions_popup_name(&popup) == expected_initial,
8387+
"expected permissions popup to open with {expected_initial} selected: {popup}"
8388+
);
83368389
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
8390+
let popup = render_bottom_popup(&chat, 120);
8391+
#[cfg(target_os = "windows")]
8392+
let expected_after_one_down = "Default";
8393+
#[cfg(not(target_os = "windows"))]
8394+
let expected_after_one_down = "Full Access";
8395+
assert!(
8396+
selected_permissions_popup_name(&popup) == expected_after_one_down,
8397+
"expected moving down to select {expected_after_one_down} before confirmation: {popup}"
8398+
);
83378399
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
83388400

83398401
let cells = drain_insert_history(&mut rx);
@@ -8360,9 +8422,21 @@ async fn permissions_selection_history_snapshot_after_mode_switch() {
83608422
chat.config.notices.hide_full_access_warning = Some(true);
83618423

83628424
chat.open_permissions_popup();
8363-
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
8425+
let popup = render_bottom_popup(&chat, 120);
83648426
#[cfg(target_os = "windows")]
8365-
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
8427+
let expected_initial = "Read Only";
8428+
#[cfg(not(target_os = "windows"))]
8429+
let expected_initial = "Default";
8430+
assert!(
8431+
selected_permissions_popup_name(&popup) == expected_initial,
8432+
"expected permissions popup to open with {expected_initial} selected: {popup}"
8433+
);
8434+
move_permissions_popup_selection_to(&mut chat, "Full Access", KeyCode::Down);
8435+
let popup = render_bottom_popup(&chat, 120);
8436+
assert!(
8437+
selected_permissions_popup_name(&popup) == "Full Access",
8438+
"expected navigation to land on Full Access before confirmation: {popup}"
8439+
);
83668440
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
83678441

83688442
let cells = drain_insert_history(&mut rx);
@@ -8395,10 +8469,16 @@ async fn permissions_selection_history_snapshot_full_access_to_default() {
83958469

83968470
chat.open_permissions_popup();
83978471
let popup = render_bottom_popup(&chat, 120);
8398-
chat.handle_key_event(KeyEvent::from(KeyCode::Up));
8399-
if popup.contains("Guardian Approvals") {
8400-
chat.handle_key_event(KeyEvent::from(KeyCode::Up));
8401-
}
8472+
assert!(
8473+
selected_permissions_popup_name(&popup) == "Full Access",
8474+
"expected permissions popup to open with Full Access selected: {popup}"
8475+
);
8476+
move_permissions_popup_selection_to(&mut chat, "Default", KeyCode::Up);
8477+
let popup = render_bottom_popup(&chat, 120);
8478+
assert!(
8479+
selected_permissions_popup_name(&popup) == "Default",
8480+
"expected navigation to land on Default before confirmation: {popup}"
8481+
);
84028482
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
84038483

84048484
let cells = drain_insert_history(&mut rx);
@@ -8437,6 +8517,11 @@ async fn permissions_selection_emits_history_cell_when_current_is_selected() {
84378517
.expect("set sandbox policy");
84388518

84398519
chat.open_permissions_popup();
8520+
let popup = render_bottom_popup(&chat, 120);
8521+
assert!(
8522+
selected_permissions_popup_name(&popup) == "Default",
8523+
"expected permissions popup to open with Default selected: {popup}"
8524+
);
84408525
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
84418526

84428527
let cells = drain_insert_history(&mut rx);
@@ -8542,7 +8627,8 @@ async fn permissions_selection_marks_guardian_approvals_current_after_session_co
85428627
let popup = render_bottom_popup(&chat, 120);
85438628

85448629
assert!(
8545-
popup.contains("Guardian Approvals (current)"),
8630+
selected_permissions_popup_name(&popup) == "Guardian Approvals"
8631+
&& selected_permissions_popup_line(&popup).contains("(current)"),
85468632
"expected Guardian Approvals to be current after SessionConfigured sync: {popup}"
85478633
);
85488634
}
@@ -8597,7 +8683,8 @@ async fn permissions_selection_marks_guardian_approvals_current_with_custom_work
85978683
let popup = render_bottom_popup(&chat, 120);
85988684

85998685
assert!(
8600-
popup.contains("Guardian Approvals (current)"),
8686+
selected_permissions_popup_name(&popup) == "Guardian Approvals"
8687+
&& selected_permissions_popup_line(&popup).contains("(current)"),
86018688
"expected Guardian Approvals to be current even with custom workspace-write details: {popup}"
86028689
);
86038690
}
@@ -8622,9 +8709,22 @@ async fn permissions_selection_can_disable_guardian_approvals() {
86228709
.sandbox_policy
86238710
.set(SandboxPolicy::new_workspace_write_policy())
86248711
.expect("set sandbox policy");
8712+
chat.set_approvals_reviewer(ApprovalsReviewer::GuardianSubagent);
86258713

86268714
chat.open_permissions_popup();
8627-
chat.handle_key_event(KeyEvent::from(KeyCode::Up));
8715+
let popup = render_bottom_popup(&chat, 120);
8716+
assert!(
8717+
selected_permissions_popup_name(&popup) == "Guardian Approvals"
8718+
&& selected_permissions_popup_line(&popup).contains("(current)"),
8719+
"expected permissions popup to open with Guardian Approvals selected: {popup}"
8720+
);
8721+
8722+
move_permissions_popup_selection_to(&mut chat, "Default", KeyCode::Up);
8723+
let popup = render_bottom_popup(&chat, 120);
8724+
assert!(
8725+
selected_permissions_popup_name(&popup) == "Default",
8726+
"expected one Up from Guardian Approvals to select Default: {popup}"
8727+
);
86288728
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
86298729

86308730
let events = std::iter::from_fn(|| rx.try_recv().ok()).collect::<Vec<_>>();
@@ -8668,18 +8768,14 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context
86688768
chat.open_permissions_popup();
86698769
let popup = render_bottom_popup(&chat, 120);
86708770
assert!(
8671-
popup
8672-
.lines()
8673-
.any(|line| line.contains("(current)") && line.contains('›')),
8771+
selected_permissions_popup_line(&popup).contains("(current)"),
86748772
"expected permissions popup to open with the current preset selected: {popup}"
86758773
);
86768774

8677-
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
8775+
move_permissions_popup_selection_to(&mut chat, "Guardian Approvals", KeyCode::Down);
86788776
let popup = render_bottom_popup(&chat, 120);
86798777
assert!(
8680-
popup
8681-
.lines()
8682-
.any(|line| line.contains("Guardian Approvals") && line.contains('›')),
8778+
selected_permissions_popup_name(&popup) == "Guardian Approvals",
86838779
"expected one Down from Default to select Guardian Approvals: {popup}"
86848780
);
86858781
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
@@ -8720,9 +8816,7 @@ async fn permissions_full_access_history_cell_emitted_only_after_confirmation()
87208816
chat.config.notices.hide_full_access_warning = None;
87218817

87228818
chat.open_permissions_popup();
8723-
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
8724-
#[cfg(target_os = "windows")]
8725-
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
8819+
move_permissions_popup_selection_to(&mut chat, "Full Access", KeyCode::Down);
87268820
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
87278821

87288822
let mut open_confirmation_event = None;

0 commit comments

Comments
 (0)