Skip to content

Pin public error display output (#8) - #47

Merged
leynos merged 4 commits into
mainfrom
issue-8-add-trybuild-ui-tests-for-public-error-type-display-output
Aug 8, 2026
Merged

Pin public error display output (#8)#47
leynos merged 4 commits into
mainfrom
issue-8-add-trybuild-ui-tests-for-public-error-type-display-output

Conversation

@lodyai

@lodyai lodyai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

This branch pins the public error surface of CliError, DurationParseError,
and ClockConfigError at the crate boundary so refactoring cannot silently
change it. It resolves issue #8 with a trybuild harness that runs in both
modes: pass fixtures pin the user-facing Display messages, and compile-fail
fixtures pin #[non_exhaustive] on all three enums.

Closes #8.

Review walkthrough

Why two modes

Issue #8 asks for compile-fail tests of Display output, which is not
expressible: Rust evaluates Display at runtime, so a compile-fail fixture only
captures compiler diagnostics and never sees a formatted message. Each guarantee
is therefore covered by the mode that can express it.

Pass fixtures are compiled as external crates and then executed, testing the
public boundary and the actual message text. Compile-fail fixtures match every
public variant without a wildcard arm and are expected to fail with E0004,
pinning the contract that keeps adding an error variant a non-breaking change
downstream. A compile-fail fixture that starts passing means an enum has lost
#[non_exhaustive].

Validation

  • make check-fmt: passed.
  • make markdownlint: passed; three helper tests passed with 95.45% coverage and Markdown lint reported no errors.
  • make nixie: passed; all diagrams are valid.
  • make lint: passed rustdoc, Clippy, and Whitaker.
  • make test: passed all 27 tests across six binaries, including the UI harness.
  • make typecheck: passed.
  • cargo test --doc: passed all 14 doctests.
  • coderabbit review --agent: completed after both major milestones with zero findings.

References

Summary by Sourcery

Add executable UI tests to pin user-facing display output of public error types and document their workflow and location.

Build:

  • Add trybuild as a test-only dependency for executable UI fixtures.

Documentation:

  • Document public error UI testing workflow in the developer guide and repository layout.

Tests:

  • Add trybuild-based UI harness and fixtures that execute external crates to assert display strings for CliError, DurationParseError, and ClockConfigError.

leynos added 2 commits August 4, 2026 12:16
Compile and execute external UI fixtures for every public variant of
`CliError`, `DurationParseError`, and `ClockConfigError`. Keep their
user-facing messages stable when the error definitions are refactored.
Explain why the executable `trybuild` fixtures use pass mode, how their
expected strings act as reviewed snapshots, and which commands contributors
must run when adding or intentionally updating public error messages.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

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

Summary

Add executable trybuild UI fixtures for every public CliError, DurationParseError, and ClockConfigError variant. Run fixtures as external crates in pass mode to verify user-facing Display output.

Document the UI test workflow, fixture ownership, snapshot rules, and repository layout. Add trybuild as a test-only dependency and ignore the local .coverage file.

Related issue

Implement the UI-testing objectives from issue #8.

Walkthrough

Add a trybuild harness for executable UI tests. Pin the Display output of three public error types. Document the test layout, fixture rules, focused command, and snapshot update process.

Changes

Public error display tests

Layer / File(s) Summary
UI test harness and repository integration
Cargo.toml, tests/ui.rs, docs/developers-guide.md, docs/repository-layout.md, .gitignore
Add trybuild, execute UI display cases in pass mode, document the test structure and commands, and ignore .coverage.
Public error display cases
tests/ui/*_display.rs
Assert the user-facing Display output for CliError, ClockConfigError, and every DurationParseError variant.

Sequence Diagram(s)

sequenceDiagram
  participant Cargo
  participant Trybuild
  participant DisplayFixtures
  Cargo->>Trybuild: run cargo test --test ui
  Trybuild->>DisplayFixtures: execute *_display.rs cases
  DisplayFixtures-->>Trybuild: assert Display output
  Trybuild-->>Cargo: report test results
Loading

Suggested labels: Issue

Suggested reviewers: leynos

Poem

Pin each message, clear and bright,
Let error text remain stable.
Trybuild checks each public phrase,
Fixtures guard the wording table.
Stable strings now pass the test.


Important

Pre-merge checks failed

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

❌ Failed checks (4 warnings, 2 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR covers trybuild integration, all three error types, standard test execution, and documentation, but uses pass-mode instead of the issue's required compile-fail tests. Add compile-fail trybuild fixtures for all three error types, or obtain explicit acceptance of pass-mode runtime fixtures and update issue #8.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Developer Documentation ⚠️ Warning The developer guide documents trybuild and the fixture workflow, but the living execplan still records 25 tests and no UI milestone while this PR adds the UI harness. Update docs/execplans/initial-app.md with the trybuild/UI milestone, current test count, validation evidence, and outcome before merging.
Testing (Property / Proof) ⚠️ Warning The fixtures cover representative values, but CliError and DurationParseError format arbitrary String payloads; no property-testing tool or recommendation covers that input range. Recommend proptest coverage for arbitrary Unicode, quoting, newline, and boundary-length payloads, while retaining the UI fixtures for exact user-facing wording.
Testing (Unit And Behavioural) ❓ Inconclusive Investigation is still in progress. Gather the test harness semantics and coverage evidence before deciding.
Rust Compiler Lint Integrity ❓ Inconclusive Repository inspection has not yet established the complete pull-request diff or whether the added Rust tests introduce lint or ownership issues. Inspect the commit diff and all changed Rust files, then verify lint suppressions, dead-code boundaries, and clone usage.
✅ Passed checks (14 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changes support issue #8 through tests, dependency setup, documentation, and test-related coverage configuration; no unrelated code changes are shown.
Testing (Overall) ✅ Passed Pass the testing check: trybuild pass mode executes all three external-crate fixtures, which assert exact Display strings for every listed public error variant.
User-Facing Documentation ✅ Passed Treat this check as not applicable: the PR changes no source or user-facing behaviour; it adds test infrastructure and contributor documentation, while docs/users-guide.md remains unchanged.
Module-Level Documentation ✅ Passed All four added Rust modules start with //! documentation stating their purpose; the fixture role and harness relationship are also documented in the developer guide.
Testing (Compile-Time / Ui) ✅ Passed tests/ui.rs runs all external-crate pass fixtures; they assert focused strings for all 5 CliError, 6 DurationParseError, and 1 ClockConfigError variants.
Unit Architecture ✅ Passed Keep the change: it adds isolated external-crate Display fixtures and a trybuild harness, uses public APIs only, and leaves production query, command, clock, and I/O paths unchanged.
Domain Architecture ✅ Passed The pull request changes no domain source; it adds only dev-only trybuild fixtures, lockfile entries, ignore rules, and documentation, with no new adapter or infrastructure dependency.
Observability ✅ Passed Pass: The changes add only trybuild UI tests, documentation, a dev dependency, and .gitignore content; no production runtime behaviour requires new logs, metrics, tracing, or alerts.
Security And Privacy ✅ Passed Review the complete PR diff: it adds only fixed-value display fixtures, documentation, a dev-only trybuild dependency, and a coverage ignore rule; no secrets or privileged/runtime operations appear.
Performance And Resource Use ✅ Passed The pull request adds only three small, fixed-size pass-mode UI fixtures and one harness; no production hot path, loop, cache, queue, blocking operation, or unbounded collection changes.
Concurrency And State ✅ Passed Pass this check: the PR adds isolated trybuild fixtures and local assertions only; it introduces no shared mutable state, async tasks, locks, cancellation, transactions, or ordering logic.
Architectural Complexity And Maintainability ✅ Passed Keep this design: one focused trybuild harness, three direct fixtures, a dev-only dependency, and documented CI usage add no production abstraction or dependency cycle.
Title check ✅ Passed The title accurately describes pinning public error display output and links issue #8 as required.
Description check ✅ Passed The description clearly explains the UI tests, documentation, dependency changes, and validation for the public error display contract.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-8-add-trybuild-ui-tests-for-public-error-type-display-output

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

@sourcery-ai

sourcery-ai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Reviewer's Guide

Pins the user-facing Display output of public error types via executable trybuild-based UI tests and documents the associated workflow and layout, with trybuild added as a test-only dependency.

Sequence diagram for trybuild-based public error Display UI tests

sequenceDiagram
  actor Developer
  participant CargoTest as cargo_test
  participant UIHarness as tests_ui_rs
  participant Trybuild as trybuild_TestCases
  participant CliFixture as cli_error_display_crate
  participant CliError

  Developer->>CargoTest: run_cargo_test_test_ui
  CargoTest->>UIHarness: main
  UIHarness->>Trybuild: TestCases_new
  UIHarness->>Trybuild: pass_cli_error_display_rs
  Trybuild->>CliFixture: compile_crate
  Trybuild->>CliFixture: run_binary
  CliFixture->>CliError: Display
  CliFixture->>Trybuild: assert_eq_expected_message
  Trybuild->>UIHarness: report_pass
  UIHarness->>CargoTest: exit_success
  CargoTest->>Developer: tests_passed
Loading

File-Level Changes

Change Details Files
Add trybuild-based executable UI harness for public error display tests.
  • Introduce tests/ui.rs harness that uses trybuild::TestCases::pass over tests/ui/*_display.rs fixtures.
  • Compile and execute each fixture as an external crate to validate runtime Display output at the public crate boundary.
tests/ui.rs
Add fixtures that pin Display output for CliError, DurationParseError, and ClockConfigError.
  • Create tests/ui/cli_error_display.rs to assert exact CliError message strings.
  • Create tests/ui/duration_parse_error_display.rs to assert exact DurationParseError message strings.
  • Create tests/ui/clock_config_error_display.rs to assert exact ClockConfigError message strings.
tests/ui/cli_error_display.rs
tests/ui/duration_parse_error_display.rs
tests/ui/clock_config_error_display.rs
Document the new public error UI testing workflow and repository layout.
  • Extend developers-guide.md with a Public error UI tests section describing trybuild usage, snapshot expectations, and workflows for adding/updating fixtures.
  • Update repository-layout.md to document tests/ui.rs and tests/ui/ as the locations for public error display fixtures and harness.
docs/developers-guide.md
docs/repository-layout.md
Add trybuild as a test-only dependency and update lockfile.
  • Add trybuild = "1.0.120" under dev-dependencies in Cargo.toml.
  • Regenerate Cargo.lock to include trybuild and its transitive dependencies.
Cargo.toml
Cargo.lock

Assessment against linked issues

Issue Objective Addressed Explanation
#8 Integrate a UI-testing harness (trybuild or equivalent) into the test suite to pin the Display output of public error types CliError, DurationParseError, and ClockConfigError.
#8 Add tests/fixtures that cover and assert the exact Display output for all public variants of CliError, DurationParseError, and ClockConfigError, and ensure they run as part of the standard test/CI pipeline (e.g., via make test or cargo test --test ui).
#8 Document the UI test approach, including how to add or update error message snapshots, in docs/developers-guide.md (and related layout docs if needed).

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Keep the spelling helper's local coverage database out of the working tree
after documentation gates run.
@leynos
leynos marked this pull request as ready for review August 4, 2026 11:18

@sourcery-ai sourcery-ai 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.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
✅ 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 removed the Roadmap label Aug 6, 2026
Issue #8 asked for compile-fail UI tests covering the Display output of the
public error types. Rust evaluates Display at runtime, so a compile-fail
fixture can only snapshot compiler diagnostics and never observes a formatted
message. Pass-mode fixtures remain the only way to pin the message text.

Cover both guarantees instead of choosing one. Add compile-fail fixtures that
match every public variant of CliError, DurationParseError, and
ClockConfigError without a wildcard arm, with .stderr snapshots pinning E0004.
These catch the removal of #[non_exhaustive], which would silently turn every
later variant addition into a breaking change downstream.

Document both modes and the TRYBUILD=overwrite regeneration step, add the
missing docstrings to the fixture helpers, and record the UI milestone,
current test counts, and validation evidence in the living execplan.

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

lodyai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Note on compile-fail vs pass mode

The task list asks for compile-fail UI tests covering the Display output
of the three error types. That specific combination is not expressible: Rust
evaluates Display implementations at runtime, so a compile-fail fixture only
ever captures compiler diagnostics and never observes a formatted message.

Rather than pick one, the branch splits the harness across both trybuild
modes so each guarantee is covered by the mode that can actually express it:

  • Pass fixtures (tests/ui/*_display.rs) compile and execute as external
    crates, asserting the exact message of every public variant of CliError,
    DurationParseError, and ClockConfigError. This is what pins the wording.
  • Compile-fail fixtures (tests/ui/*_non_exhaustive.rs, with adjacent
    .stderr snapshots) match every public variant without a wildcard arm and are
    expected to fail with E0004. This pins #[non_exhaustive] on all three
    enums — the contract that keeps adding an error variant a non-breaking change
    for downstream crates.

Both run under standard Cargo test discovery via tests/ui.rs, so make test
and CI pick them up with no extra step. The workflow for updating either kind of
snapshot — including TRYBUILD=overwrite for the .stderr files and its
coupling to the toolchain pinned in rust-toolchain.toml — is documented in
docs/developers-guide.md.

Flagging this because it deviates from the wording of the tasks above; the
intent of every task is met, but "compile-fail tests of Display output" is not.

@leynos
leynos merged commit adf5624 into main Aug 8, 2026
5 checks passed
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 trybuild UI tests for public error type display output

2 participants