Skip to content

Add tracing event snapshots (#305) - #411

Open
lodyai[bot] wants to merge 5 commits into
mainfrom
issue-305-add-insta-snapshot-tests-for-structured-tracing-event-output
Open

Add tracing event snapshots (#305)#411
lodyai[bot] wants to merge 5 commits into
mainfrom
issue-305-add-insta-snapshot-tests-for-structured-tracing-event-output

Conversation

@lodyai

@lodyai lodyai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add deterministic insta snapshots for inline fragment, token parsing, and date-span tracing events.
  • Normalize volatile log prefixes while preserving levels, targets, messages, and structured fields.
  • Verify snapshot references and the full test suite.

Closes #305

References

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @LodyAI[bot], you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Add test-only tracing snapshot support, normalize captured event lines, and assert stable Insta snapshots for inline fragment classification, date-span detection, footnote parsing, and link-or-image parsing.

Changes

Tracing snapshot coverage

Layer / File(s) Summary
Shared snapshot support
src/wrap.rs, src/wrap/tracing_snapshot_support.rs, docs/developers-guide.md
Add test-only wiring, shared event-line normalization, property tests, and developer guidance for deterministic tracing snapshots.
Inline tracing snapshots
src/wrap/inline/fragment.rs, src/wrap/inline/fragment_tracing_snapshots.rs, src/wrap/inline/span_helper_tracing_tests.rs
Capture and snapshot fragment-classification and date-sequence span events while asserting returned span and matching results.
Parsing tracing snapshots
src/wrap/tokenize/parsing.rs, src/wrap/tokenize/parsing_tracing_snapshots.rs
Wire in snapshot tests for footnote-reference and link-or-image events, including parse-result assertions.

Possibly related issues

  • leynos/mdtablefix#305 — Covers the fragment, parsing, and span tracing snapshot tests added here.

Possibly related PRs

Suggested labels: Issue

Poem

Stable traces line up bright,
DEBUG stars survive the night.
Fragments, spans, and links agree,
Snapshots guard the wrapping sea.
Insta seals each event with glee.


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Testing (Overall) ❌ Error FAIL: the declined-footnote span test can pass with end==0, and the normalisation property test still checks only count/trim, not matched content. Assert the returned span/width in the declined-coupling case, and compare each normalised line with the corresponding input line after stable-prefix stripping.
Testing (Unit And Behavioural) ⚠️ Warning The new tests lean on private tracing seams and snapshots; the helper property tests only count and trim lines, and the declined-coupling case never asserts the retained span or width. Add behaviour-first assertions at public boundaries, compare each normalised line to the matched input, and assert end/width in the declined-coupling case; keep snapshots supplementary.
✅ Passed checks (18 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the PR’s main change and includes the linked issue reference #305.
Description check ✅ Passed The description clearly describes the tracing snapshot work and related test updates.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
User-Facing Documentation ✅ Passed Only test-only tracing snapshot helpers were added; no user-facing behaviour changed, and docs/users-guide.md already covers the wrapping behaviour.
Developer Documentation ✅ Passed PASS: docs/developers-guide.md now documents the new test-only tracing snapshot helper, module locations, and reuse rules; no related ADR/execplan or locale docs exist.
Module-Level Documentation ✅ Passed PASS: Every touched Rust module, including nested test/snapshot modules and the new support module, has a leading doc comment describing purpose and context.
Testing (Property / Proof) ✅ Passed Keep the new invariants under proptest: stable_event_start and normalise_event_lines now have property tests; no substantive proof obligation is introduced.
Testing (Compile-Time / Ui) ✅ Passed PASS: No new compile-time behaviour appears; the added tracing snapshots pair stable redaction with concrete parse/span/classification assertions.
Unit Architecture ✅ Passed PASS: tracing_snapshot_support is gated by #[cfg(test)] and only used from #[traced_test] snapshot tests via logs_assert; production wrapping/parsing code is unchanged.
Domain Architecture ✅ Passed PASS: Changes are test-only tracing snapshots and docs; the new helper is cfg(test) pub(crate), and no production/domain code paths were altered.
Observability ✅ Passed Only test-only tracing snapshot support and docs changed; no production logging, metrics, alerts, or operational behaviour changed.
Security And Privacy ✅ Passed Test-only tracing snapshots and a helper were added; the committed snapshots contain only synthetic inputs and stable metadata, with no secrets or privilege changes.
Performance And Resource Use ✅ Passed All new loops and allocations live behind #[cfg(test)] and operate on small captured log slices; no production hot path, I/O, or unbounded growth changed.
Concurrency And State ✅ Passed Keep the change in the test-only snapshot path; it adds no shared mutable state, async work, locking, or ordering-sensitive logic beyond local RefCell buffers.
Architectural Complexity And Maintainability ✅ Passed Accept it: the new helper is test-only, reused by three snapshot sites, and the guide documents ownership and reuse boundaries.
Rust Compiler Lint Integrity ✅ Passed Touched Rust files add test-only helpers and snapshots without new lint suppressions, artificial references, or extra clone chains.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-305-add-insta-snapshot-tests-for-structured-tracing-event-output

Comment @coderabbitai help to get the list of available commands.

@leynos
leynos marked this pull request as ready for review July 26, 2026 02:22

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ae4ebded3

ℹ️ 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".

Comment thread src/wrap/inline/fragment.rs Outdated
Comment thread src/wrap/tracing_snapshot_support.rs
Comment thread src/wrap/tracing_snapshot_support.rs
@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot added the Issue label Jul 26, 2026
@leynos

leynos commented Jul 26, 2026

Copy link
Copy Markdown
Owner

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Reviews resumed.

leynos and others added 2 commits July 26, 2026 23:35
Capture stable tracing output for inline fragment, parser, and date-span\nclassification events. The snapshots preserve level, target, message, and\nstructured fields while excluding volatile prefixes.
Move the fragment-classification snapshot test out of the oversized
`fragment.rs` (which had grown past the 400-line limit) into a dedicated
`fragment_tracing_snapshots.rs`, wired as a `#[path]` submodule to mirror the
tokenizer snapshot layout. Add a usage example to the shared
`normalise_event_lines` helper showing the `tracing-test` line format and its
normalized output.

Document the tracing-event snapshot-testing workflow and record the shared
helper's ownership boundaries, permitted call-sites, and composition rules in
docs/developers-guide.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lodyai
lodyai Bot force-pushed the issue-305-add-insta-snapshot-tests-for-structured-tracing-event-output branch from abc3376 to c55f13f Compare July 26, 2026 21:38
Respond to review feedback that the tracing snapshot tests only pinned
internal trace output. The snapshot and event tests now also assert the
observable parsing, classification, and span-grouping results (link/footnote
atomicity, plain-fragment classification, date-sequence grouping and width,
and the declined-coupling edge case), keeping the trace snapshots
supplementary rather than primary.

Add proptest coverage for the shared normalisation helper: prove
`stable_event_start` returns an idempotent suffix and strips a synthetic
volatile prefix while preserving the event level and fields, and that
`normalise_event_lines` keeps exactly the matching lines without trailing
whitespace.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@leynos

leynos commented Jul 26, 2026

Copy link
Copy Markdown
Owner

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope).

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Testing (Unit And Behavioural) ⚠️ Warning The new tests snapshot private tracing from internal helpers, not functional outputs or boundary behaviour. Add assertions over observable wrapping/parsing results and edge/error cases at the public entrypoints; keep trace snapshots supplementary.
Testing (Property / Proof) ⚠️ Warning Recommend proptest for normalise_event_lines; it normalises arbitrary trace lines, yet this PR only adds fixed snapshots. Add property tests for normalise_event_lines/stable_event_start over arbitrary log lines to prove prefix stripping and suffix preservation.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== PR metadata ==='
gh pr view 411 --repo leynos/mdtablefix --json number,state,headRefName,baseRefName,commits,files \
  --jq '{number,state,headRefName,baseRefName,commitCount:(.commits|length),changedFiles:[.files[].path]}'

printf '%s\n' '=== Changed files ==='
git diff --name-status "$(git merge-base HEAD origin/HEAD)" HEAD 2>/dev/null || git diff --name-status HEAD~1 HEAD

printf '%s\n' '=== Relevant Rust module outlines ==='
for f in \
  src/wrap/tracing_snapshot_support.rs \
  src/wrap/inline/fragment_tracing_snapshots.rs \
  src/wrap/inline/span_helper_tracing_tests.rs \
  src/wrap/tokenize/parsing_tracing_snapshots.rs \
  src/wrap/inline/fragment.rs \
  src/wrap/tokenize/parsing.rs \
  Cargo.toml
do
  if [ -f "$f" ]; then
    printf '\n--- %s ---\n' "$f"
    case "$f" in
      *.rs) ast-grep outline "$f" --items all || true ;;
      *) sed -n '1,240p' "$f" ;;
    esac
  fi
