Repository navigation
refactor(tests): migrate the remaining features to the snap framework - #556
Merged
Merged
Conversation
The fixture's § annotations become the parameter channel: position features (hover) run once per point, range features (inlay_hint) once per range with a whole-document default, keyed into the new markers field of the envelope. hover emits the feature-layer reply before the edge — markdown from the same rendering code the server uses, with the symbol range still in byte offsets. The feature dispatch grows into a table covering document_links, document_symbol, folding_range, hover, inlay_hint and semantic_tokens.
The four remaining corpora move to tests/snap with their snapshots pinned from both paths (hover shares byte-identical markdown between inspect and the wire replay; the hover CDB pins the target triple since HoverInfo carries sizeof facts). The legacy wire driver, the migrated unit TEST_CASE(snapshot) globs and their snapshot trees retire; the tests/snapshots/integration tree is gone entirely. Cross-checked before deletion: document_symbol, inlay_hint and document_links bodies are byte-identical to the old integration pins, and every hover fixture marker appears exactly once in its new snapshot.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThe inspect CLI now supports registry-driven marker results for additional language features. Snap tooling renders marker-scoped outputs, generates feature-specific compile databases, and removes legacy wire corpus handling. New document-link, document-symbol, hover, and inlay-hint fixtures move into the current snap corpus. ChangesInspect and snapshot pipeline
Snap fixtures
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc2cf8204a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tools/snap/snapshot.ts (1)
87-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates the tail of
normalizeFileUri.Lines 94-100 are byte-identical to lines 74-80 apart from the error message. Extracting a shared
relativizeToWorkspace(fsPath, workspace, describe)keeps the two validators from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/snap/snapshot.ts` around lines 87 - 101, Extract the shared workspace-relative path logic from normalizeFilePath and normalizeFileUri into a helper such as relativizeToWorkspace(fsPath, workspace, describe). Have both validators reuse it while preserving their distinct validation and error-message behavior, then apply the existing workspace placeholder formatting consistently.src/driver/inspect.cc (2)
358-358: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnchecked dereference of
find_feature.Safe today because
run_inspectvalidates first, butprocess_fileis a separate entry point; an assertion or early error would keep it safe under refactoring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/driver/inspect.cc` at line 358, Handle a missing result from find_feature before dereferencing it in process_file: add an assertion or early error path for the absent feature, then only construct spec after validation. Preserve the existing behavior for valid features.
148-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider enforcing the "exactly one shape" invariant.
Nothing prevents a future
FeatureSpecentry from leaving all three pointers null, whichprocess_fileturns into a null function-pointer call on therun_overfallthrough. Astatic_assertover the table (exactly one non-null pointer per spec) would keep the invariant where the table lives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/driver/inspect.cc` around lines 148 - 171, Enforce the one-shape invariant for every entry in the constexpr features table by adding a compile-time assertion that exactly one of FeatureSpec::run, FeatureSpec::run_at, or FeatureSpec::run_over is non-null. Place the validation alongside the features definition so invalid entries fail during compilation before process_file can dispatch through a null pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/driver/inspect.cc`:
- Around line 187-196: The range marker ordering is inconsistent between the C++
and TypeScript consumers. In src/driver/inspect.cc lines 187-196, update
marker_ranges to establish and document the canonical ordering, preferably
grouping nameless markers last to match marker_points; in tools/snap/inspect.ts
lines 421-434, update sortedMarkers to implement that same ordering so wire and
standalone snapshots remain byte-identical.
- Around line 137-146: Update run_hover to distinguish absent hover information
from serialization failure: preserve the empty RawValue for a missing hover, but
return a distinct failure state when to_raw_json(result) cannot serialize.
Propagate that state through the caller, process_file, so it sets entry.error
consistently with the whole-document serialization path instead of recording NO
HOVER.
In `@tests/snap/hover/using_decls.snap.yml`:
- Line 17: Refresh the stale snapshot marker ranges in
tests/snap/hover/using_decls.snap.yml:17-17 and
tests/snap/hover/variables.snap.yml:55-55, updating them to the corresponding
fixture lines: 19:35-19:38 and 41:17-41:20 respectively.
In `@tools/snap/inspect.ts`:
- Around line 376-393: Update renderRawInlayHints so an unmapped hint.kind does
not fall back to the raw clice category name; instead, fail loudly when
LSP_INLAY_KIND lacks the value. Preserve the existing mapped-kind output and
remaining hint formatting.
---
Nitpick comments:
In `@src/driver/inspect.cc`:
- Line 358: Handle a missing result from find_feature before dereferencing it in
process_file: add an assertion or early error path for the absent feature, then
only construct spec after validation. Preserve the existing behavior for valid
features.
- Around line 148-171: Enforce the one-shape invariant for every entry in the
constexpr features table by adding a compile-time assertion that exactly one of
FeatureSpec::run, FeatureSpec::run_at, or FeatureSpec::run_over is non-null.
Place the validation alongside the features definition so invalid entries fail
during compilation before process_file can dispatch through a null pointer.
In `@tools/snap/snapshot.ts`:
- Around line 87-101: Extract the shared workspace-relative path logic from
normalizeFilePath and normalizeFileUri into a helper such as
relativizeToWorkspace(fsPath, workspace, describe). Have both validators reuse
it while preserving their distinct validation and error-message behavior, then
apply the existing workspace placeholder formatting consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d395b59-cac5-4ba9-87da-b572060727fe
⛔ Files ignored due to path filters (1)
tests/snap/document_links/data.binis excluded by!**/*.bin
📒 Files selected for processing (107)
.claude/CLAUDE.md.claude/skills/write-tests/SKILL.mdsrc/driver/inspect.cctests/integration/features/document_links.test.tstests/snap/document_links/café.htests/snap/document_links/escapes.cpptests/snap/document_links/escapes.snap.ymltests/snap/document_links/hash#header.htests/snap/document_links/header_a.htests/snap/document_links/header_b.htests/snap/document_links/header_c.htests/snap/document_links/main.cpptests/snap/document_links/main.snap.ymltests/snap/document_links/plus+header.htests/snap/document_links/reserved.cpptests/snap/document_links/reserved.snap.ymltests/snap/document_links/sub dir/escaped header.htests/snap/document_symbol/.clang-formattests/snap/document_symbol/basic.cpptests/snap/document_symbol/basic.snap.ymltests/snap/hover/.clang-formattests/snap/hover/attributes.cpptests/snap/hover/attributes.snap.ymltests/snap/hover/auto.cpptests/snap/hover/auto.snap.ymltests/snap/hover/basics.cpptests/snap/hover/basics.snap.ymltests/snap/hover/callee_args.cpptests/snap/hover/callee_args.snap.ymltests/snap/hover/concepts.cpptests/snap/hover/concepts.snap.ymltests/snap/hover/decltype.cpptests/snap/hover/decltype.snap.ymltests/snap/hover/docs.cpptests/snap/hover/docs.snap.ymltests/snap/hover/expressions.cpptests/snap/hover/expressions.snap.ymltests/snap/hover/fields.cpptests/snap/hover/fields.snap.ymltests/snap/hover/functions.cpptests/snap/hover/functions.snap.ymltests/snap/hover/getter_setter.cpptests/snap/hover/getter_setter.snap.ymltests/snap/hover/lambdas.cpptests/snap/hover/lambdas.snap.ymltests/snap/hover/misc.cpptests/snap/hover/misc.snap.ymltests/snap/hover/no_hover.cpptests/snap/hover/no_hover.snap.ymltests/snap/hover/no_hover_errors.cpptests/snap/hover/no_hover_errors.snap.ymltests/snap/hover/pass_types.cpptests/snap/hover/pass_types.snap.ymltests/snap/hover/spaceship.cpptests/snap/hover/spaceship.snap.ymltests/snap/hover/tag_decls.cpptests/snap/hover/tag_decls.snap.ymltests/snap/hover/template_params.cpptests/snap/hover/template_params.snap.ymltests/snap/hover/templates.cpptests/snap/hover/templates.snap.ymltests/snap/hover/this_expr.cpptests/snap/hover/this_expr.snap.ymltests/snap/hover/using_decls.cpptests/snap/hover/using_decls.snap.ymltests/snap/hover/values.cpptests/snap/hover/values.snap.ymltests/snap/hover/variables.cpptests/snap/hover/variables.snap.ymltests/snap/inlay_hint/.clang-formattests/snap/inlay_hint/basic.cpptests/snap/inlay_hint/basic.snap.ymltests/snap/snap.test.tstests/snapshots/unit/document_symbol/snapshot/basic.cpp.snap.ymltests/snapshots/unit/hover/snapshot/attributes.cpp.snap.ymltests/snapshots/unit/hover/snapshot/auto.cpp.snap.ymltests/snapshots/unit/hover/snapshot/basics.cpp.snap.ymltests/snapshots/unit/hover/snapshot/callee_args.cpp.snap.ymltests/snapshots/unit/hover/snapshot/concepts.cpp.snap.ymltests/snapshots/unit/hover/snapshot/decltype.cpp.snap.ymltests/snapshots/unit/hover/snapshot/docs.cpp.snap.ymltests/snapshots/unit/hover/snapshot/expressions.cpp.snap.ymltests/snapshots/unit/hover/snapshot/fields.cpp.snap.ymltests/snapshots/unit/hover/snapshot/functions.cpp.snap.ymltests/snapshots/unit/hover/snapshot/getter_setter.cpp.snap.ymltests/snapshots/unit/hover/snapshot/lambdas.cpp.snap.ymltests/snapshots/unit/hover/snapshot/misc.cpp.snap.ymltests/snapshots/unit/hover/snapshot/pass_types.cpp.snap.ymltests/snapshots/unit/hover/snapshot/spaceship.cpp.snap.ymltests/snapshots/unit/hover/snapshot/tag_decls.cpp.snap.ymltests/snapshots/unit/hover/snapshot/template_params.cpp.snap.ymltests/snapshots/unit/hover/snapshot/templates.cpp.snap.ymltests/snapshots/unit/hover/snapshot/this_expr.cpp.snap.ymltests/snapshots/unit/hover/snapshot/using_decls.cpp.snap.ymltests/snapshots/unit/hover/snapshot/values.cpp.snap.ymltests/snapshots/unit/hover/snapshot/variables.cpp.snap.ymltests/snapshots/unit/inlay_hint/snapshot/basic.cpp.snap.ymltests/unit/feature/document_symbol_tests.cpptests/unit/feature/hover_tests.cpptests/unit/feature/inlay_hint_tests.cpptools/compile_commands.tstools/snap/annotation.tstools/snap/inspect.tstools/snap/presenters.tstools/snap/snapshot.tstools/snap/standalone.tstools/snap/wire.ts
💤 Files with no reviewable changes (28)
- tests/snapshots/unit/hover/snapshot/getter_setter.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/template_params.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/using_decls.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/this_expr.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/pass_types.cpp.snap.yml
- tests/snapshots/unit/document_symbol/snapshot/basic.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/functions.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/docs.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/expressions.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/tag_decls.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/attributes.cpp.snap.yml
- tests/snapshots/unit/inlay_hint/snapshot/basic.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/misc.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/values.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/fields.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/templates.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/spaceship.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/callee_args.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/concepts.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/lambdas.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/variables.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/basics.cpp.snap.yml
- tests/unit/feature/inlay_hint_tests.cpp
- tests/snapshots/unit/hover/snapshot/auto.cpp.snap.yml
- tests/snapshots/unit/hover/snapshot/decltype.cpp.snap.yml
- tests/unit/feature/hover_tests.cpp
- tests/unit/feature/document_symbol_tests.cpp
- tools/compile_commands.ts
- Integration setup generates the snap-corpus CDBs too: behavioral tests borrow snap workspaces (document links) and must not run on synthesized default commands by luck. - nameless_<i> is a reserved marker namespace in both annotation twins; a named annotation using it fails loudly instead of colliding. - Wire markerRanges orders named-then-nameless like every other consumer. - Hover serialization failure is now distinct from NO HOVER (and range features report serialize_error instead of snapshotting null). - An unmapped inlay-hint kind throws instead of leaking the raw name into a shared snapshot.
extract_snap_regions and fixture_frontmatter lost their last real consumers when the feature snapshot globs migrated to tests/snap; only their own tests kept them alive. tu_index (zest glob) and the hover presentation cases are the only unit snapshot users left.
This was referenced Jul 31, 2026
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.
What
Completes the snap-test migration started in the framework refactor: hover, document_symbol, inlay_hint and document_links move to
tests/snap/, and every legacy snapshot mechanism retires. All six features are now pinned from both paths —clice inspect(standalone, no PCH) and a real server over LSP — into shared, byte-identical snapshot files.Marker channel
Fixture
§annotations become the parameter channel for inspect, so feature parameters live in the file itself:§(name)): position features run once per point — hover produces one payload per marker, keyed by name (nameless_<i>for unnamed ones).§(name)⟦...⟧): range features run once per marked range — inlay hints scope their request to the marked region, falling back to the whole document when a fixture marks nothing.The server never sees a marker: both paths strip them at the entrance with the existing twin parsers, and the sha256 handshake keeps the coordinate space honest. On the wire side the harness holds the offsets and issues one request per marker, exactly like a real editor would.
Per-feature notes
--target=x86_64-unknown-linux-gnubecause HoverInfo carries sizeof/alignof facts that differ between LP64 and LLP64 hosts.${WS}/...form the wire side pins.-I+ C++23; hover pins the triple).Retired
TEST_CASE(snapshot)globs in the hover, document_symbol and inlay_hint unit tests (granular unit cases stay).tests/snapshots/integration/entirely, and the migratedtests/snapshots/unit/subtrees (tu_index and the hover presentation cases remain on zest).Verification
snap/<feature>addressing.