Skip to content

tests: improve unit test coverage and set threshold#296

Open
pdp2121 wants to merge 7 commits into
mainfrom
improve-unit-test
Open

tests: improve unit test coverage and set threshold#296
pdp2121 wants to merge 7 commits into
mainfrom
improve-unit-test

Conversation

@pdp2121
Copy link
Copy Markdown
Collaborator

@pdp2121 pdp2121 commented May 4, 2026

High Level Overview of Change

Brings xrpl-rust unit-test coverage in line with 85% line gate by adding ~120 unit tests to previously-untested or low-coverage files, and reframing what "unit-test coverage" measures.

Tests added

  • 29 request models — were all at 0%, now 84–100%
  • 3 pseudo-transactions (enable_amendment, set_fee, unl_modify) — 0% → 91–94%
  • xchain_add_claim_attestation (0% → 92%), xchain_commit (45% → 95%)
  • 3 ledger objects (bridge, xchain_owned_claim_id, objects/mod.rs) — 0% → 87–100%
  • utils/str_conversion and utils/get_xchain_claim_id — 0% → 98–100%
  • models/results/mod.rs dispatcher (17% → 58%), and low-coverage result types account_tx/ledger_entry/tx/nftoken — 0–34% → 98–100%
  • txn_parser internals (nodes, parser, utils/mod) — 51–62% → 97–100%
  • 6 lower-coverage transaction models (check_cancel, escrow_cancel, set_regular_key, ticket_create, payment_channel_fund, check_create) — 68–80% → 97%
  • transaction/mod.rs (calculate_fee_per_transaction_type) — 0% → 44%

CI changes

  • Thresholds raised: lines 73 → 85, regions 75 → 85, functions 67 → 75
  • Excluded integration-territory files from the unit-test coverage report — CLI, async network clients (asynch/clients/**), sync wrappers around network calls (account/, ledger/, transaction/mod.rs), and the faucet client. These are exercised by the existing tests/integration/ suite (behind the integration feature flag) and would more naturally fit a separate integration-coverage gate.

Context of Change

xrpl-py is currently at 85% unit-test and 70% integration test coverage.

Type of Change

  • 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 not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Test Plan

All CI tests pass.

Add integration test coverage threshold

@pdp2121 pdp2121 changed the title Improve unit test tests: improve unit test coverage and set threshold May 4, 2026
@pdp2121
Copy link
Copy Markdown
Collaborator Author

pdp2121 commented May 4, 2026

/ai-review

@pdp2121 pdp2121 requested a review from ckeshava May 4, 2026 22:21
Copy link
Copy Markdown

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues.

Review by Claude Opus 4.6 · Prompt: V15

id: Option<Cow<'a, str>>,
destination_account: Cow<'a, str>,
destination_amount: Currency<'a>,
destination_amount: Amount<'a>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we flag this type change in the changelog? Currency -> Amount. Thanks for fixing it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically, this is a breaking change, but I am not suggesting a major-version upgrade for this ^^

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Display)]
#[strum(serialize_all = "snake_case")]
#[serde(rename_all = "snake_case")]
#[serde(tag = "role")]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call out this change in the changelog? Theoretically, this is a breaking change because the serialized version of this enum will no longer have the role discriminator.

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.

3 participants