Skip to content

Commit b0c0cb7

Browse files
committed
fix: address CodeRabbit review feedback
- Add missing 'Extracting' step name in step_manager_tests - Clarify cache test independence in challenge_repository_tests - Fix misleading comment about mistakes estimation in analytics_service_tests - Fix Challenge::new argument order (id, code_content) in session_manager_service_tests - Use specific 'Mistakes: 5' assertion instead of fragile '5' in sharing_tests - Assert buffer content changes after Esc in info_dialog fallback test - Replace silent failures with .expect() in screen_transition_manager helpers - Remove unused Colors import in theme_manager_tests
1 parent 3bd1aa8 commit b0c0cb7

File tree

8 files changed

+46
-27
lines changed

8 files changed

+46
-27
lines changed

tests/integration/screens/info_dialog_test.rs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,14 +287,15 @@ fn test_info_dialog_fallback_esc_returns_to_menu() {
287287
)) as Arc<dyn ThemeServiceInterface>,
288288
);
289289

290-
// Verify fallback renders without panic
290+
// Render fallback state
291291
let backend = TestBackend::new(120, 40);
292292
let mut terminal = Terminal::new(backend).unwrap();
293-
terminal
293+
let before = terminal
294294
.draw(|frame| {
295295
screen.render_ratatui(frame).unwrap();
296296
})
297297
.unwrap();
298+
let before_buffer = before.buffer.clone();
298299

299300
// Press Esc to return to Menu state
300301
screen
@@ -304,14 +305,20 @@ fn test_info_dialog_fallback_esc_returns_to_menu() {
304305
))
305306
.unwrap();
306307

307-
// Render again - should now be in Menu state
308+
// Render again - should now be in Menu state with different content
308309
let backend2 = TestBackend::new(120, 40);
309310
let mut terminal2 = Terminal::new(backend2).unwrap();
310-
terminal2
311+
let after = terminal2
311312
.draw(|frame| {
312313
screen.render_ratatui(frame).unwrap();
313314
})
314315
.unwrap();
316+
let after_buffer = after.buffer.clone();
317+
318+
assert_ne!(
319+
before_buffer, after_buffer,
320+
"Rendered content should change after Esc transitions from Fallback to Menu"
321+
);
315322
}
316323

317324
// Test: Fallback state - Ctrl+C exits

tests/unit/domain/models/loading/step_manager_tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ fn get_step_by_name_all_steps() {
8383
"Cloning repository",
8484
"Cache check",
8585
"Scanning repository",
86+
"Extracting functions, classes, and code blocks",
8687
"Generating challenges",
8788
"Finalizing",
8889
];

tests/unit/domain/repositories/challenge_repository_tests.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ fn test_save_then_get_cache_stats() {
165165

166166
#[test]
167167
fn test_save_then_clear_cache() {
168+
// Each test uses its own repository instance to avoid shared state
168169
let repo = create_repository();
169170
let git_repo = create_test_repo(Some("clear-test".to_string()), false);
170171
let challenges = vec![create_test_challenge("t1", "fn main() {}")];
@@ -178,6 +179,7 @@ fn test_save_then_clear_cache() {
178179

179180
#[test]
180181
fn test_save_then_list_cache_keys() {
182+
// Each test uses its own repository instance to avoid shared state
181183
let repo = create_repository();
182184
repo.clear_cache().unwrap();
183185

tests/unit/domain/services/analytics_service_tests.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,9 @@ fn test_analytics_best_cpm_tracked_across_sessions() {
445445
fn test_analytics_mistakes_estimation() {
446446
let mut mock = MockSessionRepo::new();
447447
mock.sessions = vec![make_session(1, None)];
448+
// Note: make_result sets mistakes=5 on SessionResultData, but AnalyticsService
449+
// computes total_mistakes from accuracy and stages_attempted, not from the
450+
// mistakes field: (100 - accuracy) / 100 * stages_attempted → cast to usize.
448451
// 90% accuracy with 3 stages attempted → (100-90)/100 * 3 = 0.3 → 0 as usize
449452
mock.results = vec![(1, make_result(200.0, 90.0, 10000))];
450453
mock.stage_results = vec![(1, vec![])];

tests/unit/domain/services/session_manager_service_tests.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -895,8 +895,8 @@ fn test_get_current_challenge_in_progress_with_challenges() {
895895

896896
// Populate challenge store with challenges
897897
let challenges = vec![
898-
Challenge::new("fn foo() {}".to_string(), "test.rs".to_string()),
899-
Challenge::new("fn bar() {}".to_string(), "test.rs".to_string()),
898+
Challenge::new("test-1".to_string(), "fn foo() {}".to_string()),
899+
Challenge::new("test-2".to_string(), "fn bar() {}".to_string()),
900900
];
901901
challenge_store.set_challenges(challenges);
902902

@@ -934,8 +934,8 @@ fn test_get_next_challenge_in_progress_with_challenges() {
934934
let challenge_store = Arc::new(ChallengeStore::new_for_test());
935935

936936
let challenges = vec![
937-
Challenge::new("fn foo() {}".to_string(), "test.rs".to_string()),
938-
Challenge::new("fn bar() {}".to_string(), "test.rs".to_string()),
937+
Challenge::new("test-1".to_string(), "fn foo() {}".to_string()),
938+
Challenge::new("test-2".to_string(), "fn bar() {}".to_string()),
939939
];
940940
challenge_store.set_challenges(challenges);
941941

tests/unit/domain/services/theme_manager_tests.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,6 @@ fn get_current_color_scheme_returns_light_scheme() {
129129

130130
#[test]
131131
fn get_colors_returns_valid_colors() {
132-
use gittype::presentation::ui::Colors;
133-
134132
let manager = create_theme_service();
135133
let colors = manager.get_colors();
136134
let _ = colors.border();

tests/unit/presentation/sharing_tests.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ fn create_share_text_without_repo() {
6363

6464
assert!(text.contains("150"), "should contain score");
6565
assert!(text.contains("300"), "should contain cpm");
66-
assert!(text.contains("5"), "should contain total mistakes (3+2)");
66+
assert!(
67+
text.contains("Mistakes: 5"),
68+
"should contain total mistakes (3+2)"
69+
);
6770
assert!(text.contains("#gittype"));
6871
assert!(text.contains("github.com/unhappychoice/gittype"));
6972
// Should NOT contain repo info

tests/unit/presentation/tui/screen_transition_manager_tests.rs

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -499,33 +499,38 @@ mod tests {
499499

500500
fn start_session(sm: &Arc<dyn SessionManagerInterface>) {
501501
// Start the game via Title→Typing transition
502-
let _ = ScreenTransitionManager::reduce(ScreenType::Title, ScreenType::Typing, sm);
502+
ScreenTransitionManager::reduce(ScreenType::Title, ScreenType::Typing, sm)
503+
.expect("Title→Typing transition should succeed");
503504
}
504505

505506
fn set_git_repository(sm: &Arc<dyn SessionManagerInterface>) {
506507
use gittype::domain::models::GitRepository;
507-
if let Some(concrete_sm) = sm.as_any().downcast_ref::<SessionManager>() {
508-
concrete_sm.set_git_repository(Some(GitRepository {
509-
user_name: "test".to_string(),
510-
repository_name: "repo".to_string(),
511-
remote_url: "https://github.com/test/repo".to_string(),
512-
branch: Some("main".to_string()),
513-
commit_hash: Some("abc123def456".to_string()),
514-
is_dirty: false,
515-
root_path: None,
516-
}));
517-
}
508+
let concrete_sm = sm
509+
.as_any()
510+
.downcast_ref::<SessionManager>()
511+
.expect("should downcast to SessionManager");
512+
concrete_sm.set_git_repository(Some(GitRepository {
513+
user_name: "test".to_string(),
514+
repository_name: "repo".to_string(),
515+
remote_url: "https://github.com/test/repo".to_string(),
516+
branch: Some("main".to_string()),
517+
commit_hash: Some("abc123def456".to_string()),
518+
is_dirty: false,
519+
root_path: None,
520+
}));
518521
}
519522

520523
fn start_session_with_tracker(sm: &Arc<dyn SessionManagerInterface>) {
521524
use gittype::domain::services::scoring::StageTracker;
522525

523526
start_session(sm);
524527
// Set up a stage tracker so handle_game_failure can use it
525-
if let Some(concrete_sm) = sm.as_any().downcast_ref::<SessionManager>() {
526-
let tracker = StageTracker::new("test challenge".to_string());
527-
concrete_sm.set_current_stage_tracker(tracker);
528-
}
528+
let concrete_sm = sm
529+
.as_any()
530+
.downcast_ref::<SessionManager>()
531+
.expect("should downcast to SessionManager");
532+
let tracker = StageTracker::new("test challenge".to_string());
533+
concrete_sm.set_current_stage_tracker(tracker);
529534
}
530535

531536
#[test]

0 commit comments

Comments
 (0)