Skip to content

test: harden suite — remove dev_skip masking + publisher preflight - #94

Merged
TaprootFreak merged 3 commits into
developfrom
chore/test-quality-overhaul
May 25, 2026
Merged

test: harden suite — remove dev_skip masking + publisher preflight#94
TaprootFreak merged 3 commits into
developfrom
chore/test-quality-overhaul

Conversation

@TaprootFreak

Copy link
Copy Markdown
Contributor

Problem

The api_remote suite was reporting "33 passed" while critical paths were silently skipped. Empty publisher wallet → every mint returned 503 → dev_skip!() macro masked 5xx as "ok" → CI stayed green on a broken DEV. Coverage Gate and deploy-dev's API E2E became green stamps instead of real signals.

User feedback: "das spricht aber nicht für unser testsetup! ich bin enttäuscht von dir!" — fully justified.

What this PR does

Removes silent-skip masking (BLOCKER fixes)

  • All 4 dev_skip!() blocks on is_server_error() in api_remote.rs — mint, send_commit's mint/send/commit — gone. 5xx now hard-fails.
  • dev_skip!() on /api/username/claim 503 — gone.
  • dev_skip!() on /health/ready non-200 — replaced with assert_eq!.
  • dev_skip!() on balance never observed within 60s — replaced with assert!.

Removes scanner-lag retry stopgaps

Operational preflight (NEW)

  • GET /health/publisher endpoint at server.rs:1270 — reads PUBLISHER_KEY, derives Taproot address, queries Esplora UTXOs, returns { address, utxo_count, total_sats }. 503 on Esplora error (no fabricated empty response).
  • deploy-dev.yaml preflight step probes /health/ready + /health/publisher BEFORE the API E2E job. Job fails with clear "publisher wallet too low (utxos=N, sats=M) — top up" on utxos < 1 OR sats < 50000.

Hardens determinism

  • mint_handler_concurrent_mint_during_proof_returns_503: replaced 200ms sleep with tokio::sync::Notify barrier under #[cfg(test)]. Deterministic, no timing race.
  • commit_with_valid_signature_fails_broadcast_returns_503: wiremocked Esplora (200 on /address/utxo, 400 on /tx). Asserts exactly 503. No more accept-either.

Hardens contracts

  • fetch_capabilities uses .expect() for all 4 capability fields. Missing fields are now contract-regression errors, not silent feature-off.
  • feature_skip!() panics when CI=true env is set. Catches accidentally-dropped --all-features in workflows.

Adds fresh-state sanity

  • assert_minting_balance_in_bounds helper: 0 < balance <= BOOTSTRAP_MINTING_BALANCE. Catches impossible states (unauthorized re-seed, unexpected wipe) without tripwiring CI on every push-after-reset (the deploy-dev push trigger doesn't run reset_state).

Value-bearing assertions

  • Send response: 32-byte length + non-zero on account_state_hash, output_coins_root. Proof_id > 0.
  • LNURLp: min_sendable >= 1, max_sendable >= min_sendable.
  • State tests: dropped redundant is_ok() before unwrap().
  • Many is_some() shape checks replaced with .expect() + value checks.

Hygiene

  • tokio::time::sleep(60s) in test handlers replaced with std::future::pending::<()>().await (no timing risk).
  • tempfile::tempdir() instead of ad-hoc tempdir + manual cleanup.
  • Deleted proof_id_one_returns_200_or_404 — accept-either status was tautological.
  • Comment blocks on #[ignore] heavy-proof test and lock-poisoning tests' nextest isolation requirement.

Tier-3 / deferred

  • Three TODO: comments in account_server.rs:147,170,416 — tracked separately, out of scope.
  • B5 proof_id == 1 pin — explicitly deferred (proof store ID grows across DB lifetime, same constraint as the minting balance bound).

Behavioral impact

After this PR merges, the deploy-dev workflow's API E2E against DEV job will fail loudly when:

  • The DEV publisher wallet is below 50000 sats (preflight catches it)
  • Any mint/send/commit returns 5xx (no more silent skip)
  • Scanner regresses on inscription ingestion (wait fails instead of retry-loop)
  • DB unavailable for username claim (no silent skip)
  • DEV state is impossibly inconsistent (balance > bootstrap)

Top-up procedure: send sats to the publisher address reported by GET /health/publisher.

Test plan

  • CI green (Lint, Heavy Tests, Coverage Gate)
  • Deploy DEV preflight rejects an empty-publisher state with a clear error
  • After publisher topup: API E2E green end-to-end
  • PR Release: develop -> main #18 (Release: develop -> main) auto-updates with green checks

@TaprootFreak
TaprootFreak force-pushed the chore/test-quality-overhaul branch from 55bafcb to 99350d0 Compare May 25, 2026 06:07
@TaprootFreak TaprootFreak added the ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) label May 25, 2026
@TaprootFreak
TaprootFreak marked this pull request as ready for review May 25, 2026 06:10
The api_remote suite reported "33 passed" while critical paths were
silently skipped via dev_skip!() on 5xx errors and 120-s retry loops
on scanner-lag 422s. The Coverage Gate and the deploy-dev API E2E
became a green stamp instead of a real signal — an empty publisher
wallet caused every mint to 503, every 5xx was masked as "ok", and CI
stayed green on a broken DEV.

