Refactor/di cleanup and test coverage

[eannchen]

Description

• Improve codebase testability by removing non-deterministic behavior and increase test coverage across all packages.
• Replace log.Fatalf with error return in logger.Init() for graceful error handling
• Inject Rand interface into scheduler for deterministic testing (fixes flaky tests)

Type of Change

• New feature
• Bug fix
• Refactor
• Documentation update
• Build/CI-related change
• Other (specify):

Checklist

• My code follows the project's style.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• All new and existing unit tests pass locally with my changes.
• I have manually tested the new functionality in the CLI.

Summary by CodeRabbit

• New Features
  • Added Settings, Version, Migrate, and Reset commands for managing configuration and app data.
• Improvements
  • History timestamps now show human-readable relative times.
  • History rendering supports multi-platform problem URLs.
• Tests
  • Expanded deterministic tests across scheduler, clock, IO, config, storage, use cases, and logging.
• Chores
  • Simplified application logging/init flow and made scheduler randomness testable.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Refactors logging to package-level loggers, removes logger dependencies from handlers and use cases, introduces testable Rand and MockClock abstractions, adds IOHandler.FormatTimeAgo, improves URL parsing, and expands deterministic tests across core, handler, internal, and storage packages.

Changes

Cohort / File(s)	Summary
Logging System Overhaul internal/logger/logger.go, internal/logger/logger_test.go, main.go	Replace per-instance Logger with package-level loggers; add Init/InitNop, Infof/Errorf; update tests and main to initialize logger and handle Init errors; remove logger parameters from constructors/callers.
Testable RNG & Scheduler core/scheduler.go, core/scheduler_test.go, usecase/usecase_integration_test.go, usecase/usecase_test.go	Add Rand interface, DefaultRand, FixedRand; expose SM2Scheduler.Rand and NewSM2SchedulerWithRand; replace package rand usage with injected Rand; update tests and use cases to use FixedRand for determinism.
Mock Clock & Deterministic Time internal/clock/clock.go, internal/clock/clock_test.go, handler/io_test.go, storage/file_test.go	Add MockClock with Now/Today/ToDate/AddDays and tests; replace time.Now() in many tests with fixed mock clock for deterministic expectations.
Handler & IO Enhancements handler/handler.go, handler/handler_test.go, handler/io.go, handler/io_test.go	Remove logger param from NewHandler; delegate time formatting to IOHandler.FormatTimeAgo; implement FormatTimeAgo on IOHandlerImpl; update history URL parsing to multi-platform parser; broaden deterministic IO tests.
Use Case Layer Updates usecase/usecase.go, usecase/usecase_test.go, usecase/usecase_integration_test.go	Remove per-instance logger from QuestionUseCase; update NewQuestionUseCase signature to drop logger; switch internal logging to package-level logger.Infof/Errorf; adapt tests for deterministic clock and FixedRand.
Command & Settings Tests command/command_test.go, config/config_test.go	Extend command mock to track Setting/Version/Migrate/Reset handler calls; add tests for new commands; add settings registry/value/info tests.
Model, Errors, FileUtil, URLParser Tests core/model_test.go, internal/errs/codederr_test.go, internal/fileutil/fileutil_test.go, internal/urlparser/urlparser_test.go	Add/extend tests for enum string representations, CodedError branches, JSON file Delete behavior, and URL parse error cases.
Storage Tests storage/file_test.go	Make time deterministic in tests and add TestFileStorage_DeleteAllData to verify deletion of questions and deltas.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• Refactor/di cleanup and test coverage #16 — Similar refactor: introduces Rand/FixedRand and NewSM2SchedulerWithRand, replaces logger.NewLogger with logger.Init/InitNop, and removes logger parameters from constructors; likely the same or closely related changes.

Poem

I nibble logs and fix the clock,
I stash randomness in a lock,
Handlers freed, constructors slim,
Tests now pass on every whim—
A rabbit's hop makes builds tick-tock. 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.40% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title is partially related to the changeset. It refers to real aspects of the PR (refactoring, dependency injection cleanup, and test coverage expansion), but is overly broad and uses vague phrasing ('di cleanup') that doesn't convey the specific, main changes to a teammate scanning history.	Consider a more specific title that captures the primary changes, such as 'Remove logger dependencies and add testability abstractions' or 'Refactor logging and scheduler for deterministic testing'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bdca97 and 9c2ce1e.

