test(core): cover DirHistory move-to-front, trim, persistence, bounds - #1663
Open
hi-neason wants to merge 1 commit into
Open
test(core): cover DirHistory move-to-front, trim, persistence, bounds#1663hi-neason wants to merge 1 commit into
hi-neason wants to merge 1 commit into
Conversation
DirHistory's exported methods (Add, List, Get, Previous, Contains, SetMaxSize) had no direct tests; engine tests only used NewDirHistory as setup. The methods carry non-trivial logic — move-to-front dedup, maxSize trimming, 1-based Get with bounds check, Previous at index 2, defensive-copy List, JSON persistence across instances — so a regression in any of them would have gone unnoticed. Add table-driven tests covering each behavior plus the corrupt-file load path. Co-Authored-By: Claude <noreply@anthropic.com>
chenhg5
approved these changes
Aug 13, 2026
chenhg5
left a comment
Owner
There was a problem hiding this comment.
结论: Approve
总体判断: 一个 test-only PR,给 core/dir_history.go 加了 13 个 table-driven 测试,覆盖 6 个 exported method + load/reload/corrupt 三个错误路径。production 代码不动,CI 全绿。建议合入。
Review 范围:
- 看了
core/dir_history_test.go新增的 181 行 + 13 个测试函数。 - 看了 PR body 列出的覆盖矩阵(move-to-front / dedup / trim / bounds / persistence / corrupt / per-project isolation)。
- CI: run 31349356164 全绿。
✅ 做得好的地方:
- 覆盖矩阵完整:每个 exported method 都有独立测试,且覆盖 happy path + edge case + error path(corrupt JSON → ignored + 后续 Add 仍可用)。这种「不漏测」是后续 refactor dir_history.go 的安全网。
- Defensive copy 测试钉住不变式:
TestDirHistory_List_ReturnsDefensiveCopy验证 List 返回的 slice 修改不影响内部 state——这是 LRU-like 结构的常见 bug,作者明确测了它。 - Per-project isolation 测试:
TestDirHistory_PerProjectIsolation验证两个 project 不共享 entries。这是 data-structure correctness 的关键不变式,漏了会导致跨用户串数据。 - Persistence 测试用真实 filesystem:用
t.TempDir()+ 第二次NewDirHistory实例化——不 mock filesystem 是合理选择(这是 stdlib JSON + 真实目录读写)。 - Corrupt JSON 测试钉住 graceful degradation:malformed JSON 时不 panic、log 但忽略、Add 仍工作。这是「用户数据损坏 → 不阻断服务」的标准 pattern。
🟠 建议改进(不阻塞):
TestDirHistory_PreviousReturnsIndexTwo测试命名:实际钉住的是「Previous 固定返回 index 2」。如果未来Previous的语义被改成「上一条」(LRU 语义)而不是「倒数第二条」(固定的 index 2),这个测试需要跟着改。建议测试名更直接:TestDirHistory_PreviousReturnsSecondMostRecent或加注释明确「previous = history index 2」的设计意图。TestDirHistory_Add_TrimsToMaxSize没测「SetMaxSize 后再 trim」:作者写了SetMaxSize_AppliesToNextAdd但没明确「先 Add 5 个(maxSize=10)→ SetMaxSize(3) → Add 第 6 个 → 验证总长仍 10 但顺序不变」。建议补一个 case。- Test 命名风格统一:13 个测试里有些用
TestDirHistory_Add_MovesToFrontAndDedups(方法_行为),有些用TestDirHistory_PerProjectIsolation(行为)。建议统一为Method_Behavior风格便于 grep。
🔵 可选优化:
- 可以加一个 fuzz test(
FuzzDirHistory_Add)验证任意输入序列都不 panic。低优先。
Testing / Risk:
- 已看到的验证: CI 全绿;13 个新测试通过。
- Blast radius: 0(test-only,无 production 改动)。
Next step:
- 建议 owner 直接 merge。Test-only PR,没有 production 改动风险,coverage 提升明显。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
core/dir_history.gotracks per-project recently-used directories (backing the/cd-style switch history). Its six exported methods carry non-trivial logic — move-to-front dedup onAdd, trim-to-maxSize, 1-based indexing onGetwith bounds checking,Previousfixed at index 2, defensive-copy onList, JSON persistence across instances — but the package had nodir_history_test.go. Existing coverage only instantiatedNewDirHistoryas setup inengine_test.go, so any regression in the history behavior itself would have gone unnoticed.Change
No production code changes — this PR adds
core/dir_history_test.gowith 13 table-driven tests covering every exported method plus the load/reload and corrupt-file paths.Type of change
Testing
Automated tests added in this PR
core/dir_history_test.go:TestDirHistory_Add_MovesToFrontAndDedups— re-adding a directory moves it to the front and does not duplicate.TestDirHistory_Add_IgnoresEmpty— empty dir is a no-op.TestDirHistory_Add_TrimsToMaxSize— history is capped at the most recent N entries.TestDirHistory_SetMaxSize_AppliesToNextAdd— shrinkingmaxSizedoes not retroactively drop entries but trims on the nextAdd.TestDirHistory_SetMaxSize_ClampsToAtLeastOne—SetMaxSize(0)clamps to 1.TestDirHistory_Get_OneBasedAndBoundsCheck— index 1 is the most recent, index 0/out-of-range returns "".TestDirHistory_PreviousReturnsIndexTwo—Previousis hard-wired to the second-most-recent entry.TestDirHistory_Contains— true / false / unknown project cases.TestDirHistory_List_ReturnsDefensiveCopy— mutating the returned slice does not affect internal state.TestDirHistory_PerProjectIsolation— two projects do not share entries.TestDirHistory_PersistsAcrossInstances— a secondNewDirHistoryon the same data dir rehydrates from disk in most-recent-first order.TestDirHistory_Load_MissingFileIsNotAnError— empty data dir is not an error andListreturns nil.TestDirHistory_Load_CorruptFileIsIgnored— malformed JSON is logged and ignored; subsequentAddstill works.For bug fixes only — regression test
Not applicable — test-only change with no production fix.
Critical User Journeys (CUJ) impact
Manual / user-visible behavior change
None.
Checklist (reviewer will verify)
go build ./...passesgo test ./core/ -run TestDirHistorypasses (all 13 new tests)core/Related
None.