Repository navigation
Make integration test assertions fallible - #708
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (24)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Summary
The author reports removing all 45 measured The PR targets WalkthroughThe test suite replaces panic-based assertions with fallible checks that return descriptive errors. Lifecycle server tasks propagate failures, fixture payload generation uses checked or cycling conversions, and an unused test harness method is removed. ChangesTest error propagation
Suggested labels: Priority: ⬇️ Low Change: Other Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established; the change is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
Full details: Developer DocumentationExplanation The pull request adds a shared test abstraction without updating Resolution Add a section to Check each frame and pass errors along. Comment |
Reviewer's GuideThe PR replaces panicking assertions and related lint suppressions across 18 integration-test files with fallible checks that preserve detailed expected/actual diagnostics, propagates failures through async and I/O paths, makes fixture conversions safe, and removes one unused harness helper; the stated validation suite passes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
a91831b to
ae3b655
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ae3b655 to
cf65478
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cf65478 to
b83fe9b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
b83fe9b to
5e85f6a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @leynos, your pull request is larger than the review limit of 150,000 diff characters
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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 o11y, code safety, documentation and validation coverage as in scope). ❌ Failed check (1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '--- branch and PR ---'
git status --short
git rev-parse --short HEAD
gh pr view 708 --repo leynos/wireframe --json headRefOid,updatedAt,files,statusCheckRollup --jq '{headRefOid,updatedAt,files:[.files[].path],checks:[.statusCheckRollup[]?|{name,conclusion,status,workflowName}]}'
printf '%s\n' '--- documentation and helper file locations ---'
fd -p 'developers-guide.md|fallible_assertions' docs tests 2>/dev/null || true
printf '%s\n' '--- documentation headings and references ---'
rg -n -C2 'fallible.assert|panic_in_result_fn|integration test|^#{1,4} ' docs/developers-guide.md | head -220
printf '%s\n' '--- helper outlines ---'
for f in tests/common/fallible_assertions/check.rs tests/common/fallible_assertions/check_eq.rs tests/common/fallible_assertions/check_equal.rs; do
echo "$f"
ast-grep outline "$f" --items all 2>/dev/null || true
done
printf '%s\n' '--- helper contents ---'
for f in tests/common/fallible_assertions/check.rs tests/common/fallible_assertions/check_eq.rs tests/common/fallible_assertions/check_equal.rs; do
echo "$f"
sed -n '1,100p' "$f"
done
printf '%s\n' '--- documentation changes in PR ---'
gh pr diff 708 --repo leynos/wireframe --name-only | rg 'docs/|fallible_assertions' || trueLength of output: 15080 🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '--- current guide change in PR ---'
gh pr diff 708 --repo leynos/wireframe -- docs/developers-guide.md 2>/dev/null || git diff "$(git merge-base HEAD origin/main)" HEAD -- docs/developers-guide.md
printf '%s\n' '--- test support section ---'
sed -n '775,930p' docs/developers-guide.md
printf '%s\n' '--- helper module imports ---'
rg -n -C2 'fallible_assertions/(check|check_eq|check_equal)\.rs|check_equal\(|check_eq\(|check\(' tests/workspace_manifest.rs tests/middleware_order.rs tests/client_lifecycle.rs | head -125Length of output: 23505 🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '--- current guide change in PR ---'
git diff "$(git merge-base HEAD origin/main)" HEAD -- docs/developers-guide.md | head -220
printf '%s\n' '--- test support section ---'
sed -n '775,930p' docs/developers-guide.md
printf '%s\n' '--- helper module imports ---'
rg -n -C2 'fallible_assertions/(check|check_eq|check_equal)\.rs|check_equal\(|check_eq\(|check\(' tests/workspace_manifest.rs tests/middleware_order.rs tests/client_lifecycle.rs | head -125Length of output: 23435
My earlier answer was wrong. The search output I relied on stopped before the new section. Withdraw the Developer Documentation finding; no AI agent prompt or follow-up issue is needed for this warning. The documentation finding is resolved by the diff, although that does not establish that every hosted check has rerun. You are interacting with an AI system. |
9f0ca75 to
9989de1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace panic-prone assertions in Result-returning integration tests with fallible checks that preserve their predicates and diagnostics. Keep genuine I/O and subprocess failures on the existing Result paths, and share small fallible assertion helpers across related test crates. Remove the unused connection harness builder and make fixture byte construction fallible without changing its generated payloads.
Move the fallible preamble server exchange and workspace-member checks into small helpers. Preserve the existing predicates, diagnostics, and callback order while satisfying the CodeScene method-size gate on the integration-test layer.
tests/prepared_app.rs (575 lines) and tests/connection.rs (549 lines) both exceeded the repository's 400-line file limit, and the fallible assertion work pushed prepared_app.rs further over it. Split both files into feature-focused modules, following the existing `tests/preamble/` and `tests/fragment_transport/` layout: - prepared_app/: support, startup, teardown, reuse, tcp, property - connection/: support, multi_packet, logs, state The root files are now thin drivers that declare the submodules. Shared fixtures, harness configuration, and assertions move into a `support` module in each crate. No assertion predicate, message, callback ordering, or test name changed; the crates still build to the same 5 and 17 tests respectively. The `#[path]` declaration for the fallible assertion helpers stays in the driver file because those helpers are `pub(super)`, which resolves to crate visibility only at the crate root. Co-Authored-By: Claude Code <noreply@anthropic.com>
The fallible assertion abstraction added in this branch had no entry in the developers' guide, which documents the other shared test-support boundaries. Add a "Fallible assertions in integration tests" subsection covering why `Result`-returning tests need it, how to choose between `check`, `check_eq`, and `check_equal`, and the `Result<(), String>` failure contract. Record the `#[path]` placement rule, since the helpers are `pub(super)` and therefore must be declared in the crate root rather than in a submodule. Remains test-only scaffolding, so no ADR or design document change is needed. Co-Authored-By: Claude Code <noreply@anthropic.com>
The rebase conflict resolution merged `main`'s wireframe-loom assertions into this branch's extracted `check_workspace_members` helper by adding `loom_package_id` as a fifth parameter. This repo pins `too-many-arguments-threshold = 4` in `clippy.toml` (the default is 7), so the helper breached `clippy::too_many_arguments` under `-D warnings`. The helper now takes the expected members as a slice of `(package_id, description)` pairs, and each call site names its own pairs. That keeps `main`'s description-carrying loop and the verification of all four workspace members, including the wireframe-loom crate, while restoring the two-parameter signature. The branch exists to remove `#[expect]`s of this shape, so suppressing the lint here would have worked against its purpose. Gate result for this head: `make check-fmt`, `make typecheck`, `make test`, `make markdownlint`, and `make spelling` all exited 0; `make lint` failed only on this one diagnostic, which is now fixed. Co-Authored-By: Claude Code <noreply@anthropic.com>
Merging `main`'s wireframe-loom assertion into `cargo_metadata_reports_explicit_members_without_widening_default_members` grew that function to 71 lines against CodeScene's threshold of 70, so CodeScene raised a "Large Method" finding on the rebased head. It was 65 lines on `main`, so the growth came from the conflict resolution rather than from either side's intent. Extract the two identical `cargo metadata` array lookups into a `metadata_array` helper. The function is now 55 lines, below both the CodeScene threshold and its pre-rebase length, and the duplicated `ok_or_else` formatting lives in one place. The diagnostic text is unchanged: it still names the requested field and reports the actual value. `cargo check --workspace --all-targets --all-features` and `cargo fmt --all -- --check` both exit 0 for this change. Co-Authored-By: Claude Code <noreply@anthropic.com>
613ed21 to
f693b29
Compare
Rebase update — citations refreshed for the new head
SHAs cited in the earlier confirmations no longer exist on this branch. Commit-message-preserving equivalents in the new series:
All three previously-confirmed concerns re-verified at
Because
Gates on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '--- head and PR status ---'
git rev-parse --short HEAD
gh pr view 708 --repo leynos/wireframe --json headRefOid,statusCheckRollup --jq '{headRefOid,checks:[.statusCheckRollup[]?|{name,status,conclusion}]}'
printf '%s\n' '--- named files ---'
fd -p 'tests/client_lifecycle.rs|tests/workspace_manifest.rs|tests/prepared_app.rs|tests/prepared_app/tcp.rs|tests/prepared_app_tcp.rs|docs/developers-guide.md' . || true
printf '%s\n' '--- outlines ---'
for f in tests/client_lifecycle.rs tests/workspace_manifest.rs tests/prepared_app.rs tests/prepared_app/tcp.rs tests/prepared_app_tcp.rs; do
if test -f "$f"; then echo "--- $f"; ast-grep outline "$f" --items all 2>/dev/null | head -85; fi
done
printf '%s\n' '--- lifecycle server and checks ---'
sed -n '130,220p' tests/client_lifecycle.rs
printf '%s\n' '--- workspace checks ---'
sed -n '1,205p' tests/workspace_manifest.rs
printf '%s\n' '--- prepared app references ---'
rg -n -C3 'factory_calls|#\[path|check_equal|panic_in_result_fn|PreparedApp' tests/prepared_app.rs tests/prepared_app/tcp.rs tests/prepared_app_tcp.rs 2>/dev/null | head -160
wc -l tests/prepared_app.rs tests/prepared_app/tcp.rs tests/workspace_manifest.rs
printf '%s\n' '--- guide section ---'
rg -n -C7 'Fallible assertions in integration tests|fallible_assertions|check_equal' docs/developers-guide.md | head -110
printf '%s\n' '--- doc diff from replay boundary ---'
git diff --stat 43b90a3 HEAD -- docs/developers-guide.mdLength of output: 27557
The inspected files support your report about the three earlier concerns. I did not rerun the local gates. Your reported local gate results remain author-reported. ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
Summary
This branch replaces panicking assertions in
Result-returning integration tests with fallible checks, so test failures retain useful diagnostics without suppressingclippy::panic_in_result_fn. It removes all 45 measured expectations for that lint across the assigned integration surface, retainsResultfor real I/O and subprocess work, makes three numeric fixture conversions fallible, and deletes one unused connection-harness helper.This is the second test-quality layer, stacked directly above #706 (
test-quality-sweep). Spelling, structural module moves, production lint fixes, and final lint enforcement remain in later layers. No generated spelling configuration or byte-exact golden data changed.Review feedback
Two findings from the previous round are addressed on the current head.
clippy::panic_in_result_fnmotivation, how to choose betweencheck,check_eq, andcheck_equal, theResult<(), String>failure contract, and the#[path]/pub(super)placement rule. The abstraction is test-only scaffolding, so no ADR or design-document change was needed.tests/prepared_app.rsover the 400-line limit. The branch's originaltests/prepared_app.rs(575 lines) andtests/connection.rs(549 lines) both exceeded theAGENTS.mdlimit, so both are split into feature-focused modules behind thin driver files. The rebase re-expressed that split againstmain's newer layout; see Rebase below.tests/workspace_manifest.rs. The extractedcheck_workspace_membershelper and its caller were shortened so both fall below the method-size threshold. CodeScene re-reviewed at613ed21and approved.Review walkthrough
with_responsebuilder is now gone; prepared-application driver with real-socket cases.Rebase
origin/mainadvanced to2892081(PR #642, "Prepare applications before server readiness") after the previous review round, which put this branch in conflict. The branch is rebased onto2892081; the replay boundary (OLD_BASE) is43b90a3, the last inherited commit, which was both the merge base and the prior rebase boundary because the target fast-forwarded from it.Six commits were replayed. Three conflicts arose across two files:
tests/prepared_app.rs— main's structure and main's semantics won, plus this branch's fallible conversion.mainhad independently solved the same 400-line problem with sibling crates (prepared_app.rs+prepared_app_properties.rs+prepared_app_tcp.rs+common/prepared_app.rsfixtures), which is a strict superset of this branch's subdirectory split, so main's layout was taken and the branch's five superseded split modules dropped. The fallible-assertion purpose was then applied to main's content, which also removed a newly-added#[expect(clippy::panic_in_result_fn)]inprepared_app_tcp.rs. Because main's PR changed production startup semantics — prepare once and share one immutablePreparedApproot across workers — the expected counts changed with it (factory_calls: 1for server startup rather than one per connection), so the old numbers were not re-asserted.tests/prepared_app.rs, size limit. With main's structure adopted, the merged file still exceeded 400 lines, so the real-socket scenarios were extracted totests/prepared_app/tcp.rsbehind a#[path]module declaration, following the existingtests/connection.rs+tests/connection/idiom.tests/prepared_app_tcp.rs, import region. Resolved to main's fixture imports plus this branch's fallible-assertion module.typos.tomlis unchanged by this branch: the pre-rebase "Refresh the generated spelling dictionary" commit is byte-identical to43b90a3andorigin/mainalready carries that refresh, so the rebase correctly dropped it as an empty change.Validation
An independent Scrutineer ran all seven gates sequentially at
f693b29and every gate exited 0:make check-fmt(2s),make lint(31s),make typecheck(11s),make test(134s),make markdownlint(8s),make spelling(4s), andmake nixie(4s).make testran 81 test binaries with 1222 passed, 0 failed, 0 ignored underRUSTFLAGS="-D warnings". Per-gate logs are under/tmp/<action>-test-quality-fallible-assertions-4.out.Because the conflict resolution was manual, the two resolved files were checked to be genuinely exercised and not merely compiling.
tests/prepared_app/tcp.rscontributes theprepared_app_tcpbinary (2 tests), and theprepared_appbinary runs 4 tests includingtcp::prepared_app_serves_real_tcp_connection_without_retransforming, confirming the#[path]module is reachable from the parent rather than orphaned by the split. The siblingtests/prepared_app_tcp.rsalso ran and passed, so the extracted module and the pre-existing top-level file coexist.Behaviour preservation for the
connectionsplit is verified three ways: assertion-message literals are byte-identical, the normalized set of source lines is unchanged apart from moved imports, and the crate still builds to the same test count with module-qualified names confirming the submodules actually run.All five split files are under the 400-line limit:
prepared_app.rs331,prepared_app/tcp.rs104,common/prepared_app.rs91,prepared_app_properties.rs124,prepared_app_tcp.rs143, andconnection.rs17 with its modules at 82/201/70/221.The change surface is 30 paths: 29 under
tests/plusdocs/developers-guide.md.Notes
This is the second test-quality layer, above #706 and below the spelling layer #709. The source inventory said 46
panic_in_result_fnexpectations, but direct inspection of the 18 assigned integration files found 45; all 45 are removed here.origin/mainstill carries 45 across those 18 files — none survive on this head. Tests with real I/O or subprocess setup still returnResult. Lower layers remain under their own lint configuration; full baseline enforcement comes later.The blocking
CIbuild-test lane, which had stalled before the previous round when the pull request's merge ref went stale, dispatched normally for613ed21(36331777486, success, 10m33s). The stall was cleared by the earlier rebase onto a fresh base, confirming it was stale merge-ref metadata rather than anything in the diff.References
🤖 Generated with Claude Code