📒 Files selected for processing (1)

• handler/handler.go

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (8)

handler/io.go (1)

222-248: Consider handling future timestamps.

The implementation correctly formats relative time strings with proper singular/plural handling. However, if t is in the future (i.e., diff < 0), the current logic would produce unexpected results like negative minutes.

Consider adding a check at the beginning:

if diff < 0 {
    return "in the future"
}

This would make the behavior more predictable for edge cases.

core/scheduler_test.go (1)

10-33: Consider using the shared MockClock from internal/clock.

Based on the relevant code snippets, internal/clock/clock.go already provides NewMockClock. This local MockClock duplicates that implementation. Consider importing and using clock.NewMockClock instead to reduce duplication and ensure consistency.

However, if this is intentional to avoid circular dependencies or keep test utilities self-contained within the package, this is acceptable.

internal/logger/logger_test.go (5)

9-26: Init return value should be checked.

Per the relevant code snippet, Init returns an error. The test ignores this return value. While the test passes implicitly (file creation succeeds), explicitly checking the error would make the test more robust and serve as documentation.

🔎 Proposed fix

 	// Should not panic
-	Init(infoPath, errorPath)
+	if err := Init(infoPath, errorPath); err != nil {
+		t.Fatalf("Init failed: %v", err)
+	}

---

45-66: Check Init return value.

Same issue as TestInit - the error return from Init should be checked.

🔎 Proposed fix

-	Init(infoPath, errorPath)
+	if err := Init(infoPath, errorPath); err != nil {
+		t.Fatalf("Init failed: %v", err)
+	}

---

68-89: Check Init return value.

Same issue - the error return from Init should be checked.

🔎 Proposed fix

-	Init(infoPath, errorPath)
+	if err := Init(infoPath, errorPath); err != nil {
+		t.Fatalf("Init failed: %v", err)
+	}

---

91-99: Test mutates package-level state without restoration.

This test sets infoLogger and errorLogger to nil, which could interfere with other tests if run in parallel or in a specific order. Consider restoring the original state after the test.

🔎 Proposed fix

 func TestInfof_BeforeInit(t *testing.T) {
+	// Save original state
+	origInfo := infoLogger
+	origError := errorLogger
+	defer func() {
+		infoLogger = origInfo
+		errorLogger = origError
+	}()
+
 	// Reset loggers to nil
 	infoLogger = nil
 	errorLogger = nil
 
 	// Should not panic when loggers are nil
 	Infof("test message")
 	Errorf("test error")
 }

---

101-122: Check Init return value and consider using sync.WaitGroup.

The Init error should be checked. Additionally, sync.WaitGroup is more idiomatic for waiting on goroutines than a channel-based approach.

🔎 Proposed fix

+import "sync"
+
 func TestConcurrentLogging(t *testing.T) {
 	infoPath := filepath.Join(os.TempDir(), "test_concurrent_info.log")
 	errorPath := filepath.Join(os.TempDir(), "test_concurrent_error.log")
 
 	defer os.Remove(infoPath)
 	defer os.Remove(errorPath)
 
-	Init(infoPath, errorPath)
+	if err := Init(infoPath, errorPath); err != nil {
+		t.Fatalf("Init failed: %v", err)
+	}
 
-	done := make(chan bool)
+	var wg sync.WaitGroup
 	for i := 0; i < 10; i++ {
+		wg.Add(1)
 		go func(n int) {
+			defer wg.Done()
 			Infof("concurrent info message %d", n)
 			Errorf("concurrent error message %d", n)
-			done <- true
 		}(i)
 	}
 
-	for i := 0; i < 10; i++ {
-		<-done
-	}
+	wg.Wait()
 }

usecase/usecase_integration_test.go (1)

14-27: Consider extracting shared test time constant.