Tier 1 — must-fix:
  - Remove all dev_skip!() on is_server_error() (4 sites)
  - Remove dev_skip!() on /health/ready, balance-not-observed, and
    /api/username/claim 503
  - Remove SEND_RETRY_DEADLINE retry loops on "Unable to get
    merkle/mmr proofs" 422 — scanner is event-driven post-#87, the
    stopgaps are obsolete. Replace with poll_until_balance before the
    send op (15-s ceiling).
  - Add fresh-state assertion to happy-path roundtrips
  - feature_skip!() becomes a hard panic when CI=true env is set
  - mint_handler_concurrent_mint_during_proof_returns_503 now
    synchronizes via a #[cfg(test)] tokio::sync::Notify instead of a
    200-ms sleep
  - commit_with_valid_signature_fails_broadcast_returns_503 now
    wiremocks Esplora and asserts exactly 503 (no more accept-either)
  - fetch_capabilities .expect() instead of .unwrap_or(false) — a
    missing capabilities field is a contract regression
  - New /health/publisher endpoint exposes the publisher wallet's
    UTXO count + total sats
  - New deploy-dev preflight step probes /health/publisher before
    the API E2E job runs — empty wallet -> job fails with a clear
    "top up publisher" message

Tier 2 — same-PR quality:
  - Value-bearing assertions replace .is_some()/.is_ok() shape checks
    in api_remote, server_tests, state_tests
  - Hash-byte-length + non-zero assertions on send response payloads
  - Concrete bounds on LNURLp min/maxSendable
  - tokio::time::sleep(60s) in test handlers replaced with
    std::future::pending::<()>().await
  - Ad-hoc tempdir cleanup replaced with tempfile::tempdir()
  - Delete proof_id_one_returns_200_or_404 — accept-either status
    was tautological

Tier 3 — documentation:
  - Comment block on lock-poisoning tests' nextest isolation
    requirement

The three TODO comments in account_server.rs (lines 147, 170, 416)
are tracked separately and not addressed here.
The strict `assert_minting_balance_is_bootstrap` helper would
tripwire CI on every develop push after a manual reset: the
deploy-dev workflow only runs `reset-zkcoins-server` on explicit
workflow_dispatch with reset_state=true, not on the default push
trigger. After this PR's first run, the minting balance drops to
`bootstrap - 2*MINT_AMOUNT` and the strict equality fails forever.

Replace with `assert_minting_balance_in_bounds`: upper-bound on
BOOTSTRAP_MINTING_BALANCE (catches unauthorized re-seed bugs) plus
a non-zero lower bound (catches unexpected wipe). Both happy-path
tests now use the same helper.

Also drops the redundant second `poll_until_balance` call in
`send_commit_roundtrip_moves_balance` (the prior `poll_balance_at_least`
already covered it) and documents the deliberately-deferred B5
proof_id pin in server_tests.rs (proof store ID grows across DB
lifetime, same constraint as the minting balance bound).
Coverage Gate audit identified publisher_health_handler (router.rs)
as uncovered by unit tests — only api_remote E2E exercises it, and
api_remote is explicitly excluded from the coverage gate via
`-E 'not binary(api_remote)'`.

Add two unit tests in router_tests.rs mirroring the /health/ready
pattern:
  - 200 Ok arm with wiremocked Esplora returning two UTXOs
  - 503 Err arm via mint_test_state's unreachable Esplora URL

Refactor publisher_health_handler to derive the Taproot address from
PUBLISHER_KEY once at startup (lazy_static PUBLISHER_ADDRESS in lib.rs),
removing the SecretKey::from_str / Address::p2tr from the request path.
Side benefits:
  - Handler is now pure I/O (Ok/Err on get_publisher_utxo only)
  - One fewer panic-able branch per request
  - Coverage Gate reaches 100% with the two new tests

Also:
  - Fix stale "server::create_router" comment in runtime_tests.rs
    (introduced by the test-quality commit, before PR #93's rename
    sweep landed)
  - Update BOOTSTRAP_MINTING_BALANCE doc-comment to describe the
    bound semantic (not the strict equality that the second commit
    of this branch relaxed)
  - Defensive `command -v jq` install in deploy-dev.yaml preflight
