From 211067fc45dfc05911920bc777692e2ece874d75 Mon Sep 17 00:00:00 2001 From: Iraklis Analitis Date: Mon, 21 Sep 2026 17:28:16 -0500 Subject: [PATCH] fix(tui): stop the live git probe leaking real repo state into tests `gather_git_info` spawns a background thread that shells out to `git` and writes the answer into a process-global cache. The first caller in a test binary sees `None`, but every test after it inherits the developer's real branch, ahead/behind and dirty counts, so frame assertions depended on how many tests ran before them and on whether the checkout was clean. `master` therefore reports different `jcode-tui --lib` failures on different machines and on different runs of the same machine. Tests now read that cache without ever spawning the probe, and `seed_git_info_cache_for_tests` remains the explicit way to opt in to git data for the tests that want it. This also narrows `test_background_task_markdown_is_suppressed_even_if_role_was_lost`, which asserted the frame contains no box-drawing character at all. Info-widget overlays legitimately draw their own bordered box in the right margin, and whether they are docked depends on how many frames the process has already rendered, so that assertion was order-dependent rather than a statement about the card under test. The card assertions themselves are unchanged. Measured on `master` (`cargo test -p jcode-tui --lib -- --test-threads=1`): 19 failed before, 17 failed after, with the remaining failures tracked separately (#1340, #1342). Two of the failures this removes are the onboarding "recent project" tests that fail or pass depending on the developer's git state. --- crates/jcode-tui/src/tui/app/helpers.rs | 60 +++++++++++++------ .../tests/remote_events_reload_02/part_01.rs | 12 +++- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/crates/jcode-tui/src/tui/app/helpers.rs b/crates/jcode-tui/src/tui/app/helpers.rs index 6730fad940..799b1ab8c9 100644 --- a/crates/jcode-tui/src/tui/app/helpers.rs +++ b/crates/jcode-tui/src/tui/app/helpers.rs @@ -1111,34 +1111,56 @@ pub(super) fn gather_git_info() -> Option { const TTL: Duration = Duration::from_secs(5); - if let Ok(mut guard) = GIT_INFO_CACHE.lock() { - if let Some((ts, cached, refreshing)) = guard.as_mut() { - if ts.elapsed() < TTL { - return cached.clone(); - } - if *refreshing { - return cached.clone(); + // Tests never probe the live repository. The probe runs on a background + // thread and writes its answer into this process-global cache, so the + // first test to call this gets `None` while every later test in the same + // binary silently inherits the developer's real branch and dirty counts. + // That made frame assertions depend on how many tests ran before them and + // on whether the checkout happened to be clean. + // + // Tests that want git data seed it explicitly with + // `seed_git_info_cache_for_tests`, which marks the entry `refreshing` and + // is honored by the read below. + #[cfg(test)] + { + return GIT_INFO_CACHE + .lock() + .ok() + .and_then(|guard| guard.as_ref().map(|(_, cached, _)| cached.clone())) + .flatten(); + } + + #[cfg(not(test))] + { + if let Ok(mut guard) = GIT_INFO_CACHE.lock() { + if let Some((ts, cached, refreshing)) = guard.as_mut() { + if ts.elapsed() < TTL { + return cached.clone(); + } + if *refreshing { + return cached.clone(); + } + let stale = cached.clone(); + *refreshing = true; + std::thread::spawn(|| { + let result = gather_git_info_inner(); + if let Ok(mut guard) = GIT_INFO_CACHE.lock() { + *guard = Some((Instant::now(), result, false)); + } + }); + return stale; } - let stale = cached.clone(); - *refreshing = true; + + *guard = Some((backdated_now(TTL + Duration::from_secs(1)), None, true)); std::thread::spawn(|| { let result = gather_git_info_inner(); if let Ok(mut guard) = GIT_INFO_CACHE.lock() { *guard = Some((Instant::now(), result, false)); } }); - return stale; } - - *guard = Some((backdated_now(TTL + Duration::from_secs(1)), None, true)); - std::thread::spawn(|| { - let result = gather_git_info_inner(); - if let Ok(mut guard) = GIT_INFO_CACHE.lock() { - *guard = Some((Instant::now(), result, false)); - } - }); + None } - None } /// Fetch a session's todos plus its goal-level assessments through the same diff --git a/crates/jcode-tui/src/tui/app/tests/remote_events_reload_02/part_01.rs b/crates/jcode-tui/src/tui/app/tests/remote_events_reload_02/part_01.rs index c36287d0a8..c5baa3f24b 100644 --- a/crates/jcode-tui/src/tui/app/tests/remote_events_reload_02/part_01.rs +++ b/crates/jcode-tui/src/tui/app/tests/remote_events_reload_02/part_01.rs @@ -892,7 +892,17 @@ fn test_background_task_markdown_is_suppressed_even_if_role_was_lost() { let mut terminal = ratatui::Terminal::new(backend).expect("failed to create test terminal"); let text = render_and_snap(&app, &mut terminal); - assert!(!text.contains("╭") && !text.contains("594967sj63")); + // The card itself must not render: no task id, no bordered tool card, and + // no fragment of the background-task markdown. + // + // Deliberately not asserting the frame has no box-drawing characters at + // all. Info-widget overlays (overview/context) legitimately draw their own + // bordered box in the right margin, and whether they are docked yet + // depends on how many frames the process has already rendered, which makes + // that assertion order-dependent rather than a statement about this test. + assert!(!text.contains("594967sj63"), "task id leaked:\n{text}"); + assert!(!text.contains("Background task"), "card leaked:\n{text}"); + assert!(!text.contains("Full output"), "card body leaked:\n{text}"); assert!(app.display_messages().is_empty()); assert_eq!(app.display_user_message_count(), 0); }