fix(test): assert full error chain in pr-comment missing-id test (unbreaks cargo test --all) #149

Merged
stephen merged 1 commit from fix/pr-comment-test-green into main 2026-06-11 02:55:27 +00:00
Owner

Fixes the main-red regression: client::integration_tests::pr_comment_edit_and_delete_missing_id_error failed under the full cargo test --all (it passed in #131's isolated CI, so it slipped in).

Root cause: the test asserted on err.to_string(), which renders only the top-level error, not the source chain. One of the two code paths surfaces the API 404 only in anyhow's alternate ({:#}) form, so the assertion failed depending on which path/error rendering was exercised.

Fix: assert on format!("{err:#}") (full source chain), validating the test's documented intent for both paths. Test-only change; no production behavior touched.

Follow-up (out of scope, noted for a separate PR): delete_comment bypasses ensure_success, so a missing comment returns a raw reqwest error instead of ApiError (no friendly "not found" message, and exits EXIT_GENERIC instead of EXIT_NOT_FOUND).

Red-CI fix: mandatory cross-family review.

Fixes the main-red regression: `client::integration_tests::pr_comment_edit_and_delete_missing_id_error` failed under the full `cargo test --all` (it passed in #131's isolated CI, so it slipped in). Root cause: the test asserted on `err.to_string()`, which renders only the top-level error, not the source chain. One of the two code paths surfaces the API 404 only in anyhow's alternate (`{:#}`) form, so the assertion failed depending on which path/error rendering was exercised. Fix: assert on `format!("{err:#}")` (full source chain), validating the test's documented intent for both paths. Test-only change; no production behavior touched. Follow-up (out of scope, noted for a separate PR): `delete_comment` bypasses `ensure_success`, so a missing comment returns a raw reqwest error instead of `ApiError` (no friendly "not found" message, and exits EXIT_GENERIC instead of EXIT_NOT_FOUND). Red-CI fix: mandatory cross-family review.
Fix flaky-looking pr comment 404 test: assert on full error chain
All checks were successful
ci / check (pull_request) Successful in 9m53s
ci / live-e2e (pull_request) Successful in 1m45s
ci / coverage (pull_request) Successful in 1m59s
30ceb0c97e
pr_comment_edit_and_delete_missing_id_error asserted
`edit_err.to_string().contains("404")`, but `edit_comment` goes through
`client.json` -> `ensure_success`, which wraps 404s in a friendly
`not found: {resource}` context. anyhow's `Display` (`to_string`) shows
only the outermost context, so the status code lives in the source chain,
not the top-level message. The assertion failed deterministically.

The delete assertion only passed by accident: `delete_comment` uses
reqwest's `error_for_status`, whose `Display` happens to include
"404 Not Found".

The edit/delete feature is correct (it surfaces the 404 instead of
silently succeeding). Assert against the full error chain (`{:#}`) for
both paths so the test validates the documented intent regardless of
which formatting layer the status lands in.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
rasterstate/fj!149
No description provided.