diff --git a/Cargo.lock b/Cargo.lock index 693ea4a2..81e8df48 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3391,6 +3391,8 @@ dependencies = [ "bb8", "bincode", "bytes", + "camino", + "cap-std", "criterion", "dashmap", "derive_more 2.0.1", @@ -3409,6 +3411,7 @@ dependencies = [ "rstest-bdd", "rstest-bdd-macros", "serde", + "serde_json", "serial_test", "socket2 0.6.0", "static_assertions", diff --git a/Cargo.toml b/Cargo.toml index 2f575acd..d4ce8475 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,6 +10,11 @@ keywords = ["async", "networking", "binary-protocol", "protocol", "tokio"] categories = ["network-programming", "asynchronous"] documentation = "https://docs.rs/wireframe" +[workspace] +members = ["."] +default-members = ["."] +resolver = "3" + [lib] name = "wireframe" path = "src/lib.rs" @@ -63,6 +68,9 @@ metrics-util = "0.20.0" tracing-test = "0.2.5" mockall = "0.13.1" criterion = "0.5.1" +cap-std = { version = "3.4.5", features = ["fs_utf8"] } +camino = "1.2.2" +serde_json = "1.0.141" tokio = { version = "1.47.1", default-features = false, features = [ "macros", diff --git a/docs/developers-guide.md b/docs/developers-guide.md index ab243ed7..ca79bb9f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -49,16 +49,47 @@ Message continuation ordering lives in `src/message_assembler/series.rs`. The public-facing control point is `validate_and_advance_sequence()`, which keeps the series state machine readable by delegating to two private helpers: -- `handle_untracked_first_sequence()` handles the first numbered continuation - after an untracked start. It switches the series into tracked mode and - derives the next expected sequence. +- `start_sequence_tracking()` handles the first numbered continuation after an + untracked start. It switches the series into tracked mode and then delegates + to `advance_sequence_or_overflow()` to derive and record the next expected + sequence number. - `advance_tracked_sequence()` handles all later numbered continuations once tracking is active. It rejects duplicates, gaps, and sequence overflow before - advancing the expected sequence. - -This split is intentional. The helpers isolate the untracked-to-tracked -transition from steady-state tracked validation so sequence policy stays flat, -testable, and documentation-friendly during future refactors. + calling `advance_sequence_or_overflow()` to advance the expected sequence. + +`advance_sequence_or_overflow()` is the shared leaf helper: it calls +`checked_increment()` on the incoming sequence and raises +`MessageSeriesError::SequenceOverflow` only when the counter wraps while more +frames are still expected. + +This three-helper split is intentional. `start_sequence_tracking()` isolates +the untracked-to-tracked transition; `advance_tracked_sequence()` owns the +duplicate/gap/out-of-order validation; and `advance_sequence_or_overflow()` +owns the overflow decision. Each piece is independently testable and the +decision tree stays flat and readable during future refactors. + +### `fill_buf_with_prefix` + +The private helper `fill_buf_with_prefix(buf, prefix, endianness)` in +`src/frame/conversion.rs` copies a validated length-prefix byte slice into the +correct position of an 8-byte staging buffer. For big-endian prefixes it places +the bytes at the high end of the buffer (`buf[8 - size..]`); for little-endian +prefixes it places them at the low end (`buf[..size]`). Callers must guarantee +that `prefix.len()` is one of `{1, 2, 4, 8}`; this invariant is enforced +upstream in `bytes_to_u64`, which is why the helper validates the range before +copying and reports an error when the prefix width falls outside the supported +set. + +### `ServerShutdownHandle` + +`ServerShutdownHandle` in `wireframe_testing::client_pair` is a type alias for +the tuple `(oneshot::Sender<()>, JoinHandle>)` that +`PendingServer` stores between server start-up and explicit shutdown. Naming +the alias keeps `PendingServer`'s field types readable and makes +`PendingServer::take` return a self-documenting +`Option` instead of an opaque inline tuple. Tests that +call `WireframePair::shutdown()` do not interact with this type directly; it +is an internal implementation detail of the pair harness. ## Vocabulary normalization outcome (2026-02-20) @@ -81,3 +112,101 @@ Use the Makefile targets as the contributor entrypoint for routine validation: Install Whitaker through the standalone installer described in the [Whitaker user's guide](whitaker-users-guide.md) so local linting matches continuous integration (CI). + +## Cargo workspace semantics + +Wireframe now uses a hybrid root manifest: the repository root `Cargo.toml` +contains both `[package]` and `[workspace]`. + +During roadmap item 10.1.1 the workspace is intentionally staged with only the +root package as a member and default member: + +- `members = ["."]` +- `default-members = ["."]` + +That means ordinary root-level commands such as `cargo build`, `cargo check`, +`cargo test`, and `cargo clippy` retain their existing ergonomics and continue +to target the main `wireframe` package by default. + +Use plain root-level Cargo commands for day-to-day work on the main crate. +Reach for `--workspace` when a task is explicitly meant to cover every current +workspace member, for example repository-wide validation in CI or when later +roadmap items add internal verification crates. + +One Cargo nuance is worth knowing: `cargo metadata` for this repository still +reports the in-tree helper crate `wireframe_testing` in `workspace_members` +because it lives under the repository root as a path dependency. That does not +change day-to-day command ergonomics because `default-members = ["."]` keeps +plain root-level Cargo commands focused on the main `wireframe` package. + +### Workspace manifest test support + +The shared module `tests/common/workspace_manifest_support.rs` keeps the +workspace-contract helpers beside the integration tests that use them. It is a +module, not a library crate, because the code is test-only scaffolding and +should not widen the published crate surface or add another Cargo target. + +The support layer uses `cap-std` with the `fs_utf8` feature for +capability-oriented directory access, `camino` for UTF-8-typed paths, and +`serde_json` for structured assertions over `cargo metadata` output. + +- `repo_root()` locates the repository root as a `Utf8PathBuf`. +- `repo_dir()` opens that root as a `cap_std::fs_utf8::Dir`. +- `root_manifest()` reads the root `Cargo.toml` into a `String`. +- `run_cargo(args)` runs `cargo` in the repository root and returns UTF-8 + stdout, or an error that includes stderr. +- `cargo_metadata()` wraps `cargo metadata --no-deps --format-version 1`. +- `root_package_id()` wraps `cargo pkgid -- wireframe` and trims trailing + whitespace. +- `has_manifest_line(manifest, line)` checks for a complete trimmed line rather + than a substring match. + +`WorkspaceManifestWorld` in `tests/fixtures/workspace_manifest.rs` is the +behaviour-driven development (BDD) fixture for these assertions. Extend it by +loading more workspace-state inputs in `load()` and adding focused verification +methods that the step definitions and scenario can reuse. + +## Test infrastructure and framework + +### rstest and rstest-bdd + +The test suite uses [`rstest`](https://crates.io/crates/rstest) for +fixture-based parametric tests and `rstest-bdd` (via `rstest_bdd_macros`) for +behaviour-driven development (BDD) scenarios expressed in Gherkin. + +Fixtures are plain Rust functions annotated with `#[fixture]`. Inject them into +tests by listing them as parameters; `rstest` constructs each fixture before +running the test body. + +BDD scenarios live in `.feature` files under `tests/features/`. Each file +describes one or more scenarios using the standard Given/When/Then syntax. +Scenario functions are annotated with `#[scenario(path = "…", name = "…")]` and +receive fixture parameters by name. + +### Feature files and step definitions + +Each `.feature` file under `tests/features/` has a corresponding +step-definition module under `tests/steps/`. Step functions are annotated with +`#[given]`, `#[when]`, or `#[then]` and accept a mutable reference to the BDD +world fixture as their first argument. + +Add new scenarios by: + +1. Writing a new Gherkin scenario in the relevant `.feature` file. +2. Implementing the missing step functions in the corresponding + `tests/steps/` module. +3. Adding a scenario function in `tests/scenarios/` that names the new + scenario, injects the fixture, and delegates to a helper that invokes the + step logic in sequence. + +### `LoggerHandle::Default` and `ObservabilityHandle::Default` + +Both `LoggerHandle` (in `wireframe_testing::logging`) and `ObservabilityHandle` +(in `wireframe_testing::observability`) implement `Default`. The `default()` +method delegates to `new()` in each case, providing a convenient way to acquire +a fresh handle without explicitly calling the constructor. + +`LoggerHandle::new()` tolerates a poisoned mutex: if a prior test panicked +while holding the logger lock, `new()` recovers the guard via `into_inner()` +and drains any buffered log records, so the next test starts from a clean +state. diff --git a/docs/execplans/10-1-1-convert-root-manifest-into-hybrid-workspace.md b/docs/execplans/10-1-1-convert-root-manifest-into-hybrid-workspace.md new file mode 100644 index 00000000..d0057b63 --- /dev/null +++ b/docs/execplans/10-1-1-convert-root-manifest-into-hybrid-workspace.md @@ -0,0 +1,403 @@ +# Convert the root manifest into a hybrid workspace + +This ExecPlan is a living document. The sections `Constraints`, `Tolerances`, +`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and +`Outcomes & Retrospective` must be kept up to date as work proceeds. + +Status: IMPLEMENTED (environment-limited validation) + +This document must be maintained in accordance with `AGENTS.md` at the +repository root, including quality gates, test policy, and documentation style +requirements. + +## Purpose / big picture + +Roadmap item 10.1.1 establishes the Cargo layout needed for formal verification +work without disrupting the existing root package or normal contributor +workflow. + +After this change: + +- the repository root `Cargo.toml` is a hybrid manifest containing both + `[package]` and `[workspace]`, +- the root `wireframe` package remains the default workspace member, so plain + root `cargo build`, `cargo test`, `cargo check`, and `cargo clippy` keep + their current ergonomics, +- the repository is ready for follow-on items that add + `crates/wireframe-verification`, Kani, Stateright, and Verus support. + +Observable success is: + +- `cargo metadata --no-deps --format-version 1` reports a workspace rooted at + the repository and shows the root package as the only default member for this + stage. +- `cargo build` passes from the repository root. +- `cargo test --workspace` passes from the repository root. +- New `rstest` integration coverage and `rstest-bdd` behavioural coverage + protect the workspace-default-member contract without recursively invoking + nested `cargo test` runs. +- `docs/formal-verification-methods-in-wireframe.md` records the staged + workspace decision, `docs/developers-guide.md` explains the new contributor + workflow, `docs/users-guide.md` is updated if consumer-facing guidance + changes, and `docs/roadmap.md` marks 10.1.1 done. + +## Constraints + +- Preserve the existing root package metadata and public crate identity in + `Cargo.toml`. +- Keep the root package as the default workspace member for all non-workspace + cargo commands. +- Treat 10.1.1 as infrastructure-only. Do not implement the verification crate, + Kani and Verus tooling, Makefile verification targets, or continuous + integration (CI) jobs owned by roadmap items 10.1.2 through 10.1.5 unless a + minimal compatibility shim is strictly required. +- Prefer no new dependencies. In particular, avoid introducing a crate solely + to inspect Cargo metadata unless a smaller local helper is clearly + insufficient. +- Unit and integration validation must use `rstest`, reusing fixtures where + shared repository-metadata setup exists. +- Behavioural validation must use `rstest-bdd` with the existing + feature/fixture/steps/scenario layout under `tests/`. +- Behavioural tests must not recursively run `cargo build` or `cargo test` + inside `cargo test`; reserve those commands for top-level validation. +- Record the staging decision in the relevant design document: + `docs/formal-verification-methods-in-wireframe.md`. +- Update `docs/developers-guide.md` with any contributor-facing guidance about + hybrid workspace semantics and command usage. +- Update `docs/users-guide.md` if, and only if, the workspace conversion + changes something a library consumer needs to know. +- On completion of the feature, mark roadmap item 10.1.1 done in + `docs/roadmap.md`. + +## Tolerances (exception triggers) + +- Scope: if keeping 10.1.1 separate from 10.1.2 proves impossible without + creating a placeholder `crates/wireframe-verification` package, stop and + decide explicitly whether to collapse roadmap boundaries or relax the member + list temporarily. +- Dependencies: if metadata validation appears to require a new parser crate, + first attempt a minimal local parser or stable string-based assertions; only + add a dependency with recorded rationale. +- Tooling spillover: if the workspace conversion forces Makefile or CI changes + beyond documentation updates, stop and reassess because 10.1.4 and 10.1.5 own + that plumbing. +- Test stability: if metadata-based `rstest-bdd` coverage is flaky across + three hardening attempts, fall back to a simpler manifest-observation + scenario and document the limitation. +- Validation: if `cargo test --workspace` surfaces unrelated pre-existing + failures, record them separately and only proceed once it is clear whether + 10.1.1 caused the regression. + +## Risks + +- Risk: the formal-verification guide currently shows the final end-state + member list (`[".", "crates/wireframe-verification"]`), while the roadmap + splits workspace conversion and verification-crate creation into separate + items. Severity: high. Likelihood: high. Mitigation: document 10.1.1 as a + staged workspace conversion and update the design document to explain when + the verification crate joins the workspace. + +- Risk: path dependencies such as `wireframe = { path = ".", ... }` in + `dev-dependencies` and `wireframe_testing` may behave differently once the + root manifest also acts as a workspace. Severity: medium. Likelihood: medium. + Mitigation: validate `cargo metadata`, `cargo build`, and + `cargo test --workspace` early, before adding regression tests or docs. + +- Risk: repository-metadata tests can become brittle if they assert absolute + package IDs or filesystem paths. Severity: medium. Likelihood: medium. + Mitigation: assert only stable properties such as package names, relative + member counts, and the presence of the root package in default members. + +- Risk: contributors may incorrectly assume that plain `cargo test` now + includes future verification crates. Severity: medium. Likelihood: medium. + Mitigation: update `docs/developers-guide.md` with explicit command + semantics, including when to use `--workspace`. + +## Progress + +- [x] (2026-04-10) Drafted the ExecPlan, reviewed the roadmap item, reviewed + the formal-verification design guidance, and confirmed the current repo + is still a single-package root with `wireframe_testing` outside any + workspace. +- [x] (2026-04-11) Stage A: confirmed the staged 10.1.1 contract remains + "explicit hybrid workspace now, verification crate in 10.1.2" and + verified Cargo can keep `members = ["."]` without a placeholder crate. +- [x] (2026-04-11) Stage B: converted the root `Cargo.toml` into a hybrid + manifest with `[workspace]`, `members = ["."]`, + `default-members = ["."]`, and `resolver = "3"`. +- [x] (2026-04-11) Stage C: added `rstest` and `rstest-bdd` regression + coverage for workspace + invariants. +- [x] (2026-04-11) Stage D: updated the formal-verification guide, updated the + developers' guide, inspected the user guide and determined no consumer + guidance change was required, and marked roadmap item 10.1.1 done. +- [x] (2026-04-11) Stage E: ran the full validation and documentation gates. + Repository checks passed except for environment-limited tooling: + `make lint` remained blocked by the missing `whitaker` wrapper even + though `cargo doc` and + `cargo clippy --all-targets --all-features -- -D warnings` both passed, + and `make fmt` remained blocked because `mdformat-all` requires `fd` in + the execution environment. + +## Surprises & Discoveries + +- Discovery: `docs/formal-verification-methods-in-wireframe.md` already + recommends a hybrid workspace and explicitly calls out + `default-members = ["."]`, but its example shows the later end-state member + list that includes `crates/wireframe-verification`. + +- Discovery: `wireframe_testing` is currently a separate path crate and not + explicitly listed in the `members` array, which means a hybrid workspace + conversion must avoid accidentally implying that all path crates join the + workspace automatically. + +- Discovery: `cargo metadata` already reported the repository root as a + one-member workspace before the explicit `[workspace]` section existed, + because a lone package still produces workspace-shaped metadata. The + regression tests therefore check both the manifest text and the metadata, so + the explicit hybrid-workspace contract is observable. + +- Discovery: once the root manifest became an explicit workspace, Cargo pulled + the in-repository `wireframe_testing` path dependency into + `workspace_members` even with `members = ["."]`. Attempts to isolate that + crate with `exclude` or a nested local workspace were ineffective or invalid, + so the implemented 10.1.1 contract was relaxed to keep the root package as + the sole `default-members` entry while documenting the helper crate's + presence in workspace metadata. + +- Discovery: the `Makefile` already describes `typecheck` as a "workspace + typecheck" even though the repository was not yet an explicit workspace. The + documentation update should align contributor language with the actual Cargo + layout. + +## Decision Log + +- Decision: implement 10.1.1 as a staged workspace conversion. Add + `[workspace]`, `default-members = ["."]`, and `resolver = "3"` in the root + manifest during 10.1.1, but defer adding `crates/wireframe-verification` to + `members` until 10.1.2 creates the crate, unless Cargo forces a placeholder + package earlier. Rationale: the roadmap explicitly separates the workspace + conversion from verification-crate creation, so the implementation should + preserve that granularity and document the staged rollout in the design doc. + Date/Author: 2026-04-10 / Codex. + +- Decision: use `cargo metadata` as the regression-test oracle for workspace + semantics, while keeping `cargo build` and `cargo test --workspace` as + top-level validation commands rather than nested test subprocesses. + Rationale: metadata inspection is stable, fast, and avoids recursive build + execution from inside `cargo test`. Date/Author: 2026-04-10 / Codex. + +## Outcomes & Retrospective + +- Implemented the explicit hybrid workspace in the root `Cargo.toml` without + changing root-level command ergonomics; the root `wireframe` package remains + the sole default workspace member even though Cargo metadata also reports the + in-repository helper crate. +- Added focused `rstest` coverage in `tests/workspace_manifest.rs` and a small + `rstest-bdd` slice covering the same staged-workspace contract. +- Updated the formal-verification guide and developers' guide to document the + two-stage rollout; inspected `docs/users-guide.md` and left it unchanged + because the Cargo layout change does not alter consumer-facing library usage. +- Marked roadmap item 10.1.1 complete after implementation. +- Validation log paths and final gate results have been recorded below. +- Validation log paths: + `/tmp/workspace_manifest_test.log`, `/tmp/workspace_manifest_bdd.log`, + `/tmp/workspace_manifest_metadata.log`, `/tmp/make_check_fmt.log`, + `/tmp/make_markdownlint.log`, `/tmp/make_nixie.log`, `/tmp/make_test.log`, + `/tmp/make_test_doc.log`, `/tmp/make_doctest_benchmark.log`, + `/tmp/make_lint.log`. +- Validation results: + `cargo test --test workspace_manifest` passed. + `cargo test --test bdd --features advanced-tests workspace_manifest` passed. + `cargo metadata --no-deps --format-version 1` passed. `make check-fmt` + passed. `make markdownlint` passed. `make nixie` passed. `make test` passed. + `make test-doc` passed. `make doctest-benchmark` passed. `make fmt` could not + complete because `mdformat-all` requires `fd`, which is absent in the + environment; Markdown formatting was applied manually with `mdtablefix` for + the changed files. `make lint` is blocked by the missing `whitaker` wrapper + binary; the preceding `cargo doc --no-deps` and + `cargo clippy --all-targets --all-features -- -D warnings` steps both passed. + +## Context and orientation + +Relevant repository areas: + +- Before 10.1.1, `Cargo.toml` defined only the root `wireframe` package. +- `wireframe_testing/Cargo.toml` depends on the root crate by path and is not + explicitly listed in the workspace `members` array, even though it may + appear in the `workspace_members` metadata set at runtime. +- `docs/roadmap.md` owns the 10.1.1 acceptance criteria. +- `docs/formal-verification-methods-in-wireframe.md` is the relevant design + document and already defines the eventual hybrid-workspace end state. +- `docs/developers-guide.md` is the correct place for contributor-facing Cargo + workflow guidance. +- `docs/users-guide.md` must be inspected for any consumer-facing fallout, + although none is expected. +- `tests/` already contains both `rstest` integration tests and the + `rstest-bdd` structure under `tests/features/`, `tests/fixtures/`, + `tests/steps/`, and `tests/scenarios/`. + +The most important design nuance for this item is the distinction between the +**stage-1 workspace shape** and the **final formal-verification end state**. +Item 10.1.1 should only make the root manifest workspace-aware and keep the +root package as the default member. Item 10.1.2 should then add the dedicated +verification crate and extend the member list. + +## Plan of work + +### Stage A: confirm the staged workspace contract + +Resolve the staging question before editing files: + +1. Verify that the root manifest can become a hybrid workspace without adding a + placeholder verification crate. +2. Lock the 10.1.1 contract to "hybrid workspace now, verification member in + 10.1.2" unless Cargo proves otherwise. +3. Identify the minimal file set for this item: + `Cargo.toml`, design docs, developer docs, optional user docs, roadmap, and + new regression-test files. + +Go/no-go: proceed only once the staged contract is written into this ExecPlan +and ready to be mirrored into the formal-verification design doc. + +### Stage B: convert the root manifest + +Edit the root manifest to add the workspace section while preserving the root +package. + +Planned manifest changes: + +- add `[workspace]`, +- set `members = ["."]` for 10.1.1, +- set `default-members = ["."]`, +- set `resolver = "3"`. + +Then validate the raw Cargo behaviour immediately: + +- `cargo metadata --no-deps --format-version 1` +- `cargo build` +- `cargo test --workspace` + +If the hybrid layout changes package resolution, path dependency behaviour, or +test target discovery, fix those regressions before moving on to test or docs +work. + +Go/no-go: continue only when the repository builds and the workspace metadata +matches the staged 10.1.1 contract. + +### Stage C: add `rstest` regression coverage + +Add a focused integration test, likely `tests/workspace_manifest.rs`, that uses +`#[rstest]` fixtures to gather and reuse repository metadata. + +The test should validate stable invariants only: + +- the repository now exposes a workspace, +- the root package is a workspace member, +- the root package is the default member, +- 10.1.1 does not unexpectedly widen default-member coverage. + +Prefer a small local metadata helper in `tests/common/` if more than one test +needs to inspect the `cargo metadata` output. Do not invoke nested +`cargo build` or `cargo test` from these tests. + +Go/no-go: continue only when the new `rstest` coverage passes reliably in +isolation. + +### Stage D: add `rstest-bdd` behaviour-driven development (BDD) coverage + +Add a small BDD slice that expresses the contributor-facing behaviour of the +new layout, for example: + +- feature file under `tests/features/workspace_manifest.feature`, +- fixture world under `tests/fixtures/workspace_manifest.rs`, +- step definitions under `tests/steps/workspace_manifest_steps.rs`, +- scenario bindings under `tests/scenarios/workspace_manifest_scenarios.rs`, +- module wiring in `tests/fixtures/mod.rs`, `tests/steps/mod.rs`, and + `tests/scenarios/mod.rs`. + +The scenario should describe the observable contract in plain language: the +repository acts as a hybrid workspace, while the root package remains the +default target for ordinary root-level cargo commands. The implementation +should inspect metadata or other stable manifest observations, not recursively +re-run the full build. + +Go/no-go: continue only when the new BDD coverage compiles and passes alongside +the existing feature suite. + +### Stage E: documentation and roadmap updates + +Update the design and contributor documentation once the manifest and tests are +stable. + +Required documentation work: + +- update `docs/formal-verification-methods-in-wireframe.md` so the + "Root `Cargo.toml` changes" section explains the staged rollout: + `members = ["."]` in 10.1.1, then `[".", "crates/wireframe-verification"]` in + 10.1.2; +- update `docs/developers-guide.md` with hybrid-workspace semantics and clear + guidance on when to use plain cargo commands versus `--workspace`; +- inspect `docs/users-guide.md` and update it only if the workspace change + alters anything a library consumer needs to know; +- mark 10.1.1 done in `docs/roadmap.md` after all gates pass. + +Go/no-go: proceed to final validation only when the docs reflect the actual +implemented contract. + +### Stage F: validation and quality gates + +Run targeted checks first, then the full repository gates. Because long output +is truncated in this environment, every command should use `set -o pipefail` +and `tee` to a log file. + +Targeted validation: + +- `cargo test --test workspace_manifest` +- `cargo test --test bdd workspace_manifest` +- `cargo metadata --no-deps --format-version 1` +- `cargo build` +- `cargo test --workspace` + +Repository gates: + +- `make fmt` +- `make check-fmt` +- `make lint` +- `make test` +- `make test-doc` +- `make doctest-benchmark` +- `make markdownlint` +- `make nixie` + +Record the resulting log paths in `Outcomes & Retrospective` when the feature +is implemented. + +## Concrete file plan + +Expected edits for implementation: + +- `Cargo.toml` +- `docs/formal-verification-methods-in-wireframe.md` +- `docs/developers-guide.md` +- `docs/users-guide.md` if required +- `docs/roadmap.md` +- `tests/workspace_manifest.rs` +- `tests/features/workspace_manifest.feature` +- `tests/fixtures/workspace_manifest.rs` +- `tests/steps/workspace_manifest_steps.rs` +- `tests/scenarios/workspace_manifest_scenarios.rs` +- `tests/fixtures/mod.rs` +- `tests/steps/mod.rs` +- `tests/scenarios/mod.rs` + +Files that should remain out of scope for 10.1.1 unless a blocker appears: + +- `crates/wireframe-verification/**` +- `tools/kani/**` +- `tools/verus/**` +- `scripts/install-kani.sh` +- `scripts/install-verus.sh` +- `scripts/run-verus.sh` +- `.github/workflows/**` +- `Makefile` verification targets owned by 10.1.4 diff --git a/docs/formal-verification-methods-in-wireframe.md b/docs/formal-verification-methods-in-wireframe.md index 444409fe..4e88441a 100644 --- a/docs/formal-verification-methods-in-wireframe.md +++ b/docs/formal-verification-methods-in-wireframe.md @@ -319,7 +319,7 @@ Add a workspace section to the root manifest: ```toml [workspace] -members = [".", "crates/wireframe-verification"] +members = ["."] default-members = ["."] resolver = "3" ``` @@ -333,6 +333,27 @@ crate by accident.[^26] That is a small but important stability choice. +One nuance matters here: Cargo metadata for this repository still reports the +in-repository helper crate `wireframe_testing` in `workspace_members` once the +root manifest becomes an explicit workspace, even though the staged +`members = ["."]` list only names the root package. The practical 10.1.1 +contract is therefore: + +- the root manifest is explicitly workspace-aware, +- the root `wireframe` package remains the sole `default-members` entry, and +- the dedicated verification crate is still deferred until 10.1.2. + +This should land in two stages: + +1. Roadmap item 10.1.1 adds the explicit hybrid workspace with + `members = ["."]`. +2. Roadmap item 10.1.2 extends the member list to + `[".", "crates/wireframe-verification"]` when the verification crate exists. + +That staging keeps the Cargo layout change separate from the introduction of +new verification code and avoids a placeholder crate solely to satisfy the +workspace manifest. + ## Kani integration ### Kani tooling model diff --git a/docs/roadmap.md b/docs/roadmap.md index d42e6922..7da8c207 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -580,7 +580,7 @@ Wireframe's protocol, framing, and message assembly layers. ### 15.1. Verification workspace and tooling -- [ ] 15.1.1. Convert the root manifest into a hybrid workspace while keeping +- [x] 15.1.1. Convert the root manifest into a hybrid workspace while keeping the root package as the default member. See [formal-verification-methods-in-wireframe.md §Root `Cargo.toml` changes](formal-verification-methods-in-wireframe.md#root-cargotoml-changes). Success criteria: `cargo build` and `cargo test --workspace` pass with the diff --git a/docs/wireframe-testing-crate.md b/docs/wireframe-testing-crate.md index f777ce41..fde82dea 100644 --- a/docs/wireframe-testing-crate.md +++ b/docs/wireframe-testing-crate.md @@ -422,6 +422,34 @@ Tests using `ObservabilityHandle` should not run concurrently; the global lock serializes access, so favour a shared fixture or a dedicated serial test binary for observability assertions. +### `Default` implementations + +Both `LoggerHandle` and `ObservabilityHandle` implement `Default`. The +`default()` method delegates to `new()` in each case, so the two forms are +equivalent: + +```rust,no_run +use wireframe_testing::{LoggerHandle, observability::ObservabilityHandle}; + +// These two lines are identical in effect: +let handle = LoggerHandle::default(); +let handle = LoggerHandle::new(); + +let obs = ObservabilityHandle::default(); +let obs = ObservabilityHandle::new(); +``` + +`Default::default()` (or a derived `#[derive(Default)]` on a fixture struct) +fits cases where the handle is a field of a larger fixture world and the +standard initialization machinery should construct it automatically. `new()` +fits cases where an explicit construction point with a visible call site is +required, for example during recovery from a poisoned mutex in test teardown. + +`LoggerHandle::new()` is poison-tolerant: if a prior test panicked while +holding the shared logger mutex, `new()` recovers the guard and drains any +stale log records before returning the handle, so the next test begins from a +clean state. + The codec-error regression coverage added for roadmap item 9.7.4 establishes a recommended pattern for taxonomy assertions: diff --git a/src/frame/conversion.rs b/src/frame/conversion.rs index 85ef32dd..caa7418d 100644 --- a/src/frame/conversion.rs +++ b/src/frame/conversion.rs @@ -29,31 +29,40 @@ fn checked_prefix_cast>(len: usize) -> io::Result { T::try_from(len).map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, ERR_FRAME_TOO_LARGE)) } -/// Copies `prefix` into an 8-byte buffer, right-aligned for big-endian or -/// left-aligned for little-endian. +/// Copy `prefix` into the appropriate slice of `buf` for the given endianness. /// -/// The caller guarantees that `prefix` is `1`, `2`, `4`, or `8` bytes, so the -/// slice operations are infallible. -fn fill_prefix_buffer(prefix: &[u8], endianness: Endianness) -> [u8; 8] { - let mut buf = [0u8; 8]; +/// Callers must guarantee that `prefix.len()` is one of `{1, 2, 4, 8}`. +#[inline] +fn fill_buf_with_prefix( + buf: &mut [u8; 8], + prefix: &[u8], + endianness: Endianness, +) -> io::Result<()> { let size = prefix.len(); - debug_assert!( - matches!(size, 1 | 2 | 4 | 8), - "prefix must be 1, 2, 4, or 8 bytes" - ); - let offset = match endianness { - Endianness::Big => 8 - size, - Endianness::Little => 0, - }; - let dst = buf.get_mut(offset..offset + size); - debug_assert!( - dst.is_some(), - "validated prefix length always fits within the buffer" - ); - if let Some(dst) = dst { - dst.copy_from_slice(prefix); + if !matches!(size, 1 | 2 | 4 | 8) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + ERR_UNSUPPORTED_PREFIX, + )); } - buf + let target = match endianness { + Endianness::Big => { + let start = 8usize.checked_sub(size).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX) + })?; + let end = start.checked_add(size).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX) + })?; + buf.get_mut(start..end).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX) + })? + } + Endianness::Little => buf + .get_mut(..size) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX))?, + }; + target.copy_from_slice(prefix); + Ok(()) } /// Converts a byte slice into a `u64` according to `size` and `endianness`. @@ -81,7 +90,8 @@ pub fn bytes_to_u64(bytes: &[u8], size: usize, endianness: Endianness) -> io::Re let prefix = bytes .get(..size) .ok_or_else(|| io::Error::new(io::ErrorKind::UnexpectedEof, ERR_INCOMPLETE_PREFIX))?; - let buf = fill_prefix_buffer(prefix, endianness); + let mut buf = [0u8; 8]; + fill_buf_with_prefix(&mut buf, prefix, endianness)?; // Wire prefix declares its endianness; normalising into an 8-byte buffer and // using explicit conversion helpers keeps decoding deterministic on any host @@ -191,3 +201,48 @@ pub fn u64_to_bytes( Ok(size) } + +#[cfg(test)] +mod tests { + //! Unit tests for prefix-buffer placement across supported endianness cases. + + use rstest::rstest; + + use super::*; + use crate::frame::format::Endianness; + + #[rstest] + #[case(&[0xab], Endianness::Big, [0, 0, 0, 0, 0, 0, 0, 0xab])] + #[case(&[0x01, 0x02], Endianness::Big, [0, 0, 0, 0, 0, 0, 0x01, 0x02])] + #[case( + &[0x01, 0x02, 0x03, 0x04], + Endianness::Big, + [0, 0, 0, 0, 0x01, 0x02, 0x03, 0x04] + )] + #[case( + &[1, 2, 3, 4, 5, 6, 7, 8], + Endianness::Big, + [1, 2, 3, 4, 5, 6, 7, 8] + )] + #[case(&[0xcd], Endianness::Little, [0xcd, 0, 0, 0, 0, 0, 0, 0])] + #[case(&[0x01, 0x02], Endianness::Little, [0x01, 0x02, 0, 0, 0, 0, 0, 0])] + #[case( + &[0x01, 0x02, 0x03, 0x04], + Endianness::Little, + [0x01, 0x02, 0x03, 0x04, 0, 0, 0, 0] + )] + #[case( + &[1, 2, 3, 4, 5, 6, 7, 8], + Endianness::Little, + [1, 2, 3, 4, 5, 6, 7, 8] + )] + fn fill_buf_with_prefix_copies_bytes_into_expected_region( + #[case] input: &[u8], + #[case] endianness: Endianness, + #[case] expected: [u8; 8], + ) { + let mut buf = [0u8; 8]; + fill_buf_with_prefix(&mut buf, input, endianness).expect("prefix copy should succeed"); + assert_eq!(buf, expected); + } +} diff --git a/src/message_assembler/series.rs b/src/message_assembler/series.rs index 527b5006..55ebc96e 100644 --- a/src/message_assembler/series.rs +++ b/src/message_assembler/series.rs @@ -197,28 +197,36 @@ impl MessageSeries { Ok(MessageSeriesStatus::Incomplete) } - /// Starts explicit sequence tracking when the first numbered continuation - /// arrives after an untracked series. + /// Advance the expected sequence number, returning an error on overflow + /// when more frames are still expected. + fn advance_sequence_or_overflow( + &mut self, + incoming: FrameSequence, + is_last: bool, + ) -> Result<(), MessageSeriesError> { + let new_next = incoming.checked_increment(); + if new_next.is_none() && !is_last { + return Err(MessageSeriesError::SequenceOverflow { last: incoming }); + } + self.next_sequence = new_next; + Ok(()) + } + + /// Transition from untracked to tracked on the first sequenced frame. /// /// The helper records the next expected sequence number and reports /// overflow only when additional frames are still required. - fn handle_untracked_first_sequence( + fn start_sequence_tracking( &mut self, incoming: FrameSequence, is_last: bool, ) -> Result<(), MessageSeriesError> { - // First continuation with a sequence number; start tracking. + self.advance_sequence_or_overflow(incoming, is_last)?; self.sequence_tracking = SequenceTracking::Tracked; - self.next_sequence = incoming.checked_increment(); - // Overflow is only an error if more frames are expected. - if self.next_sequence.is_none() && !is_last { - return Err(MessageSeriesError::SequenceOverflow { last: incoming }); - } Ok(()) } - /// Validates a numbered continuation against the tracked expectation and - /// advances the series state when the sequence is correct. + /// Validate and advance the sequence number while tracking is active. /// /// The helper rejects duplicates, gaps, and unexpected overflow so /// `validate_and_advance_sequence` can delegate the tracked path without @@ -231,7 +239,6 @@ impl MessageSeries { let expected = self .next_sequence .ok_or(MessageSeriesError::SequenceOverflow { last: incoming })?; - if incoming.0 < expected.0 { // Duplicate: sequence already seen. return Err(MessageSeriesError::DuplicateFrame { @@ -239,7 +246,6 @@ impl MessageSeries { sequence: incoming, }); } - if incoming != expected { // Out of order or gap. return Err(MessageSeriesError::SequenceMismatch { @@ -247,14 +253,7 @@ impl MessageSeries { found: incoming, }); } - - // Advance expected sequence. - self.next_sequence = incoming.checked_increment(); - // Overflow is only an error if more frames are expected. - if self.next_sequence.is_none() && !is_last { - return Err(MessageSeriesError::SequenceOverflow { last: incoming }); - } - Ok(()) + self.advance_sequence_or_overflow(incoming, is_last) } fn validate_and_advance_sequence( @@ -263,7 +262,7 @@ impl MessageSeries { is_last: bool, ) -> Result<(), MessageSeriesError> { match self.sequence_tracking { - SequenceTracking::Untracked => self.handle_untracked_first_sequence(incoming, is_last), + SequenceTracking::Untracked => self.start_sequence_tracking(incoming, is_last), SequenceTracking::Tracked => self.advance_tracked_sequence(incoming, is_last), } } @@ -277,3 +276,6 @@ impl MessageSeries { self.next_sequence = Some(next); } } + +#[cfg(test)] +mod tests; diff --git a/src/message_assembler/series/tests.rs b/src/message_assembler/series/tests.rs new file mode 100644 index 00000000..c796c88f --- /dev/null +++ b/src/message_assembler/series/tests.rs @@ -0,0 +1,180 @@ +//! Unit and property tests for sequence tracking helper logic. + +use super::*; +use crate::message_assembler::{FirstFrameHeader, FrameSequence, MessageKey}; + +fn make_series() -> MessageSeries { + MessageSeries::from_first_frame(&FirstFrameHeader { + message_key: MessageKey(1), + metadata_len: 0, + body_len: 10, + total_body_len: None, + is_last: false, + }) +} + +#[test] +fn advance_sequence_or_overflow_advances_on_normal_increment() { + let mut series = make_series(); + let seq = FrameSequence(1); + assert!(series.advance_sequence_or_overflow(seq, false).is_ok()); + assert_eq!(series.next_sequence, Some(FrameSequence(2))); +} + +#[test] +fn advance_sequence_or_overflow_allows_overflow_on_last_frame() { + let mut series = make_series(); + let seq = FrameSequence(u32::MAX); + assert!(series.advance_sequence_or_overflow(seq, true).is_ok()); + assert_eq!(series.next_sequence, None); +} + +#[test] +fn advance_sequence_or_overflow_errors_on_overflow_mid_stream() { + let mut series = make_series(); + let seq = FrameSequence(u32::MAX); + let result = series.advance_sequence_or_overflow(seq, false); + assert!(matches!( + result, + Err(MessageSeriesError::SequenceOverflow { last }) if last == seq + )); +} + +#[test] +fn start_sequence_tracking_switches_to_tracked_mode() { + let mut series = make_series(); + assert_eq!(series.sequence_tracking, SequenceTracking::Untracked); + let seq = FrameSequence(0); + let result = series.start_sequence_tracking(seq, false); + assert!(result.is_ok()); + assert_eq!(series.sequence_tracking, SequenceTracking::Tracked); + assert_eq!(series.next_sequence, Some(FrameSequence(1))); +} + +#[test] +fn start_sequence_tracking_delegates_overflow_on_last() { + let mut series = make_series(); + let seq = FrameSequence(u32::MAX); + assert!(series.start_sequence_tracking(seq, true).is_ok()); + assert_eq!(series.sequence_tracking, SequenceTracking::Tracked); +} + +#[test] +fn advance_tracked_sequence_accepts_expected_sequence() { + let mut series = make_series(); + series.force_next_sequence_for_tests(FrameSequence(3)); + assert!( + series + .advance_tracked_sequence(FrameSequence(3), false) + .is_ok() + ); + assert_eq!(series.next_sequence, Some(FrameSequence(4))); +} + +#[test] +fn advance_tracked_sequence_rejects_duplicate() { + let mut series = make_series(); + series.force_next_sequence_for_tests(FrameSequence(5)); + let result = series.advance_tracked_sequence(FrameSequence(2), false); + assert!(matches!( + result, + Err(MessageSeriesError::DuplicateFrame { .. }) + )); +} + +#[test] +fn advance_tracked_sequence_rejects_out_of_order() { + let mut series = make_series(); + series.force_next_sequence_for_tests(FrameSequence(3)); + let result = series.advance_tracked_sequence(FrameSequence(7), false); + assert!(matches!( + result, + Err(MessageSeriesError::SequenceMismatch { .. }) + )); +} + +#[test] +fn start_sequence_tracking_overflow_mid_stream_preserves_state() { + let mut series = make_series(); + assert_eq!(series.sequence_tracking, SequenceTracking::Untracked); + assert_eq!(series.next_sequence, None); + let result = series.start_sequence_tracking(FrameSequence(u32::MAX), false); + assert!(matches!( + result, + Err(MessageSeriesError::SequenceOverflow { .. }) + )); + assert_eq!(series.sequence_tracking, SequenceTracking::Untracked); + assert_eq!(series.next_sequence, None); +} + +mod property { + //! Property tests for sequence helper boundary conditions. + + use proptest::prelude::*; + + use super::super::*; + use crate::message_assembler::{FirstFrameHeader, FrameSequence, MessageKey}; + + fn make_series() -> MessageSeries { + MessageSeries::from_first_frame(&FirstFrameHeader { + message_key: MessageKey(1), + metadata_len: 0, + body_len: 10, + total_body_len: None, + is_last: false, + }) + } + + proptest! { + /// Any sequence strictly below the maximum always advances without an + /// overflow error when `is_last` is `false`. + #[test] + fn advance_sequence_or_overflow_no_error_below_max( + seq in 0u16..u16::MAX, + ) { + let mut s = make_series(); + let incoming = FrameSequence(u32::from(seq)); + let result = s.advance_sequence_or_overflow(incoming, false); + prop_assert!(result.is_ok()); + } + + /// `start_sequence_tracking` always switches to Tracked mode for any + /// non-overflowing sequence value. + #[test] + fn start_sequence_tracking_always_enters_tracked_mode(seq in 0u16..u16::MAX) { + let mut s = make_series(); + prop_assert_eq!(s.sequence_tracking, SequenceTracking::Untracked); + let incoming = FrameSequence(u32::from(seq)); + let result = s.start_sequence_tracking(incoming, false); + prop_assert!(result.is_ok()); + prop_assert_eq!(s.sequence_tracking, SequenceTracking::Tracked); + } + + /// `advance_tracked_sequence` accepts exactly the expected sequence for + /// any non-overflowing value. + #[test] + fn advance_tracked_sequence_accepts_exact_match(expected in 1u16..u16::MAX) { + let mut s = make_series(); + let exp = FrameSequence(u32::from(expected)); + s.force_next_sequence_for_tests(exp); + let result = s.advance_tracked_sequence(exp, false); + prop_assert!(result.is_ok()); + } + + /// Any sequence strictly below `expected` is treated as a duplicate. + #[test] + fn advance_tracked_sequence_duplicate_below_expected( + expected in 2u16..u16::MAX, + delta in 1u16..10u16, + ) { + let exp = expected; + let below = exp.saturating_sub(delta); + prop_assume!(below < exp); + let mut s = make_series(); + s.force_next_sequence_for_tests(FrameSequence(u32::from(exp))); + let result = s.advance_tracked_sequence(FrameSequence(u32::from(below)), false); + let is_duplicate = matches!(result, Err(MessageSeriesError::DuplicateFrame { .. })); + prop_assert!(is_duplicate); + } + } +} diff --git a/tests/bdd/mod.rs b/tests/bdd/mod.rs index e97458b7..96da7f36 100644 --- a/tests/bdd/mod.rs +++ b/tests/bdd/mod.rs @@ -9,6 +9,9 @@ #[path = "../common/terminator.rs"] mod terminator; +#[path = "../common/workspace_manifest_support.rs"] +mod workspace_manifest_support; + #[path = "../support.rs"] mod support; diff --git a/tests/common/workspace_manifest_support.rs b/tests/common/workspace_manifest_support.rs new file mode 100644 index 00000000..72089032 --- /dev/null +++ b/tests/common/workspace_manifest_support.rs @@ -0,0 +1,59 @@ +//! Shared helpers for workspace-manifest integration and behavioural tests. + +use std::{env, process::Command}; + +use camino::Utf8PathBuf; +use cap_std::{ambient_authority, fs_utf8::Dir}; + +pub(crate) type WorkspaceManifestResult = + Result>; + +pub(crate) fn repo_root() -> WorkspaceManifestResult { + Utf8PathBuf::from_path_buf(env::current_dir()?).map_err(|path| { + format!( + "repository root path is not valid UTF-8: {}", + path.display() + ) + .into() + }) +} + +pub(crate) fn repo_dir() -> WorkspaceManifestResult { + Ok(Dir::open_ambient_dir(repo_root()?, ambient_authority())?) +} + +pub(crate) fn root_manifest() -> WorkspaceManifestResult { + Ok(repo_dir()?.read_to_string("Cargo.toml")?) +} + +pub(crate) fn run_cargo(args: &[&str]) -> WorkspaceManifestResult { + let command_args = if args.is_empty() { + "".to_owned() + } else { + args.join(" ") + }; + let output = Command::new("cargo") + .args(args) + .current_dir(repo_root()?) + .output()?; + if !output.status.success() { + return Err(format!( + "`cargo {command_args}` failed: {}", + String::from_utf8_lossy(&output.stderr) + ) + .into()); + } + Ok(String::from_utf8(output.stdout)?) +} + +pub(crate) fn cargo_metadata() -> WorkspaceManifestResult { + run_cargo(&["metadata", "--no-deps", "--format-version", "1"]) +} + +pub(crate) fn root_package_id() -> WorkspaceManifestResult { + run_cargo(&["pkgid", "--", "wireframe"]).map(|stdout| stdout.trim().to_owned()) +} + +pub(crate) fn has_manifest_line(manifest: &str, expected: &str) -> bool { + manifest.lines().any(|line| line.trim() == expected) +} diff --git a/tests/features/workspace_manifest.feature b/tests/features/workspace_manifest.feature new file mode 100644 index 00000000..c2ac94c7 --- /dev/null +++ b/tests/features/workspace_manifest.feature @@ -0,0 +1,10 @@ +Feature: Hybrid workspace manifest + The repository should expose an explicit hybrid Cargo workspace while keeping + the root package as the only default member during roadmap item 10.1.1. + + Scenario: The root manifest stages the hybrid workspace conversion + Given the repository workspace metadata is loaded + Then the root Cargo manifest declares the staged hybrid workspace + And the workspace metadata reports the root package as a workspace member + And the workspace metadata reports the root package as the only default member + And the workspace metadata does not include the verification crate yet diff --git a/tests/fixtures/mod.rs b/tests/fixtures/mod.rs index bc3d2439..60237530 100644 --- a/tests/fixtures/mod.rs +++ b/tests/fixtures/mod.rs @@ -45,3 +45,4 @@ pub mod streaming_request; pub mod test_observability; pub mod testkit_export; pub mod unified_codec; +pub mod workspace_manifest; diff --git a/tests/fixtures/workspace_manifest.rs b/tests/fixtures/workspace_manifest.rs new file mode 100644 index 00000000..c2f666dd --- /dev/null +++ b/tests/fixtures/workspace_manifest.rs @@ -0,0 +1,179 @@ +//! Fixture world for workspace-manifest behavioural scenarios. + +use rstest::fixture; +use serde_json::Value; + +use crate::workspace_manifest_support::{ + WorkspaceManifestResult as FixtureResult, + cargo_metadata, + has_manifest_line, + root_manifest, + root_package_id, +}; + +pub type TestResult = FixtureResult<()>; +const ROOT_PACKAGE_NAME: &str = "wireframe"; +const HELPER_PACKAGE_NAME: &str = "wireframe_testing"; +const VERIFICATION_PACKAGE_NAME: &str = "wireframe-verification"; + +/// BDD world holding the root manifest and `cargo metadata` output. +#[derive(Debug, Default)] +pub struct WorkspaceManifestWorld { + manifest: Option, + metadata: Option, + package_id: Option, +} + +#[rustfmt::skip] +#[fixture] +pub fn workspace_manifest_world() -> WorkspaceManifestWorld { + std::hint::black_box(()); + WorkspaceManifestWorld::default() +} + +impl WorkspaceManifestWorld { + /// Load the repository manifest and workspace metadata for later checks. + /// + /// # Errors + /// + /// Returns an error when the manifest cannot be read or `cargo metadata` + /// fails. + pub fn load(&mut self) -> TestResult { + self.manifest = Some(root_manifest()?); + self.metadata = Some(cargo_metadata()?); + self.package_id = Some(root_package_id()?); + Ok(()) + } + + fn manifest(&self) -> Result<&str, String> { + self.manifest + .as_deref() + .ok_or_else(|| "workspace manifest not loaded".to_owned()) + } + + fn metadata(&self) -> Result<&str, String> { + self.metadata + .as_deref() + .ok_or_else(|| "workspace metadata not loaded".to_owned()) + } + + fn package_id(&self) -> Result<&str, String> { + self.package_id + .as_deref() + .ok_or_else(|| "workspace package id not loaded".to_owned()) + } + + fn metadata_json(&self) -> FixtureResult { Ok(serde_json::from_str(self.metadata()?)?) } + + fn metadata_array<'a>(metadata: &'a Value, field: &str) -> Result<&'a [Value], String> { + metadata + .get(field) + .and_then(Value::as_array) + .map(Vec::as_slice) + .ok_or_else(|| format!("cargo metadata should expose {field} as an array")) + } + + fn packages(metadata: &Value) -> Result<&[Value], String> { + Self::metadata_array(metadata, "packages") + } + + fn packages_include_name(metadata: &Value, name: &str) -> Result { + Ok(Self::packages(metadata)? + .iter() + .any(|package| package.get("name").and_then(Value::as_str) == Some(name))) + } + + /// Verify the root manifest declares the staged hybrid workspace contract. + /// + /// # Errors + /// + /// Returns an error when the manifest is missing an expected workspace + /// clause. + pub fn verify_staged_hybrid_workspace_manifest(&self) -> TestResult { + let manifest = self.manifest()?; + for expected in [ + "[workspace]", + "members = [\".\"]", + "default-members = [\".\"]", + "resolver = \"3\"", + ] { + if !has_manifest_line(manifest, expected) { + return Err(format!("expected `{expected}` in root Cargo.toml").into()); + } + } + Ok(()) + } + + /// Verify the root package remains part of the workspace membership. + /// + /// # Errors + /// + /// Returns an error when the metadata omits the root package. + pub fn verify_root_is_workspace_member(&self) -> TestResult { + let package_id = self.package_id()?; + let metadata = self.metadata_json()?; + let workspace_members = Self::metadata_array(&metadata, "workspace_members")?; + if !workspace_members + .iter() + .any(|member| member.as_str() == Some(package_id)) + { + return Err(format!( + "workspace metadata did not include the root package id in workspace_members: \ + {workspace_members:?}" + ) + .into()); + } + if !Self::packages_include_name(&metadata, ROOT_PACKAGE_NAME)? { + return Err(format!( + "cargo metadata packages did not include the root package name \ + `{ROOT_PACKAGE_NAME}`" + ) + .into()); + } + Ok(()) + } + + /// Verify the root package is the only default workspace member. + /// + /// # Errors + /// + /// Returns an error when the metadata widens default-member coverage. + pub fn verify_root_is_only_default_member(&self) -> TestResult { + let package_id = self.package_id()?; + let metadata = self.metadata_json()?; + let workspace_default_members = + Self::metadata_array(&metadata, "workspace_default_members")?; + if workspace_default_members.len() != 1 + || workspace_default_members.first().and_then(Value::as_str) != Some(package_id) + { + return Err(format!( + "workspace metadata did not keep the root package as the only default member: \ + {workspace_default_members:?}" + ) + .into()); + } + Ok(()) + } + + /// Verify the staged rollout has not yet added the verification crate. + /// + /// # Errors + /// + /// Returns an error when the 10.1.2 crate appears too early or the helper + /// crate unexpectedly disappears from Cargo metadata. + pub fn verify_verification_crate_is_absent(&self) -> TestResult { + let metadata = self.metadata_json()?; + if Self::packages_include_name(&metadata, VERIFICATION_PACKAGE_NAME)? { + return Err( + "verification crate should not join the workspace until roadmap item 10.1.2".into(), + ); + } + if !Self::packages_include_name(&metadata, HELPER_PACKAGE_NAME)? { + return Err( + "workspace metadata should continue to report the in-repository helper crate" + .into(), + ); + } + Ok(()) + } +} diff --git a/tests/scenarios/mod.rs b/tests/scenarios/mod.rs index 0b1b3b15..310f7fbc 100644 --- a/tests/scenarios/mod.rs +++ b/tests/scenarios/mod.rs @@ -47,3 +47,4 @@ mod streaming_request_scenarios; mod test_observability_scenarios; mod testkit_export_scenarios; mod unified_codec_scenarios; +mod workspace_manifest_scenarios; diff --git a/tests/scenarios/workspace_manifest_scenarios.rs b/tests/scenarios/workspace_manifest_scenarios.rs new file mode 100644 index 00000000..25ed1d44 --- /dev/null +++ b/tests/scenarios/workspace_manifest_scenarios.rs @@ -0,0 +1,30 @@ +//! Scenario tests for workspace-manifest behaviours. + +use rstest_bdd_macros::scenario; + +use crate::fixtures::workspace_manifest::{ + TestResult, + WorkspaceManifestWorld, + workspace_manifest_world, +}; + +fn assert_root_manifest_stages_hybrid_workspace( + workspace_manifest_world: &mut WorkspaceManifestWorld, +) -> TestResult { + workspace_manifest_world.load()?; + workspace_manifest_world.verify_staged_hybrid_workspace_manifest()?; + workspace_manifest_world.verify_root_is_workspace_member()?; + workspace_manifest_world.verify_root_is_only_default_member()?; + workspace_manifest_world.verify_verification_crate_is_absent()?; + Ok(()) +} + +#[scenario( + path = "tests/features/workspace_manifest.feature", + name = "The root manifest stages the hybrid workspace conversion" +)] +fn root_manifest_stages_hybrid_workspace(workspace_manifest_world: WorkspaceManifestWorld) { + let mut workspace_manifest_world = workspace_manifest_world; + assert_root_manifest_stages_hybrid_workspace(&mut workspace_manifest_world) + .expect("workspace manifest scenario should pass"); +} diff --git a/tests/steps/mod.rs b/tests/steps/mod.rs index ffbf1eba..859a4572 100644 --- a/tests/steps/mod.rs +++ b/tests/steps/mod.rs @@ -43,3 +43,4 @@ mod streaming_request_steps; mod test_observability_steps; mod testkit_export_steps; mod unified_codec_steps; +mod workspace_manifest_steps; diff --git a/tests/steps/workspace_manifest_steps.rs b/tests/steps/workspace_manifest_steps.rs new file mode 100644 index 00000000..e4a35bc8 --- /dev/null +++ b/tests/steps/workspace_manifest_steps.rs @@ -0,0 +1,40 @@ +//! Step definitions for workspace-manifest behavioural tests. + +use rstest_bdd_macros::{given, then}; + +use crate::fixtures::workspace_manifest::{TestResult, WorkspaceManifestWorld}; + +#[given("the repository workspace metadata is loaded")] +fn given_repository_workspace_metadata_is_loaded( + workspace_manifest_world: &mut WorkspaceManifestWorld, +) -> TestResult { + workspace_manifest_world.load() +} + +#[then("the root Cargo manifest declares the staged hybrid workspace")] +fn then_root_manifest_declares_staged_hybrid_workspace( + workspace_manifest_world: &mut WorkspaceManifestWorld, +) -> TestResult { + workspace_manifest_world.verify_staged_hybrid_workspace_manifest() +} + +#[then("the workspace metadata reports the root package as a workspace member")] +fn then_workspace_metadata_reports_workspace_member( + workspace_manifest_world: &mut WorkspaceManifestWorld, +) -> TestResult { + workspace_manifest_world.verify_root_is_workspace_member() +} + +#[then("the workspace metadata reports the root package as the only default member")] +fn then_workspace_metadata_reports_only_default_member( + workspace_manifest_world: &mut WorkspaceManifestWorld, +) -> TestResult { + workspace_manifest_world.verify_root_is_only_default_member() +} + +#[then("the workspace metadata does not include the verification crate yet")] +fn then_workspace_metadata_excludes_verification_crate( + workspace_manifest_world: &mut WorkspaceManifestWorld, +) -> TestResult { + workspace_manifest_world.verify_verification_crate_is_absent() +} diff --git a/tests/workspace_manifest.rs b/tests/workspace_manifest.rs new file mode 100644 index 00000000..b2c04d05 --- /dev/null +++ b/tests/workspace_manifest.rs @@ -0,0 +1,99 @@ +//! Regression tests for the staged hybrid-workspace manifest contract. +//! +//! These checks verify that the repository advertises an explicit hybrid +//! workspace while keeping the root package as the only default member during +//! roadmap item 10.1.1. + +#[path = "common/workspace_manifest_support.rs"] +mod workspace_manifest_support; + +use rstest::rstest; +use serde_json::Value; +use workspace_manifest_support::{ + WorkspaceManifestResult as TestResult, + cargo_metadata, + has_manifest_line, + repo_root, + root_manifest, + root_package_id, +}; + +fn parse_metadata_json(metadata: &str) -> TestResult { Ok(serde_json::from_str(metadata)?) } + +fn contains_json_string_field(json: &str, field: &str, value: &str) -> bool { + let escaped = value.replace('\\', "\\\\").replace('"', "\\\""); + json.contains(&format!("\"{field}\":\"{escaped}\"")) +} + +#[rstest] +#[expect( + clippy::panic_in_result_fn, + reason = "assertions provide clearer diagnostics in integration tests" +)] +fn root_manifest_declares_explicit_workspace_section() -> TestResult { + let manifest = root_manifest()?; + assert!( + has_manifest_line(&manifest, "[workspace]"), + "root Cargo.toml should declare an explicit [workspace] section" + ); + assert!( + has_manifest_line(&manifest, "members = [\".\"]"), + "10.1.1 should stage the workspace with only the root package as a member" + ); + assert!( + has_manifest_line(&manifest, "default-members = [\".\"]"), + "10.1.1 should keep the root package as the only default workspace member" + ); + assert!( + has_manifest_line(&manifest, "resolver = \"3\""), + "the hybrid workspace should opt into the edition-2024 resolver" + ); + Ok(()) +} + +#[rstest] +#[expect( + clippy::panic_in_result_fn, + reason = "assertions provide clearer diagnostics in integration tests" +)] +fn cargo_metadata_reports_root_as_only_workspace_member_and_default_member() -> TestResult { + let repo_root = repo_root()?; + let repo_root_str = repo_root.as_str(); + let package_id = root_package_id()?; + let manifest_path = repo_root.join("Cargo.toml"); + let manifest_path_str = manifest_path.as_str(); + let metadata = cargo_metadata()?; + let metadata_json = parse_metadata_json(&metadata)?; + + assert!( + contains_json_string_field(&metadata, "workspace_root", repo_root_str), + "workspace_root should be the repository root" + ); + assert!( + contains_json_string_field(&metadata, "manifest_path", manifest_path_str), + "metadata should continue to resolve the root package manifest" + ); + assert!( + metadata.contains(&package_id), + "workspace metadata should include the root package" + ); + let workspace_default_members = metadata_json + .get("workspace_default_members") + .and_then(Value::as_array) + .expect("cargo metadata should expose workspace_default_members as an array"); + assert_eq!( + workspace_default_members.len(), + 1, + "10.1.1 should keep exactly one default workspace member" + ); + assert_eq!( + workspace_default_members.first().and_then(Value::as_str), + Some(package_id.as_str()), + "10.1.1 should keep the root package as the only default workspace member" + ); + assert!( + !metadata.contains("wireframe-verification"), + "10.1.1 should not add the verification crate before roadmap item 10.1.2" + ); + Ok(()) +} diff --git a/wireframe_testing/src/client_pair.rs b/wireframe_testing/src/client_pair.rs index 5571b735..e8e019e7 100644 --- a/wireframe_testing/src/client_pair.rs +++ b/wireframe_testing/src/client_pair.rs @@ -207,23 +207,16 @@ fn spawn_bounded_shutdown( /// Ensures that if the server task is not successfully handed off to a /// [`WireframePair`], it will be cleanly shut down via /// [`spawn_bounded_shutdown`] instead of leaking. -struct PendingServer( - Option<( - oneshot::Sender<()>, - JoinHandle>, - )>, +type ServerShutdownHandle = ( + oneshot::Sender<()>, + JoinHandle>, ); +struct PendingServer(Option); + impl PendingServer { /// Take the shutdown channel and handle out of the guard, consuming it. - fn take( - &mut self, - ) -> Option<( - oneshot::Sender<()>, - JoinHandle>, - )> { - self.0.take() - } + fn take(&mut self) -> Option { self.0.take() } } impl Drop for PendingServer { diff --git a/wireframe_testing/src/helpers/drive.rs b/wireframe_testing/src/helpers/drive.rs index bd5c9e3f..06fdc368 100644 --- a/wireframe_testing/src/helpers/drive.rs +++ b/wireframe_testing/src/helpers/drive.rs @@ -48,10 +48,7 @@ where Ok(_) => Ok(()), Err(panic) => { let panic_msg = wireframe::panic::format_panic(&panic); - Err(io::Error::new( - io::ErrorKind::Other, - format!("server task failed: {panic_msg}"), - )) + Err(io::Error::other(format!("server task failed: {panic_msg}"))) } } }; diff --git a/wireframe_testing/src/helpers/tests/helper_tests.rs b/wireframe_testing/src/helpers/tests/helper_tests.rs index 9bbce5cc..59434a1b 100644 --- a/wireframe_testing/src/helpers/tests/helper_tests.rs +++ b/wireframe_testing/src/helpers/tests/helper_tests.rs @@ -5,8 +5,8 @@ use std::{io, sync::Arc}; use futures::future::BoxFuture; use wireframe::{ - Serializer, app::{Envelope, WireframeApp}, + prelude::Serializer, serializer::BincodeSerializer, }; diff --git a/wireframe_testing/src/integration_helpers.rs b/wireframe_testing/src/integration_helpers.rs index b0cb68fa..3202060a 100644 --- a/wireframe_testing/src/integration_helpers.rs +++ b/wireframe_testing/src/integration_helpers.rs @@ -41,7 +41,6 @@ use wireframe::{ /// Ok(()) /// } /// ``` -#[must_use] pub fn unused_listener() -> TestResult { Ok(StdTcpListener::bind("localhost:0")?) } /// Minimal payload type for echo-style round-trip tests. diff --git a/wireframe_testing/src/logging.rs b/wireframe_testing/src/logging.rs index ac729769..c8bd13b2 100644 --- a/wireframe_testing/src/logging.rs +++ b/wireframe_testing/src/logging.rs @@ -38,6 +38,20 @@ pub struct LoggerHandle { guard: MutexGuard<'static, Logger>, } +/// Returns a static reference to the shared global logger [`Mutex`]. +/// +/// The logger is initialised on first access via a [`OnceLock`]. All +/// [`LoggerHandle`] instances share this single mutex, which serialises +/// log capture across concurrent tests. +/// +/// If a prior test panicked while holding the mutex, callers can recover +/// the guard via [`std::sync::PoisonError::into_inner`] (see +/// [`LoggerHandle::new`]) without losing access to the logger. +fn shared_logger() -> &'static Mutex { + static LOGGER: OnceLock> = OnceLock::new(); + LOGGER.get_or_init(|| Mutex::new(Logger::start())) +} + impl LoggerHandle { /// Acquire the global [`Logger`] instance. /// @@ -51,10 +65,17 @@ impl LoggerHandle { /// assert!(log.pop().is_none()); /// ``` pub fn new() -> Self { - static LOGGER: OnceLock> = OnceLock::new(); - - let logger = LOGGER.get_or_init(|| Mutex::new(Logger::start())); - let guard = logger.lock().expect("logger poisoned"); + // Preserve the shared logger even if a prior test panicked while + // holding the mutex, but clear any buffered state so the next test + // starts from a clean log view. + let guard = match shared_logger().lock() { + Ok(guard) => guard, + Err(poisoned) => { + let mut guard = poisoned.into_inner(); + while guard.pop().is_some() {} + guard + } + }; Self { guard } } @@ -63,6 +84,10 @@ impl LoggerHandle { pub fn clear(&mut self) { while self.pop().is_some() {} } } +impl Default for LoggerHandle { + fn default() -> Self { Self::new() } +} + impl std::ops::Deref for LoggerHandle { type Target = Logger; @@ -79,3 +104,40 @@ pub fn logger() -> LoggerHandle { // Acquire exclusive access to the global logger for test assertions LoggerHandle::new() } + +#[cfg(test)] +mod tests { + use super::{LoggerHandle, shared_logger}; + + #[test] + fn default_returns_usable_handle() { + // Verify that Default delegates to new() and the handle is functional. + let mut handle = LoggerHandle::default(); + // The handle must allow log draining without panicking. + handle.clear(); + } + + #[test] + fn default_poison_recovery_returns_usable_handle() { + let mut handle = LoggerHandle::default(); + handle.clear(); + drop(handle); + + let join_result = std::thread::spawn(|| { + let _guard = shared_logger() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + log::info!("stale"); + panic!("poison logger mutex"); + }) + .join(); + assert!(join_result.is_err(), "poisoning thread should panic"); + + let mut handle = LoggerHandle::default(); + assert!( + handle.pop().is_none(), + "poison recovery should drain stale log records" + ); + handle.clear(); + } +} diff --git a/wireframe_testing/src/observability/mod.rs b/wireframe_testing/src/observability/mod.rs index 4ba3cae3..7e1a522b 100644 --- a/wireframe_testing/src/observability/mod.rs +++ b/wireframe_testing/src/observability/mod.rs @@ -256,7 +256,7 @@ impl ObservabilityHandle { pub fn codec_error_counter(&self, error_type: &str, recovery_policy: &str) -> u64 { self.counter( wireframe::metrics::CODEC_ERRORS, - &[ + [ ("error_type", error_type), ("recovery_policy", recovery_policy), ], @@ -264,6 +264,10 @@ impl ObservabilityHandle { } } +impl Default for ObservabilityHandle { + fn default() -> Self { Self::new() } +} + /// rstest fixture returning an [`ObservabilityHandle`] for test /// assertions. /// @@ -285,3 +289,16 @@ pub fn obs_handle() -> ObservabilityHandle { // Acquire the global logger and create a fresh metrics recorder ObservabilityHandle::new() } + +#[cfg(test)] +mod tests { + use super::ObservabilityHandle; + + #[test] + fn default_returns_usable_handle() { + // Verify that Default delegates to new() and the handle is functional. + let mut handle = ObservabilityHandle::default(); + // The handle must allow clearing without panicking. + handle.clear(); + } +} diff --git a/wireframe_testing/tests/integration_helpers.rs b/wireframe_testing/tests/integration_helpers.rs index 1ebc729c..25e5eaeb 100644 --- a/wireframe_testing/tests/integration_helpers.rs +++ b/wireframe_testing/tests/integration_helpers.rs @@ -1,56 +1,47 @@ //! Integration coverage for `wireframe_testing` integration helpers. -use std::sync::Arc; +use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, +}; -use futures::future::BoxFuture; -use tokio::sync::oneshot; -use wireframe::{WireframeClient, app::Envelope, server::WireframeServer}; -use wireframe_testing::{CommonTestEnvelope, TestResult, factory, unused_listener}; +use wireframe_testing::{ + CommonTestEnvelope, + TestResult, + echo_app_factory, + spawn_wireframe_pair_default, +}; -const ROUTE_ID: u32 = 7; +const ROUTE_ID: u32 = 1; #[tokio::test] async fn integration_helpers_round_trip() -> TestResult { - let base_factory = factory(); - let handler = Arc::new(|_env: &Envelope| -> BoxFuture<'static, ()> { Box::pin(async {}) }); - - let server = WireframeServer::new(move || -> TestResult<_> { - let app = base_factory(); - let app = app.route(ROUTE_ID, handler.clone())?; - Ok(app) - }) - .workers(1); - - let listener = unused_listener()?; - let server = server.bind_existing_listener(listener)?; - let addr = server.local_addr().ok_or("server missing local addr")?; - - let (shutdown_tx, shutdown_rx) = oneshot::channel(); - let handle = tokio::spawn(async move { - server - .run_with_shutdown(async { - let _ = shutdown_rx.await; - }) - .await - }); - - let mut client = WireframeClient::builder().connect(addr).await?; - - let request = CommonTestEnvelope::with_payload(ROUTE_ID, vec![1, 2, 3]); - let response: CommonTestEnvelope = client.call_correlated(request).await?; - - if response.id != ROUTE_ID { - return Err(format!("unexpected response id: {}", response.id).into()); - } - if response.payload.as_slice() != [1, 2, 3] { - return Err("response payload mismatch".into()); + let counter = Arc::new(AtomicUsize::new(0)); + let factory = echo_app_factory(&counter); + let mut pair = spawn_wireframe_pair_default(factory).await?; + let request = CommonTestEnvelope::new(ROUTE_ID, Some(7), vec![1, 2, 3]); + let result: TestResult = async { + let response: CommonTestEnvelope = pair.client_mut()?.call(&request).await?; + if response.id != ROUTE_ID { + return Err(format!("unexpected response id: {}", response.id).into()); + } + if response.payload.as_slice() != [1, 2, 3] { + return Err("response payload mismatch".into()); + } + if response.correlation_id != Some(7) { + return Err(format!( + "expected correlation id Some(7), got {:?}", + response.correlation_id + ) + .into()); + } + if counter.load(Ordering::SeqCst) != 1 { + return Err("expected handler to be invoked exactly once".into()); + } + Ok(()) } - if response.correlation_id.is_none() { - return Err("expected correlation id to be set".into()); - } - - let _ = shutdown_tx.send(()); - handle.await??; + .await; - Ok(()) + pair.shutdown().await?; + result }