test(parsers): annotate every test with its CASE.* in TEST_CASES.md#8729
test(parsers): annotate every test with its CASE.* in TEST_CASES.md#8729keivenchang wants to merge 3 commits into
Conversation
Cross-index existing inline parser tests with the universal test-case taxonomy in lib/parsers/TESTING.md. Each test gets a one-line trailer on its #[test] / #[rstest] attribute (e.g. `#[test] // CASE.5, CASE.16`) so a reader of TESTING.md can grep `CASE.5` and find every test that exercises missing-end-token recovery, across every parser. Coverage: - 232 inline tests annotated across 9 tool-call parsers + 5 reasoning parsers (lib/parsers/src/tool_calling/ + lib/parsers/src/reasoning/). - 5 new categories added to TESTING.md (CASE.20-24) for tests that didn't fit the existing 19 entries: detection helpers, function-name conventions, whitespace tolerance, token-format variants, empty wrappers. - Multi-category tests carry all relevant CASEs on one line. No code change, just comments + TESTING.md edits. 389 existing tests still pass. Deferred to follow-up: annotating tests under lib/llm/tests/ (integration suites like tool_choice.rs, tool_choice_finish_reasons.rs, test_streaming_tool_parsers.rs). Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
Annotated tests in the parser sources are the index now (one comment per test). TESTING.md should describe what each CASE covers, not duplicate that index. Removed all Ex: ... / Example: ... lines. Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
WalkthroughTest functions across parser modules are annotated with inline case identifiers (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/parsers/src/tool_calling/dsml/parser.rs (1)
324-327:⚠️ Potential issue | 🟡 MinorReplace internal Linear ticket reference in source comments.
pre-DIS-1765introduces an internal ticket ID in added Rust lines. Please replace it with a public GitHub issue/PR reference (or neutral wording) to keep source comments externally safe and compliant. As per coding guidelines, "**/*.{py,rs,go,ts,tsx,js,mjs,cjs,sh,yaml,yml,toml}: Flag internal Linear ticket references (e.g. DIS-XXXX, DYN-XXXX, OPS-XXXX, DEP-XXXX, or any -NNNN ID) in the added lines; see .ai/linear-ticket-refs.md for what to recommend."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/parsers/src/tool_calling/dsml/parser.rs` around lines 324 - 327, The source comment in parser.rs contains an internal Linear ticket reference "pre-DIS-1765" inside the CASE.5 comment; replace that token with either a public GitHub issue/PR URL or neutral wording (e.g., "prior internal ticket" or "pre-release issue") so no internal IDs remain. Update the comment text near the CASE.5 block (the lines mentioning "Fix mid-stream truncation" and "pre-DIS-1765") to use the neutral phrasing or a public reference, keeping the rest of the comment intact and preserving the recovery pattern description and surrounding wording.
🧹 Nitpick comments (2)
lib/parsers/TEST_CASES.md (2)
46-52: Clarify applicability ofCASE.20–CASE.24in summary sections.These new categories are well-defined, but their required/optional scope is not reflected in the existing “Category layout” and “Applicability summary,” which may create ambiguity for new parser authors. A short summary row or note would close the loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/parsers/TEST_CASES.md` around lines 46 - 52, Add a short, explicit applicability note for CASE.20–CASE.24 to the “Category layout” and “Applicability summary” sections: state which categories are required vs optional for parsers (e.g., CASE.20 detection helpers required for streaming entry points like detect_tool_call_start_* / find_tool_call_end_position_*, CASE.21–CASE.23 rules optional but recommended, and CASE.24 must-be-handled/edge-case behavior), and include a single-row summary or brief bulleted note that maps CASE.20–CASE.24 to their scope (parser-internal vs full-pipeline) and relationship to CASE.8 so new author readers can immediately see applicability.
35-35: Prefer explicit GitHub issue/PR references over generic “ticket ID”.To align with the markdown guideline, this line should explicitly require a GitHub issue/PR reference (and optionally Linear as secondary context when paired).
Suggested wording
-- **`CASE.16`** Regression for a specific customer bug (ticket ID referenced in test name or body). +- **`CASE.16`** Regression for a specific customer bug (GitHub issue/PR referenced in test name or body; optional Linear ID only when paired with the matching GitHub link).As per coding guidelines "
**/*.md: prefer to reference GitHub issues instead of Linear tickets, but may include Linear ticket references for internal context when a matching GitHub link also exists."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/parsers/TEST_CASES.md` at line 35, Update the TEST_CASES.md entry for "CASE.16" so it requires an explicit GitHub issue or PR link instead of a generic "ticket ID": locate the line containing "**`CASE.16`** Regression for a specific customer bug (ticket ID referenced in test name or body)" and change the wording to demand a GitHub issue/PR reference (you may allow an optional Linear ticket for internal context only when paired with the GitHub link); ensure the new text clearly instructs contributors to include the GitHub URL in the test name or body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/parsers/src/tool_calling/dsml/parser.rs`:
- Around line 324-327: The source comment in parser.rs contains an internal
Linear ticket reference "pre-DIS-1765" inside the CASE.5 comment; replace that
token with either a public GitHub issue/PR URL or neutral wording (e.g., "prior
internal ticket" or "pre-release issue") so no internal IDs remain. Update the
comment text near the CASE.5 block (the lines mentioning "Fix mid-stream
truncation" and "pre-DIS-1765") to use the neutral phrasing or a public
reference, keeping the rest of the comment intact and preserving the recovery
pattern description and surrounding wording.
---
Nitpick comments:
In `@lib/parsers/TEST_CASES.md`:
- Around line 46-52: Add a short, explicit applicability note for
CASE.20–CASE.24 to the “Category layout” and “Applicability summary” sections:
state which categories are required vs optional for parsers (e.g., CASE.20
detection helpers required for streaming entry points like
detect_tool_call_start_* / find_tool_call_end_position_*, CASE.21–CASE.23 rules
optional but recommended, and CASE.24 must-be-handled/edge-case behavior), and
include a single-row summary or brief bulleted note that maps CASE.20–CASE.24 to
their scope (parser-internal vs full-pipeline) and relationship to CASE.8 so new
author readers can immediately see applicability.
- Line 35: Update the TEST_CASES.md entry for "CASE.16" so it requires an
explicit GitHub issue or PR link instead of a generic "ticket ID": locate the
line containing "**`CASE.16`** Regression for a specific customer bug (ticket ID
referenced in test name or body)" and change the wording to demand a GitHub
issue/PR reference (you may allow an optional Linear ticket for internal context
only when paired with the GitHub link); ensure the new text clearly instructs
contributors to include the GitHub URL in the test name or body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b99ee381-61b3-4649-9295-73706b4ebc51
📒 Files selected for processing (17)
lib/parsers/README.mdlib/parsers/TEST_CASES.mdlib/parsers/src/reasoning/base_parser.rslib/parsers/src/reasoning/gpt_oss_parser.rslib/parsers/src/reasoning/granite_parser.rslib/parsers/src/reasoning/minimax_append_think_parser.rslib/parsers/src/reasoning/mod.rslib/parsers/src/tool_calling/dsml/parser.rslib/parsers/src/tool_calling/harmony/harmony_parser.rslib/parsers/src/tool_calling/json/base_json_parser.rslib/parsers/src/tool_calling/json/deepseek_v3_1_parser.rslib/parsers/src/tool_calling/json/deepseek_v3_parser.rslib/parsers/src/tool_calling/json/mod.rslib/parsers/src/tool_calling/pythonic/pythonic_parser.rslib/parsers/src/tool_calling/xml/glm47_parser.rslib/parsers/src/tool_calling/xml/kimi_k2_parser.rslib/parsers/src/tool_calling/xml/parser.rs
05e0837 to
5c5bb7b
Compare
|
closing — duplicate of #8725, which is the canonical taxonomy-annotation PR. work continues there. |
Closed — duplicate of #8725. See #8725 for the canonical taxonomy-annotation PR.