Skip to content

test(cargo-budget-report): cover every arg_spec render and error path (#696) - #777

Merged
mallison031 merged 1 commit into
Tollcraft:mainfrom
Dreal-pyper:test/arg-spec-unit-coverage-696
Sep 29, 2026
Merged

mallison031 merged 1 commit into
Tollcraft:mainfrom
Dreal-pyper:test/arg-spec-unit-coverage-696

Conversation

@Dreal-pyper

Copy link
Copy Markdown
Contributor

Description

Adds comprehensive unit-test coverage for cargo-budget-report/src/arg_spec.rs — the module that
turns the typed budget.toml argument specs introduced in #152 into the flat stellar CLI argv.

The file went from 8 tests to 68 tests (+898 lines, one file; arg_spec.rs is now 1143 lines
and no production line was modified). Tests live in the module's existing inline
#[cfg(test)] mod tests, because generated_address() is module-private and a sibling test module
could not reach it — that's also why the new tests sit next to the 8 originals rather than in
cargo-budget-report/tests/ like #700's cli_arg_tests.

Coverage is organised by the shape of the code, with section banners in the file:

Section Covers
Flat entries bare-string specs, the pre-#152 form
Mixing raw + typed in one document, ordering preserved
Scalar types all ten u32 … string keywords, bool, value forwarding
Scalar errors wrong-typed values, missing values, unknown keywords
bytes / bytesn hex parsing, malformed hex, non-string values
address explicit strkey validation, rejection paths
address generate = true seed derivation, determinism, name-based uniqueness
json / struct / vec / map serialisation of each structured value
Unknown types every keyword in the error hint
Error context the full {:#} chain for each failure
Deserialisation surface deny_unknown_fields, untagged-enum errors
Whole-document regression the #152 behaviour: typed ≡ flat

Error assertions pin the complete anyhow chain (format!("{:#}", err) → function `f`, argument `x`: <cause>) rather than substrings, so a reworded context() call fails loudly. Related-issue
test count: none of the production code changed; this PR is test-only plus one CHANGELOG entry.

Related Issue

Closes #696

Motivation and Context

#696 asks for happy-path and error-path unit tests for arg_spec.rs. Measuring first showed the
gap was narrow but real: 10 uncovered production lines out of 69, and they clustered on exactly
the paths most likely to be broken silently — the whole explicit-address branch and every
value-rejection message.

Baseline → after (cargo llvm-cov, arg_spec.rs production lines only):

Metric Before After
Production lines 59/69 — 85.5% 69/69 — 100%
Functions 15/16 — 93.8% 84/84 — 100% (whole module incl. tests)
Tests in module 8 68

Lines 111–115 — the entire explicit address value path — had never executed. Neither had the
four rejection arms at 84, 85, 96, 97 (boolean forwarded for a scalar type, non-scalar value,
explicit bool value, non-boolean bool value) nor the malformed-hex bail at 103. All are
now covered.

The remaining 21 uncovered lines in the file are all inside mod tests: assertion message strings
that only run when a test fails. Those are not reachable in a green suite, by definition.

Two behaviours pinned as findings, not fixed

The issue scopes this work to tests, so both are locked in by assertion with an explanatory comment
instead of being silently patched. Each is worth a follow-up issue:

  1. address is validated by prefix only. A value starting with G is accepted however short,
    empty-tail or otherwise malformed — generated_address/explicit addresses never reach a real
    strkey decode. address_is_checked_by_prefix_only and a_muxed_account_strkey_is_rejected_by_the_prefix_rule
    pin today's behaviour. ("G" alone passes; it is deliberately in the accept list.)
  2. generated_address reads the seed through a repeating 32-byte window. Two argument names
    collide whenever one is a whole repetition of the other, so ab ≡ abab, and any two names that
    share a 32-byte prefix collide outright ("z"×31 ≡ "z"×32). Pinned by
    generated_address_reads_the_name_through_a_repeating_window. I verified the collision class
    independently against a replica of the FNV-1a loop. Realistic contract argument names
    (sender_account vs receiver_account) do not collide, so this is latent rather than live.

Also pinned incidentally: a TOML datetime under type = "json" forwards as
{"$__toml_private_datetime":"…"} — toml-crate internal encoding leaking into CLI arguments
(a_toml_datetime_forwards_as_tomls_private_json_shape).

Scope notes

  • an_unrecognised_key_is_an_error_not_an_ignored_flag asserts serde's untagged enum ArgSpec
    wording: serde swallows the inner deny_unknown_fields reason, so the precise cause is
    unobservable through the untagged enum. The test records that fact instead of over-claiming.
  • Fixtures stay synthetic. strkeys are built via an account(seed) helper wrapping
    stellar_strkey::ed25519::PublicKey rather than pasted real-looking accounts, so no test
    contains anything resembling a live address or key.
  • ROADMAP.md checkbox not done: the issue asks to check off the corresponding box, but this
    repository has no ROADMAP.md (nor the PRD.md / ARCHITECTURE.md / AGENTS.md the issue
    lists as mandatory context). Nothing to tick; flagging rather than inventing a file.

How Has This Been Tested?

  • Added/Updated tests
  • Passed cargo test
  • Passed cargo clippy
  • Formatted with cargo fmt
  • Reviewed or re-measured budget limits in amm-pool-contract/tests/budget_test.rs when contract, build profile, or toolchain changes may affect them — not applicable: no contract code, no build profile, no toolchain change.

Commands run from the workspace root on the pinned 1.91.0 toolchain:

  • cargo fmt --all -- --check → clean.
  • cargo clippy --workspace --all-targets -- -D warnings → clean, no warnings.
  • scripts/check-module-reachability.sh → "all .rs files under src/ are reachable".
  • cargo test -p cargo-budget-report → 768 unit tests pass, 60 more than before this branch
    (the 8 → 68 delta above), plus generate-manpage 21 and exit_code_tests 5.
  • cargo llvm-cov for the before/after table above. Note --branch cannot run on the pinned
    stable toolchain (the option 'Z' is only accepted on the nightly compiler), so the figures are
    line/function/region coverage only — coverage.yml reports coverage and does not gate on it.

One test in the crate's integration suite fails on this machine and does not fail on CI:
soroban_sdk_stub_compiles_as_workspace_member, with
ld: dynamic executables or dylibs must link with libSystem.dylib. It is a macOS cdylib-linking
limitation for the generated fixture crates (CI runs ubuntu-latest). I reproduced it identically
on an untouched worktree at main's tip, so it is pre-existing and unrelated to this change.

Reviewer notes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Test-only change (plus a CHANGELOG entry). No production behaviour altered; the two defective
behaviours found are pinned by tests and reported here rather than fixed, per the issue's scope
guidance.

…Tollcraft#696)

Take cargo-budget-report/src/arg_spec.rs from 85.5% to 100% production-line
coverage (cargo llvm-cov), 8 tests to 68. Ten lines of the renderer were
unreachable before: the forwarded boolean, the non-scalar rejection, both
explicit and invalid `bool` values, malformed hex for `bytes`/`bytesn`, and
the whole explicit-`address` branch.

Covers all ten scalar keywords, both bytes keywords, all four structured
keywords, generate-derived addresses at every seed length (including empty
and multibyte names), the unknown/mistyped/`type` vocabulary, and the
untagged-enum + deny_unknown_fields deserialisation surface.

Two defects are pinned as current behaviour rather than fixed, since this
issue is tests-only: a TOML datetime under `type = "json"` forwards
{"$__toml_private_datetime": "..."}, and `generate = true` reads the name
through a `seed[i % seed.len()]` window, so "ab" and "abab" derive the same
dummy address. Both are documented in the tests that pin them.
@drips-wave

drips-wave Bot commented Sep 29, 2026

Copy link
Copy Markdown

@Dreal-pyper Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@mergekeeper

mergekeeper Bot commented Sep 29, 2026

Copy link
Copy Markdown

MergeKeeper review

Scope: in scope for linked issue #152.
Verdict: clean

This PR adds comprehensive unit-test coverage for cargo-budget-report/src/arg_spec.rs fulfilling issue #696 without modifying any production code paths. All added tests pass successfully and clean up edge-case coverage gaps as requested.

Reviewed commit: a860b7d76df5731657303a4fff1d46dc9f794632.
CI and merge eligibility are checked separately.

@mallison031
mallison031 merged commit 33c8eb7 into Tollcraft:main Sep 29, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add comprehensive unit tests for cargo-budget-report/src/arg_spec.rs

2 participants