Both usecase_test.go and usecase_integration_test.go define nearly identical fixed times (testTime and integrationTestTime). While using the same value is good for consistency, you might consider extracting this to a shared test utilities file to ensure they stay in sync.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf6eaa8 and 2bdca97.

📒 Files selected for processing (21)

• command/command_test.go
• config/config_test.go
• core/model_test.go
• core/scheduler.go
• core/scheduler_test.go
• handler/handler.go
• handler/handler_test.go
• handler/io.go
• handler/io_test.go
• internal/clock/clock.go
• internal/clock/clock_test.go
• internal/errs/codederr_test.go
• internal/fileutil/fileutil_test.go
• internal/logger/logger.go
• internal/logger/logger_test.go
• internal/urlparser/urlparser_test.go
• main.go
• storage/file_test.go
• usecase/usecase.go
• usecase/usecase_integration_test.go
• usecase/usecase_test.go

🧰 Additional context used

🧬 Code graph analysis (18)

internal/fileutil/fileutil_test.go (1)

internal/fileutil/fileutil.go (1)

• NewJSONFileUtil (23-25)

internal/urlparser/urlparser_test.go (1)

internal/errs/errors.go (1)

• ErrInvalidURLFormat (16-16)

internal/errs/codederr_test.go (1)

internal/errs/codederr.go (4)

• CodedError (11-16)
• SystemErrorKind (8-8)
• BusinessErrorKind (7-7)
• ValidationErrorKind (6-6)

config/config_test.go (3)

config/test_utils.go (1)

• MockFileUtil (9-13)

config/config.go (1)

• NewConfig (223-247)

internal/logger/logger.go (1)

• Errorf (54-58)

handler/io.go (1)

internal/clock/clock.go (1)

• Clock (8-13)

command/command_test.go (2)

command/command.go (5)

• Command (11-13)
• SettingCommand (160-162)
• VersionCommand (169-171)
• MigrateCommand (178-180)
• ResetCommand (187-189)

handler/handler.go (1)

• Handler (19-36)

handler/handler.go (2)

handler/io.go (1)

• IOHandler (44-57)

internal/urlparser/urlparser.go (1)

• Parse (58-88)

internal/clock/clock_test.go (1)

internal/clock/clock.go (2)

• NewMockClock (44-46)
• Clock (8-13)

usecase/usecase.go (5)

storage/file.go (1)

• Storage (10-16)

core/scheduler.go (1)

• Scheduler (12-16)

internal/clock/clock.go (1)

• Clock (8-13)

internal/logger/logger.go (2)

• Infof (47-51)
• Errorf (54-58)

core/model.go (1)

• Question (63-75)

internal/logger/logger_test.go (1)

internal/logger/logger.go (4)

• Init (19-38)
• InitNop (41-44)
• Infof (47-51)
• Errorf (54-58)

usecase/usecase_integration_test.go (5)

usecase/usecase.go (2)

• QuestionUseCaseImpl (39-44)
• NewQuestionUseCase (47-54)

internal/clock/clock.go (1)

• NewMockClock (44-46)

storage/file.go (1)

• NewFileStorage (18-24)

core/scheduler.go (1)

• NewSM2SchedulerWithRand (67-122)

internal/logger/logger.go (1)

• InitNop (41-44)

core/model_test.go (1)

core/model.go (7)

• Platform (37-37)
• PlatformLeetCode (40-40)
• PlatformHackerRank (41-41)
• ActionType (78-78)
• ActionAdd (81-81)
• ActionUpdate (82-82)
• ActionDelete (83-83)

core/scheduler.go (2)

config/config.go (1)

• Config (204-221)

internal/clock/clock.go (1)

• Clock (8-13)

handler/io_test.go (6)

core/scheduler_test.go (1)

• NewMockClock (15-17)

handler/io.go (2)

• IOHandlerImpl (59-63)
• NewIOHandler (65-71)

internal/clock/clock.go (1)

• Clock (8-13)

internal/logger/logger.go (1)

• Errorf (54-58)

core/model.go (7)