done

printf '%s\n' '=== Test and proptest-related declarations/usages ==='
rg -n -i -C 3 \
  '(proptest|quickcheck|arbitrary|normalise_event_lines|stable_event_start|parse_link_or_image|InlineFragment|determine_token_span)' \
  -g '*.rs' -g 'Cargo.toml' -g 'Cargo.lock' .

printf '%s\n' '=== Candidate existing follow-up issues ==='
gh issue list --repo leynos/mdtablefix --state all --limit 100 \
  --search 'tracing snapshot normalise normalise_event_lines proptest test' \
  --json number,state,title,url \
  --jq '.[] | "#\(.number) [\(.state)] \(.title) \(.url)"'

Length of output: 117410


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Exact PR diff for test and support changes ==='
git diff "$(git merge-base HEAD origin/HEAD)" HEAD -- \
  src/wrap/tracing_snapshot_support.rs \
  src/wrap/inline/fragment_tracing_snapshots.rs \
  src/wrap/inline/span_helper_tracing_tests.rs \
  src/wrap/tokenize/parsing_tracing_snapshots.rs \
  src/wrap.rs \
  src/wrap/inline/fragment.rs \
  src/wrap/tokenize/parsing.rs

printf '%s\n' '=== Full normalisation helper, including properties ==='
cat -n src/wrap/tracing_snapshot_support.rs

