test(subagents): consolidate anemic loader and actor tests#654
Merged
Conversation
The loader test suite from #652 had nine near-identical tests that all reduced to `Assert.Empty(results)` after writing one malformed file. If the loader were rewritten to unconditionally return an empty list, all nine would still pass — they looked like they pinned nine distinct validation paths but actually pinned zero and never verified the "fail loud" contract (no assertion that a warning was logged, no check that the scanner actually reached the file). Replace with one consolidated test that drops a valid agent alongside eight kinds of invalid siblings in the same directory, asserts the valid agent still loads, and verifies each invalid sibling produced a specific warning naming both the file and the reason. That exercises both "don't abort the scan on one bad file" and "fail loud" in a single test. Also asserts that stray .json and .txt files in the directory are ignored at the glob layer — they must produce no warning at all because the scanner never touches them. Introduce a minimal `ListLogger<T>` that captures warning-level formatted messages so tests can assert on log contents. ~20 lines, lives in the same test file; if we need it elsewhere we can promote it. Separately, delete three `BuildUserMessage_*` unit tests on `SubAgentActor`. They were textbook examples of what CLAUDE.md says to delete ("if the test is just asserting that $"{a}/{b}" equals "a/b", delete it") — they pinned a 4-line string interpolation helper whose behavior is already covered by the two end-to-end `RuntimeContext_*` tests. With the unit tests gone, `BuildUserMessage` has no external caller, so tighten its visibility from `internal` to `private`. Net: Configuration.Tests 189 → 181, Actors.Tests SubAgent filter 30 → 27. Fewer tests, each covering real behavior. All green; slopwatch clean.
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
1 participant
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
LoadAll_skips_Xtests — each of which wrote one bad file and just assertedAssert.Empty(results)— with one consolidatedLoadAll_loads_valid_agent_and_rejects_invalid_siblings_with_warningsthat drops a valid agent alongside eight kinds of invalid siblings in the same directory and verifies (a) the valid agent still loads, (b) each invalid sibling produced a specific warning naming both the file and the reason, and (c) stray.json/.txtfiles produce no warning at all because the scanner never touches them.ListLogger<T>(≈20 lines, inline in the test file) that captures warning-level formatted messages so tests can assert on log contents.BuildUserMessage_*unit tests onSubAgentActor. They pinned a 4-line string interpolation helper whose behavior is already fully covered by the two end-to-endRuntimeContext_is_prefixed_onto_first_user_messageandNull_RuntimeContext_leaves_first_user_message_as_raw_tasktests. With the unit tests gone,BuildUserMessagehas no external caller, so tighten its visibility frominternaltoprivate.Why
The
LoadAll_skips_Xtests from #652 were a pattern the Netclaw constitution explicitly warns against: "If the test is just asserting that$"{a}/{b}"equals"a/b", delete it. Prefer fewer tests that cover real behavior over many tests that pad coverage." They looked like they pinned nine distinct validation paths but if the loader were rewritten to unconditionally return an empty list for every file, all nine would still pass. That's negative signal dressed up as coverage.More importantly, the whole point of the format redesign was "fail loud on malformed files" (per CLAUDE.md's no-silent-fallbacks rule), and the old tests didn't actually verify any warning was logged. A silent-skip regression would have sailed through the suite.
The
BuildUserMessage_*unit tests were the same smell applied to a string helper. The E2E tests exercise the same helper through the actor pipeline and catch any regression the unit tests would.Test plan
dotnet test src/Netclaw.Configuration.Tests— 181/181 green (was 189 before; removed 8 redundant tests)dotnet test src/Netclaw.Actors.Tests --filter FullyQualifiedName~SubAgent— 27/27 green (was 30; removed 3 redundant unit tests)dotnet slopwatch analyze— 0 issuesNotes
The one consolidated test intentionally asserts on substring matches in captured warning messages rather than exact strings, so minor copy edits to the loader's warning text won't break the suite — but a silent skip (no warning at all) or a warning that omits the filename / reason absolutely will.