• Question (63-75)
• Familiarity (18-18)
• Medium (23-23)
• Importance (9-9)
• HighImportance (14-14)
• MediumImportance (13-13)
• Easy (24-24)

internal/errs/codederr.go (1)

• CodedError (11-16)

usecase/usecase_test.go (6)

config/test_utils.go (3)

• TestConfig (28-47)
• MockEnv (50-138)
• MockFileUtil (9-13)

usecase/usecase.go (2)

• QuestionUseCaseImpl (39-44)
• NewQuestionUseCase (47-54)

internal/clock/clock.go (1)

• NewMockClock (44-46)

storage/file.go (2)

• NewFileStorage (18-24)
• Storage (10-16)

core/scheduler.go (2)

• NewSM2SchedulerWithRand (67-122)
• FixedRand (30-32)

internal/logger/logger.go (1)

• InitNop (41-44)

core/scheduler_test.go (1)

core/scheduler.go (1)

• Scheduler (12-16)

handler/handler_test.go (4)

internal/logger/logger.go (2)

• InitNop (41-44)
• Errorf (54-58)

handler/handler.go (1)

• NewHandler (45-52)

core/model.go (3)

• Question (63-75)
• Familiarity (18-18)
• Importance (9-9)

config/test_utils.go (1)

• MockEnv (50-138)

main.go (6)

internal/logger/logger.go (1)

• Init (19-38)

storage/file.go (1)

• NewFileStorage (18-24)

core/scheduler.go (1)

• NewSM2Scheduler (63-65)

usecase/usecase.go (1)

• NewQuestionUseCase (47-54)

handler/io.go (1)

• NewIOHandler (65-71)

handler/handler.go (1)

• NewHandler (45-52)

🔇 Additional comments (67)

core/model_test.go (3)

5-22: LGTM! Well-structured table-driven test.

The test provides good coverage of the Platform.String() method, including both known platforms and the edge case of an unknown platform value. The table-driven approach and use of subtests follow Go best practices.

---

24-42: LGTM! Comprehensive enum testing.

The test thoroughly covers ActionType.String() behavior, including the edge case where unknown action types return an empty string. The consistent table-driven structure makes the tests easy to maintain.

---

44-62: LGTM! Solid coverage of past tense formatting.

The test provides complete coverage of ActionType.PastTenseString(), including proper handling of unknown action types. The test structure is consistent with the rest of the file, making the test suite cohesive and maintainable.

internal/errs/codederr_test.go (2)

177-225: LGTM! Comprehensive branch coverage for Error() method.

The table-driven test systematically validates all formatting branches of Error(), including edge cases where Err and TechnicalMsg are absent. The expected outputs correctly match the formatting logic.

---

227-265: LGTM! Thorough validation of UserMessage() fallback chain.

The table-driven test comprehensively validates the fallback behavior: UserMsg → Err.Error() → Kind. All three branches are correctly tested, including the edge case where both UserMsg and Err are absent.

storage/file_test.go (3)

15-16: LGTM! Deterministic test time improves test reliability.

The fixed test time variable aligns with the PR's goal of removing non-deterministic behavior from tests.

---

34-38: LGTM! Consistent use of deterministic time.

Using testTime for all time-based fields ensures reproducible test behavior and makes test expectations clearer.

---

795-855: LGTM! Comprehensive test coverage for DeleteAllData.

The test properly validates:

• Data exists before deletion
• Both question store and deltas are cleared after deletion
• Cache is invalidated so subsequent loads return empty data

internal/fileutil/fileutil_test.go (2)

392-419: LGTM! Proper test for Delete with cleanup.

The test correctly verifies that:

• The file exists before deletion
• Delete returns no error
• The file no longer exists after deletion
• Includes cleanup in case of test failure (line 417)

---

421-431: LGTM! Validates idempotent delete behavior.

Correctly verifies that deleting a non-existent file returns no error, which is the expected idempotent behavior for file deletion operations.

handler/io.go (2)

10-10: LGTM! Required import for new time functionality.

---

56-56: LGTM! Clean interface extension for time formatting.

The new method signature is straightforward and follows Go conventions.

internal/urlparser/urlparser_test.go (1)