printf '%s\n' '=== Public wrapping and processing entrypoints ==='
ast-grep outline src/wrap.rs src/wrap/inline.rs src/process.rs --items all 2>/dev/null || true
rg -n -C 4 \
  'pub\s+(?:\(crate\)\s+)?fn\s+(wrap_text|wrap_preserving_code|process_stream|process_with_frontmatter)|\b(wrap_text|wrap_preserving_code|process_stream|process_with_frontmatter)\s*\(' \
  src tests -g '*.rs'

printf '%s\n' '=== Existing public-boundary coverage for target cases ==='
rg -n -i -C 5 \
  '(date|footnote|link|image|plain).{0,100}(wrap_text|wrap_preserving_code|process_stream)|(?:wrap_text|wrap_preserving_code|process_stream).{0,100}(date|footnote|link|image|plain)' \
  tests src/wrap -g '*.rs'

Length of output: 201522


@leynos: Treat both warnings as resolved.

Check Status Evidence
Testing (Unit And Behavioural) ✅ Resolved The new snapshot tests now assert concrete results before checking traces: token/cursor outputs for links and footnotes, date-span text and width, fragment classification, and declined footnote coupling. Existing public-boundary suites additionally cover these behaviours through wrap_text and process_stream, including date boundaries and over-width cases, footnote atomicity, link/image preservation, malformed links, and punctuation boundaries. The trace snapshots are supplementary.
Testing (Property / Proof) ✅ Resolved src/wrap/tracing_snapshot_support.rs now adds proptest coverage for arbitrary lines. It proves that stable_event_start returns an idempotent suffix, strips synthetic volatile prefixes while preserving TRACE/DEBUG and all following content, and that normalise_event_lines filters matching lines and removes trailing whitespace.

Do not create a follow-up issue or perform further implementation work for these two warnings. The repository inspection confirms the coverage exists; execute the Rust test suite in CI or a Rust-enabled environment to validate runtime success, since this inspection environment did not run tests.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/wrap/inline/span_helper_tracing_tests.rs`:
- Around line 89-95: Strengthen the assertions after
determine_token_span(&tokens, 0) so the declined-coupling case verifies the
expected non-empty end and _width values, while retaining the assertion that
tokens[..end] excludes "[^note]". Use the expected span and width for this
fixture rather than only checking the slice contents.

In `@src/wrap/tracing_snapshot_support.rs`:
- Around line 97-106: Strengthen the property assertions around
normalise_event_lines by comparing each output segment in order with its
corresponding matching input line after stable-prefix selection and
trailing-whitespace trimming. Keep the existing count and trim checks, and
ensure the assertions validate normalized content rather than only the number of
segments.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a0732a2c-6ba7-4ceb-877d-7f987bc9bc7d

📥 Commits

Reviewing files that changed from the base of the PR and between c55f13f and a004a77.

📒 Files selected for processing (4)
  • src/wrap/inline/fragment_tracing_snapshots.rs
  • src/wrap/inline/span_helper_tracing_tests.rs
  • src/wrap/tokenize/parsing_tracing_snapshots.rs
  • src/wrap/tracing_snapshot_support.rs

Comment thread src/wrap/inline/span_helper_tracing_tests.rs Outdated
Comment thread src/wrap/tracing_snapshot_support.rs
leynos and others added 2 commits July 27, 2026 20:03
Verify the declined-coupling edge case asserts the expected span ("word") and
width (4) for both fixtures, retaining the footnote-exclusion check. Strengthen
the `normalise_event_lines` property test to compare each output segment in
order against the independently derived stable-prefix, trimmed normalisation of
its matching line, keeping the existing count and trim checks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Assert `end == 1` explicitly in the declined-coupling case so the test can no
longer pass with an empty span, alongside the existing span/width checks.
Rework the `normalise_event_lines` property to pair each output segment with
its matching input line: assert the segment equals the prefix-stripped, trimmed
line and is a trailing slice of that line, so the property validates normalised
content rather than only segment count and trimming.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add insta snapshot tests for structured tracing event output

2 participants