test(grep): pin -n with -A/-B context line numbering + separators#14
Open
tobert wants to merge 1 commit into
Open
test(grep): pin -n with -A/-B context line numbering + separators#14tobert wants to merge 1 commit into
tobert wants to merge 1 commit into
Conversation
Existing tests cover `-n` alone and context alone, but nothing exercised them together. Add a test asserting the GNU-compatible output: every emitted line is numbered, matched lines use the `:` separator, and before/after context lines use `-`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
kaibo review (cast: deepseek — explorer
|
| Question | Verdict |
|---|---|
| Expected output correct? | ✅ Exact match with render_events |
| Exercises genuine rendering path? | ✅ Hits grep_lines_structured → render_events; context flags force the whole-buffer path, skipping the streaming shortcut |
| Fixture + match line correct? | ✅ "two" matches only line 2; lines 1 and 3 are the correct before/after context |
| Gaps? | -C not a gap (resolves to the same GrepOptions + code path). -- break + -n and multi-file + context + -n are possible future tests, out of scope here. |
Detail
- Separators: the
prefix(line_num, sep)closure (grep.rs:1013–1025) emits':'forSearchEvent::Match(grep.rs:1055) and'-'for allContextKindvariants (grep.rs:1076–1078) — so1-line one,2:line two,3-line threeis exactly right. - Path fidelity: the
before-context/after-contextnamed keys are the same canonical keys the kernel dispatcher binds;to_argv()round-trips them to--before-context=1/--after-context=1, clap parses,parse_context()resolves toSome(1). Because context isSome, the streaming shortcut (grep.rs:358–359) is skipped, so the test genuinely hits therender_eventsseparator logic. Consistent with the existing direct-execute()convention used by all grep tests.
Suggested follow-ups (not blocking, deferred)
- multi-file + context +
-n(filename prefix interacting with line-number prefix:file.txt:2:line twofor matches,file.txt-1-line onefor context) --context-break separator with-non non-contiguous matches
🤖 Generated with Claude Code
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.
What
Adds
test_grep_line_numbers_with_context, exercising-ntogether with-A/-Bcontext — a combination no existing test covered (we had-nalone and context alone).Why
-n's interaction with context is where GNU compatibility lives: matched lines use the:separator, context lines use-, and every emitted line must be numbered. Nothing pinned that, so a regression in either the numbering or the separator logic would have passed silently.Test
Matches
twoin/lines.txt(line one\nline two\nline three\nfour) with-n -B1 -A1and asserts exact output:Exact-equality so it fails loudly on any drift.
cargo test -p kaish-kernel --lib grep— 44 passedcargo clippy --all-targets— clean🤖 Generated with Claude Code