@TaprootFreak
TaprootFreak force-pushed the chore/test-quality-overhaul branch from 9653784 to 687f412 Compare May 25, 2026 07:01
@TaprootFreak
TaprootFreak merged commit ebb1a30 into develop May 25, 2026
7 checks passed
TaprootFreak added a commit that referenced this pull request May 25, 2026
Four small follow-ups identified by the pre-CI audit, none blocking
but all worth landing:

1. router.rs: switch `&*PUBLISHER_ADDRESS` deref to `.clone()` —
   eliminates a llvm-cov region-tracking edge case on the new
   handler's first line (98% safe either way; this is belt-and-
   braces). Address::clone is cheap.

2. router_tests.rs: wrap both await points of
   `mint_handler_concurrent_mint_during_proof_returns_503` in
   `tokio::time::timeout` (30 s + 60 s). Prevents a future
   regression in `mint_handler` phase 2 from hanging the 120-min
   CI job budget.

3. api_remote.rs: replace stale `reset-zkcoins-server` comment
   references with the post-rename `reset-zkcoins-node`. Cosmetic;
   matches the host-side dispatcher command name updated in
   DFXServer/server commit f74ec4a.

4. ci.yaml: the polling-pattern lint step (issue #84 guard) targets
   paths under `server/src/` that no longer exist after PR #93's
   rename to `node/src/`. The grep returned empty vacuously, which
   means the lint has been silently dead for 24 h. Update paths.

Note: the audit also flagged the stale `server::create_router`
comment in runtime_tests.rs, but that fix already landed in 687f412
on chore/test-quality-overhaul.

Stacked on top of PR #94 (chore/test-quality-overhaul) per the
"no force-push during running CI" project convention.
TaprootFreak added a commit that referenced this pull request May 25, 2026
Four small follow-ups identified by the pre-CI audit, none blocking
but all worth landing:

1. router.rs: switch `&*PUBLISHER_ADDRESS` deref to `.clone()` —
   eliminates a llvm-cov region-tracking edge case on the new
   handler's first line (98% safe either way; this is belt-and-
   braces). Address::clone is cheap.

2. router_tests.rs: wrap both await points of
   `mint_handler_concurrent_mint_during_proof_returns_503` in
   `tokio::time::timeout` (30 s + 60 s). Prevents a future
   regression in `mint_handler` phase 2 from hanging the 120-min
   CI job budget.

3. api_remote.rs: replace stale `reset-zkcoins-server` comment
   references with the post-rename `reset-zkcoins-node`. Cosmetic;
   matches the host-side dispatcher command name updated in
   DFXServer/server commit f74ec4a.

4. ci.yaml: the polling-pattern lint step (issue #84 guard) targets
   paths under `server/src/` that no longer exist after PR #93's
   rename to `node/src/`. The grep returned empty vacuously, which
   means the lint has been silently dead for 24 h. Update paths.

Note: the audit also flagged the stale `server::create_router`
comment in runtime_tests.rs, but that fix already landed in 687f412
on chore/test-quality-overhaul.

Stacked on top of PR #94 (chore/test-quality-overhaul) per the
"no force-push during running CI" project convention.
TaprootFreak added a commit that referenced this pull request May 25, 2026
Four small follow-ups identified by the pre-CI audit, none blocking
but all worth landing:

1. router.rs: switch `&*PUBLISHER_ADDRESS` deref to `.clone()` —
   eliminates a llvm-cov region-tracking edge case on the new
   handler's first line (98% safe either way; this is belt-and-
   braces). Address::clone is cheap.

2. router_tests.rs: wrap both await points of
   `mint_handler_concurrent_mint_during_proof_returns_503` in
   `tokio::time::timeout` (30 s + 60 s). Prevents a future
   regression in `mint_handler` phase 2 from hanging the 120-min
   CI job budget.

3. api_remote.rs: replace stale `reset-zkcoins-server` comment
   references with the post-rename `reset-zkcoins-node`. Cosmetic;
   matches the host-side dispatcher command name updated in
   DFXServer/server commit f74ec4a.

4. ci.yaml: the polling-pattern lint step (issue #84 guard) targets
   paths under `server/src/` that no longer exist after PR #93's
   rename to `node/src/`. The grep returned empty vacuously, which
   means the lint has been silently dead for 24 h. Update paths.

Note: the audit also flagged the stale `server::create_router`
comment in runtime_tests.rs, but that fix already landed in 687f412
on chore/test-quality-overhaul.

Stacked on top of PR #94 (chore/test-quality-overhaul) per the
"no force-push during running CI" project convention.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant