test(e2e): backfill coverage for the wallet-not-configured reporting decision - #5967
Conversation
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. Important Approval pendingCodeRabbit has no unresolved comments, but it could not review the latest commit because the review limit was reached. Follow the review guidance in this comment to continue. 📝 WalkthroughWalkthroughThe PR adds end-to-end tests for wallet-not-configured classification and reporting. It verifies tracing output for expected wallet state and genuine wallet failures. The test suite is added to the default Rust E2E suite list. ChangesWallet reporting observability
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR adds coverage for wallet-not-configured reporting without changing product behavior; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR satisfies the linked issue objective [ Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52f3c5dba5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Pushed Worth recording that the One wrapped line. Flagging it because "main is red on Rust Quality anyway" was an easy and wrong thing to conclude here. |
…mansai#5805) An unconfigured wallet is the default state of an optional feature, so the condition must never reach Sentry as an error. tinyhumansai#5805 measured 55 error events in 72 minutes on one ordinary local session while a genuine turn-killing failure emitted nothing. The part that regressed is the context-wrapped form: the bare sentinel was already demoted by an exact-equality predicate, but the RPC layer lifts the error with format!("self_identity key_status: {e}"), and exact equality stops matching the moment any caller adds context. That shape appears ~800 times in src/. Adds tests/observability_wallet_expected_e2e.rs, driving the real report_error_or_expected entry point and asserting on what it emits rather than only on the classifier's return value, since "does not page" is the property the issue is about. Registers the suite in scripts/test-rust-e2e.sh so it actually runs. Two of the six tests are deliberate guards rather than coverage of the fix: they pin that a genuine wallet defect is still reported at ERROR, which is the other side of the trade a substring matcher over caller-supplied text makes.
Whitespace only; no assertion or test name changed.
7905443 to
a1808c9
Compare
…not in logs Codex on tinyhumansai#5967: this file's module doc says the observable contract is "this does not page", but the assertions were on tracing output, and paging is not decided there. `report_error_message` reaches Sentry by calling `sentry::with_scope` / `capture_message` directly (core/observability.rs), not through a tracing layer, and no Sentry tracing layer is installed here. So the INFO/ERROR assertions would keep passing if the expected case started paging, or if the genuine-error capture stopped. The gate matters for where the fix belongs: that capture is `#[cfg(feature = "crash-reporting")]`, which is absent from `[features] default` but present in scripts/ci/product-features.txt — and the e2e lane runs this target with that set (scripts/test-rust-e2e.sh passes product-features.sh). The gap is real in the lane that runs it. Add a `#[cfg(feature = "crash-reporting")] mod paging` counting envelopes on a `sentry::test::TestTransport`, wired as tests/observability_smoke.rs does, including its process-global `sentry::init` lock — cargo runs these functions on parallel threads. a_wrapped_wallet_state_pages_nobody -> 0 captured a_genuine_wallet_failure_still_pages -> 1 captured Added, not substituted: the INFO/ERROR assertions pin the breadcrumb's level and its `kind` field, which is what makes a demotion attributable and which TestTransport does not see. `#[cfg]` on the module rather than `required-features` on the target: with the gate off there is nothing to capture and both counts would be a vacuous 0, while `required-features` would take the classification tests down with it in every contributor build — the blunt instrument Cargo.toml already warns about on `observability_smoke`. Proved non-vacuous: swapping the two inputs fails both new tests. cargo test --features crash-reporting --test observability_wallet_expected_e2e -> 8 passed; 0 failed
1 participant
Summary
Backfills e2e coverage for the wallet-not-configured reporting decision (#5805, fixed by #5811), which an audit of the last week's merges found had no e2e coverage anywhere in
tests/—ExpectedErrorKind,expected_error_kindandWALLET_NOT_CONFIGUREDreturned zero matches across both Rust e2e lanes.Adds
tests/observability_wallet_expected_e2e.rsand registers the suite inscripts/test-rust-e2e.shso it actually runs. No product code is changed.Why this one first
Of the five items in my audit slice this was the highest-risk gap, for two independent reasons:
What the tests assert
They drive the real
report_error_or_expectedentry point and assert on what it emits, not only on the classifier's return value — "does not page" is the property the issue is about, and a classification that did not change what is emitted would fix nothing.Coverage of the fix (these fail if the fix is reverted):
a_context_wrapped_wallet_state_is_classified_as_expectedself_identity key_status: {e}shape from #5805 classifies as expected — this is the form that regresseda_multiply_wrapped_wallet_state_is_still_classified_as_expectedthe_bare_wallet_sentinel_is_classified_as_expectedreporting_a_wrapped_wallet_state_emits_info_and_not_errorkind = "wallet_not_configured", and is not ERRORGuards, not coverage (these pass with the fix reverted, by design):
a_genuine_wallet_failure_is_not_demotedreporting_a_genuine_wallet_failure_still_emits_errorI am calling those two out explicitly rather than counting them as coverage, because they do not discriminate the change.
Revert-check
Fix disabled by short-circuiting the
is_wallet_not_configured_messagearm inexpected_error_kind, then restored. All runs via~/tinyhuman/ci-slot.sh.6 passed; 0 failed3 passed; 4 failed— the four failures named my assertions6 passed; 0 failedThe reverted run reproduced the issue verbatim in the captured output:
That
ERROR ... report_errorline is exactly the log #5805 reported 55 times.One test was dropped for being vacuous
the_demoted_wallet_record_withholds_caller_supplied_wrapper_textpassed with the fix reverted, so it was dropped rather than shipped. Its payload was token-shaped (sk-ant-…) and the error path already runssanitize_api_error, which redacts token-shaped text regardless — so the assertion held either way and did not discriminate the change. Pinning the deliberate withholding is still worth doing; it needs a payload the sanitizer will not redact, and a revert-check to prove it discriminates.Not covered by this PR
The other four items in my slice (#5775, #5926, #5945, #5840) are frontend and belong in the Playwright lane. Three specs are written but are not included here because I could not revert-check them under the current machine limits, and an un-revert-checked test is exactly what this work exists to avoid shipping. They are preserved with notes at
~/tinyhuman/bugs/W7-unverified-specs/and documented in~/tinyhuman/bugs/W7-test-findings.md, including a specific warning about the IME one: the obvious version of that test is vacuous, and whoever verifies it must confirm it actually fails with the fix reverted.Submission Checklist
N/A: this PR adds only a test target and one line registering it; there is no product code on the changed lines fordiff-coverto measureN/A: behaviour-only change; no feature row added, removed or renamed## Related—N/A: no matrix feature IDs affectedtracing-subscriber, already a dependencyN/A: no release-cut surface; test-only changeCloses #NNNin the## Relatedsection —N/A: #5805 is already closed by #5811; this backfills its missing coverage and should not re-close anythingRelated
Impact
tests/*_e2e.rstarget and its registration.mainis currently red on theRust Qualitylayout gate — pre-existing, being fixed separately, not from this PR.Summary by CodeRabbit
Bug Fixes
wallet_not_configuredclassification.Tests