Repository navigation
Introduce Tester and simplify TUI integration tests - #197
Conversation
… buffer renderer + setup
…rame; extract draw_once; use helpers across tests
…ppTestExt (press/ctrl/key/draw_frame)
…TestExt; make handle_action pub; remove draw_once_on; adjust logger init; refactor tests to use extension trait
…; keep declarations ordered (internal helpers -> public utils -> trait -> impl)
…lTestExt with buffer_to_lines(); update tests to use method form
…t with assert_contains/assert_not_contains/assert_snapshot; hide buffer renderer; switch tests to new helpers; prune old snapshots and keep common-based ones
…s; update tests to use Tester for cleaner API
… inline setup/snapshot helpers into Tester; remove common/app_ext.rs and common/terminal_ext.rs
…eaders to tester.rs path
WalkthroughThis PR removes the test-utils feature, broadens App::handle_action visibility, adds App::context_mut, adjusts logger initialization to avoid dropping the logs table, and replaces Terminal-based in-process tests with a new Tester harness driving input, rendering, and assertions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Tester
participant A as App
participant C as Context
participant R as Renderer
rect rgba(220,235,255,0.5)
note over T: Input event
T->>T: Build KeyEvent (press/ctrl/key)
T->>A: context_mut()
A-->>T: &mut Context
T->>C: consume(input)
C-->>T: Action
T->>A: handle_action(Action, input)
A-->>T: bool (handled)
end
rect rgba(220,255,220,0.4)
note over T,R: Render
T->>R: Terminal::draw(App)
R->>A: draw frame
A-->>R: UI
R-->>T: Buffer
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
panarch
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tui/src/action.rs (1)
385-386: Modifier check breaks with combined modifiers (CTRL+SHIFT etc.).
key.modifiers == KeyModifiers::CONTROLfails if other modifiers are present. Use.contains(...).- let ctrl = key.modifiers == KeyModifiers::CONTROL; + let ctrl = key.modifiers.contains(KeyModifiers::CONTROL);
🧹 Nitpick comments (12)
tui/src/action.rs (2)
90-90: MakingApp::handle_actionpublic: consider marking test-only intent.If this isn’t a supported external API, add
#[doc(hidden)]to reduce accidental use. Alternative is a thin test-only wrapper, but doc-hiding is simplest.impl App { - pub async fn handle_action(&mut self, action: Action, input: Input) -> bool { + #[doc(hidden)] + pub async fn handle_action(&mut self, action: Action, input: Input) -> bool {
351-370: Silent drop whento_eventreturnsNone.For non-key inputs, you return
falsewithout trace. Add a debug log to aid triage.- None => { - return false; - } + None => { + log_debug!("PassThrough: ignored non-key input"); + return false; + }tui/src/app.rs (1)
110-112: Exposecontext_mutas test-only (doc-hidden) to protect invariants.Public mutable access can bypass invariants. If needed for tests, mark hidden.
- pub fn context_mut(&mut self) -> &mut Context { + #[doc(hidden)] + pub fn context_mut(&mut self) -> &mut Context { &mut self.context }tui/tests/entry_inproc.rs (2)
20-22: String-based assertion may be brittle.If UI copy changes, tests will false-fail. Consider centralizing UI strings as constants re-used by tests.
43-50: Help overlay assertions are straightforward; consider string constant reuse.tui/tests/notebook_inproc.rs (1)
90-92: Quit confirm accept path is minimal; consider asserting the confirm prompt first.Optional: assert the presence of the confirm dialog text before accepting.
tui/tests/tester.rs (6)
11-14: Consider keeping fields private to preserve harness invariants.Exposing
termandapplets tests bypass the API and reintroduce buffer-coupling. Prefer private fields with accessor helpers if needed.
61-66: Minor: consider returning the press result from open_instant().Returning whether
'1'was handled can help assertions.- pub async fn open_instant(&mut self) -> Result<()> { + pub async fn open_instant(&mut self) -> Result<bool> { self.draw()?; - let _ = self.press('1').await; + let handled = self.press('1').await; self.draw()?; - Ok(()) + Ok(handled) }
69-78: Improve assertion diagnostics.Include the current buffer in failure messages to speed up debugging.
pub fn assert_contains(&self, needle: &str) { let text = buffer_lines(&self.term).join("\n"); - assert!(text.contains(needle)); + assert!( + text.contains(needle), + "expected buffer to contain {:?}\n--- buffer ---\n{}", + needle, + text + ); } @@ pub fn assert_not_contains(&self, needle: &str) { let text = buffer_lines(&self.term).join("\n"); - assert!(!text.contains(needle)); + assert!( + !text.contains(needle), + "expected buffer NOT to contain {:?}\n--- buffer ---\n{}", + needle, + text + ); }
81-84: Make snapshots more readable and stable.Joining lines into a single string (optionally trimming trailing spaces) makes diffs easier to scan.
pub fn assert_snapshot(&self, name: &str) { - let lines = buffer_lines(&self.term); - insta::assert_debug_snapshot!(name, lines); + let text = buffer_lines(&self.term).join("\n"); + insta::assert_snapshot!(name, text); }
86-106: CTRL+C is swallowed by the harness; verify intended semantics.Returning
truewithout notifyingAppmeans the app never sees Ctrl+C. IfApphandles quit/interrupt internally, route it there.Option: forward it to
consume/handle_actionor call the app’s quit action explicitly if one exists.
111-123: Trim trailing spaces per line for snapshot stability.Ratatui buffers are space-padded; trimming reduces noisy diffs without losing content.
fn buffer_lines(term: &Terminal<TestBackend>) -> Vec<String> { @@ - for y in 0..area.height { + for y in 0..area.height { let mut line = String::new(); for x in 0..area.width { line.push_str(buf[(x, y)].symbol()); } - lines.push(line); + lines.push(line.trim_end_matches(' ').to_string()); } lines }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tui/tests/snapshots/flow_inproc__tester__home_inproc.snapis excluded by!**/*.snaptui/tests/snapshots/flow_inproc__tester__instant_inproc.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
tui/Cargo.toml(1 hunks)tui/src/action.rs(1 hunks)tui/src/app.rs(1 hunks)tui/src/logger.rs(0 hunks)tui/tests/entry_inproc.rs(1 hunks)tui/tests/flow_inproc.rs(1 hunks)tui/tests/notebook_inproc.rs(1 hunks)tui/tests/tester.rs(1 hunks)
💤 Files with no reviewable changes (1)
- tui/src/logger.rs
🧰 Additional context used
🧬 Code graph analysis (3)
tui/tests/flow_inproc.rs (2)
tui/tests/tester.rs (1)
new(17-30)tui/src/app.rs (1)
new(20-25)
tui/tests/entry_inproc.rs (1)
tui/tests/tester.rs (1)
new(17-30)
tui/tests/notebook_inproc.rs (2)
tui/tests/tester.rs (1)
new(17-30)tui/src/app.rs (1)
new(20-25)
🔇 Additional comments (11)
tui/tests/flow_inproc.rs (3)
11-13: Snapshots usage looks good and deterministic with fixed backend size.
15-17: Flow to Instant + snapshot reads clearly.
19-20: Ctrl+C quit assertion is clear and minimal.tui/tests/entry_inproc.rs (2)
15-18: Navigation + Enter path reads cleanly.
31-32: Quit-with-qpath assertion looks good.tui/tests/notebook_inproc.rs (3)
13-19: Open note flow is concise and verifies expected content.
32-39: Note Actions dialog open/close checks are solid.
50-57: Directory Actions dialog open/close checks are solid.tui/Cargo.toml (1)
31-31: Feature removal looks fine — notest-utilsreferences foundSearched the repo (Cargo.toml and source files) for 'test-utils' / 'test_utils' with ripgrep/grep and found no matches; confirm CI and feature-enabled builds pass.
tui/tests/tester.rs (2)
32-35: Render wrapper looks good.Encapsulates drawing via the App correctly.
37-43: Input helpers are clean and focused.Nice thin wrappers over
handle_input.Also applies to: 45-52, 55-58
| mod tester; | ||
| use tester::Tester; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tester harness: avoid unsafe HOME mutation; prefer temp HOME per test.
In tester.rs, std::env::set_var isn’t unsafe—drop the unsafe block and consider a temp dir to isolate runs.
Example (tester.rs):
let tmp = tempfile::tempdir()?;
std::env::set_var("HOME", tmp.path()); // no unsafe
// store tmp in Tester to keep it alive for test duration🤖 Prompt for AI Agents
In tui/tests/flow_inproc.rs around lines 1 to 2, the tester harness uses an
unsafe block to mutate HOME in tester.rs; remove the unnecessary unsafe wrapper
around std::env::set_var and instead create a temporary directory
(tempfile::tempdir()) and set HOME to that path, storing the TempDir inside the
Tester struct so it stays alive for the test duration; update tester.rs to
import tempfile, add a field like tmp_home: tempfile::TempDir to Tester, assign
it when constructing the Tester, call std::env::set_var("HOME",
tmp_home.path()), and drop the unsafe block.
| t.press('?').await; | ||
| t.draw()?; | ||
| t.assert_contains(" [?] Hide keymap "); | ||
|
|
||
| // hide keymap | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('?'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
| app.draw_once_on(&mut term)?; | ||
| let buf = buffer_to_lines(&term).join("\n"); | ||
| assert!(!buf.contains(" [?] Hide keymap ")); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn notebook_editor_keymap_in_insert_mode_inproc() -> Result<()> { | ||
| let (mut app, mut term) = setup().await?; | ||
| open_instant(&mut app, &mut term).await?; | ||
|
|
||
| // open note and enter insert mode | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('j'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('l'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('i'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
|
|
||
| // show editor keymap with Ctrl+h | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('h'), | ||
| KeyModifiers::CONTROL, | ||
| ))) | ||
| .await; | ||
| app.draw_once_on(&mut term)?; | ||
| let buf = buffer_to_lines(&term).join("\n"); | ||
| assert!(buf.contains("Editor Keymap")); | ||
|
|
||
| // any key closes | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('x'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
| app.draw_once_on(&mut term)?; | ||
| let buf = buffer_to_lines(&term).join("\n"); | ||
| assert!(!buf.contains("Editor Keymap")); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn notebook_quit_confirm_then_cancel_inproc() -> Result<()> { | ||
| let (mut app, mut term) = setup().await?; | ||
| open_instant(&mut app, &mut term).await?; | ||
|
|
||
| // open note in normal mode (idle) | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('j'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
| app.handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('l'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
|
|
||
| // Esc opens quit confirmation | ||
| app.handle_input(Input::Key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE))) | ||
| .await; | ||
| app.draw_once_on(&mut term)?; | ||
| let buf = buffer_to_lines(&term).join("\n"); | ||
| assert!(buf.contains("Do you want to quit?")); | ||
|
|
||
| // cancel with 'n' (should not quit) | ||
| let quit = app | ||
| .handle_input(Input::Key(KeyEvent::new( | ||
| KeyCode::Char('n'), | ||
| KeyModifiers::NONE, | ||
| ))) | ||
| .await; | ||
| assert!(!quit); | ||
|
|
||
| app.draw_once_on(&mut term)?; | ||
| let buf = buffer_to_lines(&term).join("\n"); | ||
| assert!(!buf.contains("Do you want to quit?")); | ||
| t.press('?').await; | ||
| t.draw()?; | ||
| t.assert_not_contains("Do you want to quit?"); | ||
|
|
There was a problem hiding this comment.
Assertion mismatch: keymap hide check validates unrelated text.
After toggling off, asserting not to see “Do you want to quit?” doesn’t validate keymap state. Assert keymap text instead.
- t.press('?').await;
- t.draw()?;
- t.assert_not_contains("Do you want to quit?");
+ t.press('?').await;
+ t.draw()?;
+ // Either confirm the hide label disappeared...
+ t.assert_not_contains(" [?] Hide keymap ");
+ // ...or confirm the show label is visible:
+ // t.assert_contains(" [?] Show keymap ");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.press('?').await; | |
| t.draw()?; | |
| t.assert_contains(" [?] Hide keymap "); | |
| // hide keymap | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('?'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| app.draw_once_on(&mut term)?; | |
| let buf = buffer_to_lines(&term).join("\n"); | |
| assert!(!buf.contains(" [?] Hide keymap ")); | |
| Ok(()) | |
| } | |
| #[tokio::test] | |
| async fn notebook_editor_keymap_in_insert_mode_inproc() -> Result<()> { | |
| let (mut app, mut term) = setup().await?; | |
| open_instant(&mut app, &mut term).await?; | |
| // open note and enter insert mode | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('j'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('l'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('i'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| // show editor keymap with Ctrl+h | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('h'), | |
| KeyModifiers::CONTROL, | |
| ))) | |
| .await; | |
| app.draw_once_on(&mut term)?; | |
| let buf = buffer_to_lines(&term).join("\n"); | |
| assert!(buf.contains("Editor Keymap")); | |
| // any key closes | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('x'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| app.draw_once_on(&mut term)?; | |
| let buf = buffer_to_lines(&term).join("\n"); | |
| assert!(!buf.contains("Editor Keymap")); | |
| Ok(()) | |
| } | |
| #[tokio::test] | |
| async fn notebook_quit_confirm_then_cancel_inproc() -> Result<()> { | |
| let (mut app, mut term) = setup().await?; | |
| open_instant(&mut app, &mut term).await?; | |
| // open note in normal mode (idle) | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('j'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| app.handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('l'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| // Esc opens quit confirmation | |
| app.handle_input(Input::Key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE))) | |
| .await; | |
| app.draw_once_on(&mut term)?; | |
| let buf = buffer_to_lines(&term).join("\n"); | |
| assert!(buf.contains("Do you want to quit?")); | |
| // cancel with 'n' (should not quit) | |
| let quit = app | |
| .handle_input(Input::Key(KeyEvent::new( | |
| KeyCode::Char('n'), | |
| KeyModifiers::NONE, | |
| ))) | |
| .await; | |
| assert!(!quit); | |
| app.draw_once_on(&mut term)?; | |
| let buf = buffer_to_lines(&term).join("\n"); | |
| assert!(!buf.contains("Do you want to quit?")); | |
| t.press('?').await; | |
| t.draw()?; | |
| t.assert_not_contains("Do you want to quit?"); | |
| t.press('?').await; | |
| t.draw()?; | |
| t.assert_contains(" [?] Hide keymap "); | |
| // hide keymap | |
| t.press('?').await; | |
| t.draw()?; | |
| // Either confirm the hide label disappeared... | |
| t.assert_not_contains(" [?] Hide keymap "); | |
| // ...or confirm the show label is visible: | |
| // t.assert_contains(" [?] Show keymap "); |
🤖 Prompt for AI Agents
In tui/tests/notebook_inproc.rs around lines 68 to 76, the post-toggle assertion
checks for "Do you want to quit?" which is unrelated to the keymap visibility;
replace that assertion with one that verifies the keymap text is gone (for
example assert_not_contains(" [?] Hide keymap ") or another specific keymap
string used earlier) so the test actually validates that the keymap was hidden
after pressing '?'.
| let cwd = std::env::current_dir()?; | ||
| std::fs::create_dir_all(cwd.join(".glues"))?; | ||
| unsafe { | ||
| std::env::set_var("HOME", &cwd); | ||
| } | ||
| config::init().await; | ||
| logger::init().await; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate unnecessary unsafe and isolate HOME per test; restore on Drop to avoid cross-test interference.
std::env::set_varis safe; theunsafeblock is unnecessary and will trigger theunused_unsafelint.- Mutating the global
HOMEand writing.gluesunder CWD can cause flaky tests when run in parallel and pollute the repo. Use a per-test temp HOME and restore the original on drop.
Apply this diff to make HOME sandboxed and restored automatically:
+use std::ffi::OsString;
+use tempfile::TempDir;
@@
pub struct Tester {
pub app: App,
pub term: Terminal<TestBackend>,
+ // internal: preserve original HOME and keep the temp dir alive
+ original_home: Option<OsString>,
+ _temp_home: TempDir,
}
@@
pub async fn new() -> Result<Self> {
- let cwd = std::env::current_dir()?;
- std::fs::create_dir_all(cwd.join(".glues"))?;
- unsafe {
- std::env::set_var("HOME", &cwd);
- }
+ let original_home = std::env::var_os("HOME");
+ let temp_home = tempfile::tempdir()?;
+ let home = temp_home.path().to_path_buf();
+ std::fs::create_dir_all(home.join(".glues"))?;
+ std::env::set_var("HOME", &home);
config::init().await;
logger::init().await;
let backend = TestBackend::new(120, 40);
let term = Terminal::new(backend)?;
let app = App::new();
- Ok(Self { app, term })
+ Ok(Self { app, term, original_home, _temp_home: temp_home })
}
+
+impl Drop for Tester {
+ fn drop(&mut self) {
+ if let Some(ref old) = self.original_home {
+ std::env::set_var("HOME", old);
+ } else {
+ std::env::remove_var("HOME");
+ }
+ // temp dir auto-cleans on Drop
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cwd = std::env::current_dir()?; | |
| std::fs::create_dir_all(cwd.join(".glues"))?; | |
| unsafe { | |
| std::env::set_var("HOME", &cwd); | |
| } | |
| config::init().await; | |
| logger::init().await; | |
| use std::ffi::OsString; | |
| use tempfile::TempDir; | |
| pub struct Tester { | |
| pub app: App, | |
| pub term: Terminal<TestBackend>, | |
| // internal: preserve original HOME and keep the temp dir alive | |
| original_home: Option<OsString>, | |
| _temp_home: TempDir, | |
| } | |
| impl Tester { | |
| pub async fn new() -> Result<Self> { | |
| let original_home = std::env::var_os("HOME"); | |
| let temp_home = tempfile::tempdir()?; | |
| let home = temp_home.path().to_path_buf(); | |
| std::fs::create_dir_all(home.join(".glues"))?; | |
| std::env::set_var("HOME", &home); | |
| config::init().await; | |
| logger::init().await; | |
| let backend = TestBackend::new(120, 40); | |
| let term = Terminal::new(backend)?; | |
| let app = App::new(); | |
| Ok(Self { app, term, original_home, _temp_home: temp_home }) | |
| } | |
| } | |
| impl Drop for Tester { | |
| fn drop(&mut self) { | |
| if let Some(ref old) = self.original_home { | |
| std::env::set_var("HOME", old); | |
| } else { | |
| std::env::remove_var("HOME"); | |
| } | |
| // temp dir auto-cleans on Drop | |
| } | |
| } |
🤖 Prompt for AI Agents
In tui/tests/tester.rs around lines 18 to 25, remove the unnecessary unsafe
block around std::env::set_var and sandbox HOME to a per-test temporary
directory that is cleaned up and restored when the test ends; create the .glues
directory under that temp HOME rather than the repo CWD, and implement a small
RAII helper that captures the previous HOME (if any) and on Drop restores the
original value (or removes the variable if it was not set) so parallel tests
won’t interfere or pollute the working tree.
| config::init().await; | ||
| logger::init().await; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm idempotence and error handling of config::init()/logger::init().
If these set global state (common for loggers), repeated calls across parallel tests can panic or silently no-op. Also, propagate errors if they return Result.
Run this to inspect signatures and potential global initialization:
🏁 Script executed:
#!/bin/bash
# Look for init() definitions and global logger setup
rg -nP -C3 '(async\s+)?fn\s+init\s*\(' -g '!**/target/**'
rg -nP -C3 'tracing(_subscriber)?::.*(init|set_global_default)' -g '!**/target/**'
# Check call sites to ensure they won't race
rg -nP -C2 '\b(config|logger)::init\s*\(' -g '!**/target/**'Length of output: 446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "=== top-level listing ==="
ls -la | sed -n '1,200p' || true
echo
echo "=== search: fn init definitions ==="
rg -nP -C3 "(async\s+)?fn\s+init\s*\(" --hidden --no-ignore-vcs || true
echo
echo "=== search: tracing subscriber init / set_global_default ==="
rg -nP -C3 "tracing(_subscriber)?::.*(init|set_global_default)" --hidden --no-ignore-vcs || true
echo
echo "=== search: call sites for config::init or logger::init ==="
rg -nP -C3 "\b(config|logger)::init\s*\(" --hidden --no-ignore-vcs || true
echo
echo "=== show file: tui/tests/tester.rs (if present) ==="
if [ -f "tui/tests/tester.rs" ]; then
sed -n '1,240p' tui/tests/tester.rs
else
echo "tui/tests/tester.rs not found"
fi
echo
echo "=== locate config.rs / logger.rs files ==="
find . -type f \( -name 'config.rs' -o -name 'logger.rs' \) -print || trueLength of output: 5708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "=== tui/src/config.rs ==="
nl -ba tui/src/config.rs | sed -n '1,240p' || true
echo
echo "=== tui/src/logger.rs ==="
nl -ba tui/src/logger.rs | sed -n '1,240p' || true
echo
echo "=== search for Once/OnceCell/OnceLock/lazy_static/static mut in tui/src ==="
rg -n --hidden --no-ignore-vcs '\b(OnceCell|OnceLock|Once::|lazy_static|static mut|Once)\b' tui/src || true
echo
echo "=== search for tracing/log/env_logger/tracing_subscriber usage in tui/src ==="
rg -n --hidden --no-ignore-vcs 'tracing|tracing_subscriber|set_global_default|env_logger|log::|tracing::' tui/src || trueLength of output: 510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "=== tui/src/config.rs (with line numbers) ==="
awk '{printf "%4d: %s\n", NR, $0}' tui/src/config.rs || true
echo
echo "=== tui/src/logger.rs (with line numbers) ==="
awk '{printf "%4d: %s\n", NR, $0}' tui/src/logger.rs || true
echo
echo "=== scan repo for global-init / OnceCell / OnceLock / static mut / set_global_default usage ==="
rg -n --hidden --no-ignore-vcs '\b(OnceCell|OnceLock|std::sync::Once|static mut|set_global_default|tracing_subscriber::|tracing::subscriber|env_logger::|log::)\b' || true
echo
echo "=== show Cargo.toml deps (relevant logging crates) ==="
sed -n '1,240p' Cargo.toml || trueLength of output: 5779
Make init() idempotent, return Result, and avoid global env races
- These inits do NOT set a global tracing/log subscriber (no tracing_subscriber usage found), so global-subscriber panic risk is not present.
- Problem: tui/src/config.rs:get_glue() (lines 22–29), config::init() (31–59) and tui/src/logger.rs:init()/log() (7–29) call CsvStorage::new(...).unwrap() and .execute(...).await.unwrap(), causing panics instead of propagating errors. Change signatures to return a Result (e.g., pub async fn init() -> color_eyre::Result<()>) and propagate/handle errors.
- Idempotence: create_table_if_not_exists is already idempotent, but the default-row inserts in config::init() currently swallow errors (let _ = …). Make inserts explicitly idempotent (INSERT OR IGNORE / check-exists-before-insert) or handle duplicate-key errors.
- Tests race: tui/tests/tester.rs (around lines 20–24) calls std::env::set_var("HOME", …) which mutates global ENV and will race when tests run in parallel. Use a per-test tempdir and inject the path into get_glue (or a test-only GLUES_DIR), or serialize initialization.
- Recommendation: convert init() to return Result, remove unwraps, and either accept an injectable storage path (for tests) or guard one-time initialization with OnceCell/OnceLock if a single global init is intended.
🤖 Prompt for AI Agents
In tui/tests/tester.rs around lines 23–24 and related files (tui/src/config.rs
lines ~22–59 and tui/src/logger.rs lines ~7–29): change the init() functions to
return a Result (e.g., pub async fn init() -> color_eyre::Result<()>), remove
all .unwrap() calls and propagate errors instead, make default-row inserts
idempotent by using INSERT OR IGNORE or checking existence before inserting (or
explicitly handle duplicate-key errors), and avoid global env races in tests by
injecting a per-test storage path (or a test-only GLUES_DIR) or serializing
initialization with OnceCell/OnceLock so tests don’t call
std::env::set_var("HOME") globally; ensure CsvStorage::new and .execute calls
return/propagate errors rather than panicking.
This PR refactors the TUI integration test infrastructure:
Highlights
Why
Notes
Summary by CodeRabbit