272-276: LGTM! Validates malformed URL error handling.

Good edge case test ensuring the parser correctly returns ErrInvalidURLFormat for URLs with invalid schemes.

internal/clock/clock.go (2)

38-46: LGTM! Clean mock implementation for deterministic testing.

The MockClock struct and constructor are well-designed for injecting fixed times into tests, supporting the PR's goal of removing non-deterministic behavior.

---

48-62: LGTM! Complete Clock interface implementation.

All four methods are correctly implemented:

• Now() returns the fixed time
• Today() properly extracts the date portion at midnight UTC
• ToDate() and AddDays() mirror the production implementation

The mock enables deterministic testing of time-dependent code.

command/command_test.go (2)

22-31: LGTM! MockHandler extended to support new commands.

The MockHandler properly tracks calls and arguments for the new SettingCommand, VersionCommand, MigrateCommand, and ResetCommand, maintaining consistency with the existing mock implementation pattern.

---

538-606: Excellent test coverage for new commands.

The test functions for SettingCommand, VersionCommand, MigrateCommand, and ResetCommand follow the established testing patterns and properly verify:

• Handler methods are invoked
• Quit behavior remains correct (returns false)
• Arguments are passed correctly (e.g., settingArgs for SettingCommand)

usecase/usecase.go (2)

47-54: Clean refactoring to package-level logging.

The constructor signature change removes the logger parameter, centralizing logging at the package level. This is a breaking API change but aligns with the PR's objective to improve testability and remove per-instance dependencies.

---

223-223: Logging calls properly migrated to package-level logger.

All logging statements correctly updated from instance-level (u.logger.Infof/Errorf) to package-level (logger.Infof/Errorf), maintaining the same logging behavior while improving testability.

Also applies to: 323-323, 329-329, 370-370, 376-376, 416-416, 627-627

handler/handler_test.go (4)

20-21: Excellent use of fixed test time for determinism.

Defining a constant testTime eliminates time-based test flakiness and makes test expectations predictable.

---

106-109: Good mock implementation for FormatTimeAgo.

Returning a fixed string "just now" provides deterministic test behavior for time formatting assertions.

---

264-271: setupTestHandler properly updated for new architecture.

The test helper correctly initializes the no-op logger and uses the updated NewHandler signature without a logger parameter, aligning with the refactored logging approach.

---

1326-1585: Comprehensive test coverage for new handler functionality.

The new test functions provide thorough coverage:

• TestHandler_HandleVersion: Verifies version display
• TestHandler_GetChanges: Tests change detection logic with multiple scenarios
• TestHandler_HandleMigrate_*: Tests migration flow with success and cancellation
• TestHandler_HandleReset_*: Tests reset flow with success and cancellation
• TestHandler_HandleSetting_*: Tests settings display and validation
• TestHandler_ExtractQuestionNameFromURL: Tests URL parsing for multiple platforms

config/config_test.go (4)

138-159: Well-structured test for settings registry.

TestGetSettingsRegistry properly verifies:

• Registry is non-empty
• Known configurable settings exist (randomizeinterval, overduepenalty, overduelimit)

---

161-194: Thorough testing of GetSettingValue.

The test properly validates:

• Successful retrieval of valid settings
• Case-insensitive lookup (lines 180-187)
• Error handling for unknown settings

---

196-223: Good coverage of SetSettingValue functionality.

Tests verify:

• Setting int values (OverdueLimit)
• Setting bool values (RandomizeInterval)
• Error handling for unknown settings

---

225-249: GetSettingInfo properly tested.

The test validates metadata retrieval including Name and Type fields, with proper error handling for unknown settings.

internal/clock/clock_test.go (4)

131-141: Good validation of MockClock construction.

TestNewMockClock properly verifies the constructor returns a non-nil instance with the correct FixedTime.

---

143-157: Excellent test for deterministic Now() behavior.

The test verifies that multiple calls to Now() return the same fixed time, which is the key feature of MockClock for eliminating test flakiness.

---

183-207: Comprehensive AddDays test coverage.

The table-driven test covers important scenarios:

• Positive and negative day additions
• Zero days (identity)
• Month boundary crossing

---

214-225: Strong validation of deterministic behavior.

Running 100 iterations ensures MockClock consistently returns the same fixed time across many calls, which is critical for test reliability.

handler/io_test.go (4)

15-89: Excellent test coverage for time formatting edge cases.

The test suite comprehensively covers FormatTimeAgo with deterministic timing using MockClock, including boundary conditions (59 seconds, 59 minutes, 23 hours). This ensures reliable, non-flaky tests.

---

176-225: Good coverage of empty note edge case.

The separate test for empty notes (lines 205-225) ensures the "(none)" fallback is displayed correctly, preventing UI inconsistencies.

---

257-306: Comprehensive error type handling tests.

The test covers all CodedError kinds (ValidationError, BusinessError, SystemError) plus nil error handling, ensuring PrintError behaves correctly across error scenarios.

---

367-437: Delta visualization tests cover both new and update scenarios.

Tests verify both new question creation and updates with change indicators (arrows), ensuring the upsert detail display handles all cases correctly.

handler/handler.go (3)

45-52: Constructor simplified by removing logger dependency.

The removal of the logger parameter aligns with the package-level logger refactoring, reducing constructor complexity and enabling easier testing.

---

475-475: Time formatting now delegated to IO handler.

Using h.IO.FormatTimeAgo instead of a local helper enables deterministic testing via MockClock injection and maintains consistent time formatting across the UI layer.

---

593-602: Multi-platform URL parsing with robust error handling.

The switch to urlparser.Parse adds multi-platform support (LeetCode, HackerRank) and returns "unknown" on parse errors, preventing panics and improving resilience.

core/scheduler.go (3)

18-34: Well-designed RNG abstraction for deterministic testing.

The Rand interface with DefaultRand (production) and FixedRand (testing) implementations is a clean pattern that eliminates non-deterministic test behavior while preserving production randomization.

---

63-65: Constructor delegation maintains backward compatibility.

NewSM2Scheduler delegates to NewSM2SchedulerWithRand with DefaultRand, preserving existing behavior for production code while enabling test injection.

---

179-196: Randomization now uses injected RNG.

Line 185 correctly uses s.Rand.IntN(4) instead of package-level rand.IntN, enabling deterministic testing. The comment accurately explains the range transformation (-1 to +2 days).

main.go (2)

40-43: Graceful error handling for logger initialization.

The logger.Init call now returns an error instead of using log.Fatalf, allowing the application to handle initialization failures gracefully with a clear error message before exiting.

---

46-48: Constructor calls updated for logger refactoring.

Both QuestionUseCase and Handler constructors correctly omit the logger parameter, aligning with the package-level logger architecture introduced in this PR.

internal/logger/logger.go (3)

16-38: Error return instead of fatal exit improves testability.

Replacing log.Fatalf with error returns (lines 23, 29) allows callers to handle initialization failures gracefully, which is essential for testing and improves application robustness.

---

40-44: InitNop enables clean test setup.

The InitNop function provides a simple way to configure no-op loggers for tests, eliminating unnecessary log output and file I/O during test execution.

---

46-58: Defensive nil checks prevent panics.

The nil checks in Infof (line 48) and Errorf (line 55) are good defensive programming, protecting against calls before Init while adding minimal overhead.

core/scheduler_test.go (7)

35-55: LGTM!

The TestRandImplementations tests correctly verify:

• DefaultRand.IntN(n) returns values within the expected range [0, n-1]
• FixedRand.IntN(n) consistently returns the fixed value regardless of input

Good coverage for the RNG abstraction.

---

57-119: LGTM!

Good test structure:

• TestNewSM2Scheduler_Default validates the default constructor properly initializes Clock and Rand
• TestNewSM2Scheduler thoroughly verifies all configuration values (intervals, multipliers, ease factor bounds)

The separation between default and explicit RNG injection tests is clean.

---

121-215: LGTM!

Excellent test coverage for ScheduleNewQuestion:

• Clear comments explaining the expected interval calculations
• Deterministic assertions using exact date comparisons
• Good variety of test cases covering different importance levels and memory types

---

217-294: LGTM!

TestSchedule properly tests:

• Reset behavior for VeryHard familiarity
• Normal scheduling with growth based on ease factor

The deterministic RNG ensures reproducible test results.

---

443-487: LGTM!

TestSetNextReview thoroughly tests interval bounds:

• Normal intervals pass through correctly
• Minimum interval enforced at 1 day
• Maximum interval capped at 90 days

Good boundary testing.

---

590-637: LGTM!

TestEdgeCases properly validates edge conditions:

• Zero interval → minimum 1 day
• Negative interval → minimum 1 day
• Very large interval → capped at maxInterval (90 days)

Essential coverage for robustness.

---

639-670: LGTM!

TestMemoryMultipliers correctly validates that different memory types produce different intervals as expected:

• MemoryReasoned: 8 days (×1.00)
• MemoryPartial: 9 days (×1.10)
• MemoryFull: 10 days (×1.25)

internal/logger/logger_test.go (1)

28-43: LGTM!

TestInitNop correctly verifies:

• Loggers are initialized (non-nil) after InitNop
• Logging calls don't panic with discard writers

usecase/usecase_test.go (9)

16-40: LGTM!

Excellent test setup:

• testTime provides deterministic time baseline
• setupTestEnvironment properly initializes all dependencies with mocks
• Mock clock, fixed RNG scheduler, and no-op logger ensure reproducible tests

---

42-56: LGTM!

createTestQuestion properly uses testTime for all time fields, ensuring consistency with the mock clock.

---

58-125: LGTM!

TestQuestionUseCase_UpsertQuestion comprehensively tests:

• Creating a new question with all fields verified
• Updating an existing question and verifying changes
• Storage persistence validation

---

215-276: LGTM!

Good test for ListQuestionsSummary:

• Uses testTime as reference for yesterday/tomorrow calculations
• Properly verifies all summary fields

---

280-378: LGTM!

Comprehensive search test coverage:

• Text-based search with query terms
• Filter-based search (familiarity)
• Empty query returning all questions

Using UpsertQuestion to populate tries is the correct approach for realistic testing.

---

380-523: LGTM!

Excellent undo test coverage:

• Undo_AddAction: Verifies question is removed after undoing add
• Undo_UpdateAction: Verifies original state is restored after undoing update
• Undo_DeleteAction: Verifies question is restored after undoing delete
• Undo_NoActions: Verifies error when undo stack is empty

---

588-619: LGTM!

TestQuestionUseCase_SchedulerIntegration properly verifies:

• Questions with different familiarity levels are scheduled correctly
• All scheduled times are in the future relative to testTime
• Clean test isolation by deleting each question after verification

---

795-845: LGTM!

TestQuestionUseCase_MigrateToUTC properly:

• Creates a question, then manually sets non-UTC times to simulate legacy data
• Calls MigrateToUTC and verifies the migration counts
• Validates that all times are now in UTC location

---

847-889: LGTM!

TestQuestionUseCase_ResetData properly verifies:

• Data exists before reset
• Reset returns correct counts
• Data is cleared after reset

usecase/usecase_integration_test.go (4)

29-80: LGTM!

TestQuestionUseCase_Integration_UpsertAndGet properly exercises the full upsert-and-retrieve flow:

• Creates a question via UpsertQuestion
• Retrieves by ID and URL
• Verifies all fields match

---

128-174: LGTM!

TestQuestionUseCase_Integration_DeleteAndUndo provides excellent coverage of the delete-undo cycle:

• Creates a question
• Deletes and verifies it's gone
• Undoes the deletion
• Verifies the question is restored

---

176-235: LGTM!

TestQuestionUseCase_Integration_ListQuestionsSummary correctly:

• Uses integrationTestTime for relative date calculations
• Manually populates the store with due/upcoming questions
• Verifies all summary counts

---

237-257: LGTM!

TestQuestionUseCase_Integration_ErrorHandling covers essential error cases:

• Getting non-existent question
• Deleting non-existent question
• Undo with empty stack