self-development: assert byte-equality in stream-forwarding tests#1223
Closed
kelos-bot[bot] wants to merge 1 commit into
Closed
self-development: assert byte-equality in stream-forwarding tests#1223kelos-bot[bot] wants to merge 1 commit into
kelos-bot[bot] wants to merge 1 commit into
Conversation
Adds a new convention to the agent rule list in AGENTS.md (with CLAUDE.md as its symlink) and the inline agentsMD copies in self-development/ agentconfig.yaml and kelos-workers.yaml: when a function both forwards bytes from input to output and parses metadata along the way, tests must assert byte-for-byte equality (out.String() == want) instead of per-line strings.Contains. Substring checks still pass when the forwarder drops a partial line, reorders writes, or duplicates output. Motivated by repeated reviewer feedback on PR #1189, where both Cubic [P2] and the Kelos reviewer agent [P3] independently flagged assertForwarded in internal/capture/usage_test.go for using strings.Contains(out, line+"\n") per input line and missing the drop/reorder/duplicate failure modes that the streaming refactor introduced. Three rounds of feedback ended with the test being rewritten to byte-equality against the normalized input.
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.
🤖 Kelos Agent @gjkim42
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Adds one new convention to
AGENTS.md(withCLAUDE.mdas its symlink) and the inlineagentsMDcopies inself-development/agentconfig.yamlandself-development/kelos-workers.yaml:Motivating review feedback
Both reviewers on PR #1189 (
Stream agent output through kelos-capture instead of writing to /tmp) independently flagged the same test-style issue oninternal/capture/usage_test.go:169-179:internal/capture/usage_test.go:175: "Forwarding check too weak.Containscan miss dropped/reordered/duplicated lines. Compare normalized full output instead."assertForwardedonly checksstrings.Contains(out, line+"\n")for each non-empty input line, so the assertion still passes if the forwarder duplicates, reorders, or interleaves extra output.TestStreamUsageForwardsUnterminatedLinealready does the stricter byte-level comparison; the same shape (normalize input to end in\n, thenif out.String() != want) could be reused across the table-driven cases for a stronger forwarder invariant."The same PR also bounced through three rounds of reviewer feedback partly because the weak
Containsassertion masked the very regressions the refactor introduced (oversized-line handling, deferred-flush error). Codifying the byte-equality rule up-front would have shortened the review cycle.The convention complements — but does not duplicate — the existing Avoid vacuous substring assertions in printer/formatter tests rule. That rule covers
label: valuecollisions in CLI-output tests; this one covers byte-fidelity in stream-forwarding tests. Different domains, same underlying failure mode:strings.Containsis too permissive a check.Scope notes
CLAUDE.mdis a symlink toAGENTS.md, so a single edit covers both surfaces.agentsMDinself-development/agentconfig.yamlandself-development/kelos-workers.yaml, which is the existing pattern for keepingagentsMDin sync withAGENTS.md.self-development/*.yamlspawners — they consume the convention list via the sharedagentsMD; only the two files that inline that list need editing.Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
N/A
Does this PR introduce a user-facing change?
+ "```" +release-noteNONE
+ "```" +Summary by cubic
Add a testing convention to assert byte-for-byte equality in stream-forwarding tests instead of per-line
strings.Containschecks. UpdateAGENTS.mdand the inlineagentsMDinself-development/agentconfig.yamlandself-development/kelos-workers.yamlto avoid tests that miss dropped, reordered, or duplicated output; no code changes.Written for commit 873004b. Summary will update on new commits. Review in cubic