Skip to content

fork: TUI test isolation fix (also open upstream as #1366) #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ianalitis wants to merge 1 commit into from
Closed

fork: TUI test isolation fix (also open upstream as #1366) #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 41 additions & 19 deletions crates/jcode-tui/src/tui/app/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,34 +1111,56 @@ pub(super) fn gather_git_info() -> Option<GitInfo> {

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();
}
Comment on lines +1124 to +1131

@greptile-apps greptile-apps Bot Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unreachable test cache branch

This block can never compile: the enclosing function has #[cfg(not(test))], so test builds exclude the whole function, while non-test builds exclude this inner block. The active #[cfg(test)] implementation at lines 1094–1103 already provides the read-only cache behavior. Keeping this duplicate dead code makes the explanation misleading and creates a second implementation that can drift from the real test path.

Suggested change
#[cfg(test)]
{
return GIT_INFO_CACHE
.lock()
.ok()
.and_then(|guard| guard.as_ref().map(|(_, cached, _)| cached.clone()))
.flatten();
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/jcode-tui/src/tui/app/helpers.rs
Line: 1124-1131

Comment:
**Unreachable test cache branch**

This block can never compile: the enclosing function has `#[cfg(not(test))]`, so test builds exclude the whole function, while non-test builds exclude this inner block. The active `#[cfg(test)]` implementation at lines 1094–1103 already provides the read-only cache behavior. Keeping this duplicate dead code makes the explanation misleading and creates a second implementation that can drift from the real test path.

```suggestion

```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.


#[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
Expand Down
12 changes: 11 additions & 1 deletion crates/jcode-tui/src/tui/app/tests/remote_events_reload_02/part_01.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Loading