Skip to content

Commit 2260c3f

Browse files
ckeshavapdp2121claude
authored andcommitted
feat: enforce 65% integration-test coverage and add tests (XRPLF#298)
## High Level Overview of Change Closes XRPLF#297. The CI integration workflow now runs all five integration test binaries (previously only `integration_test`) under `cargo-llvm-cov` and enforces a 65% line-coverage gate scoped to integration-territory files. Six commits add tests that lift integration coverage from 44.52% → 70.30% to satisfy the gate. Beyond the integration gate, this PR also adds `is_success()` unit tests (`src/models/results/mod.rs`), splits the codecov patch gate into per-flag sections (unit/integration), fixes `get_latest_open_ledger_sequence` (it requested `ledger { ledger_index: "open" }`, which rippled rejects), and documents both coverage flows in `CONTRIBUTING.md`. (The unit thresholds — 83% lines / 85% regions / 73% functions — were set earlier in XRPLF#296 and are unchanged here.) ### Context of Change The integration suite was running only `tests/integration_test.rs` (40 transaction tests, all routed through one `test_transaction` helper). Several public APIs sat at 0% integration coverage: the sync wrappers in `src/{account,ledger,transaction}/mod.rs`, `asynch::transaction::submit_and_wait`'s poll loop, the WebSocket request/response path, multisign, the sync faucet generator, and the `clients::json_rpc::JsonRpcClient` sync facade. xrpl-py gates its integration suite at 70%; this PR starts xrpl-rust at 65%, with room to tighten as coverage grows. ### Type of Change - [x] New feature (CI coverage gate) - [x] Tests - [x] Bug fix (`get_latest_open_ledger_sequence`) - [x] Documentation (CONTRIBUTING coverage guide) ## Before / After **Workflow.** - **Before:** `cargo test --test integration_test …` — ran a single test binary, no coverage measured. - **After:** coverage runs in three steps: 1. `cargo llvm-cov --no-report … --test integration_test --test cli_integration --test funding --test utils --test test_utils` — collects coverage across all five integration binaries. 2. `cargo llvm-cov report --lcov --ignore-filename-regex "$COVERAGE_IGNORE_REGEX"` — writes an lcov scoped to integration-territory files. 3. That lcov is uploaded to codecov, which enforces the 65% gate (`codecov.yml → project.integration`), and is also saved as a CI artifact. `COVERAGE_IGNORE_REGEX` is defined once (workflow `env`), so the scoped report and the gate always use the same file list. **Scoped coverage:** 44.52% lines → **70.30% lines**. Notable per-file moves: | File | Before | After | |---|---|---| | `account/mod.rs` (sync) | 0% | 100% | | `ledger/mod.rs` (sync) | 47.37% | 100% | | `transaction/mod.rs` (sync) | 0% | 100% | | `wallet/faucet_generation.rs` | 0% | 100% | | `asynch/ledger/mod.rs` | 62.50% | 94.64% | | `asynch/account/mod.rs` | 55.17% | 95.40% | | `asynch/clients/client.rs` | 0% | 91.67% | | `asynch/clients/websocket/websocket_base.rs` | 12.73% | 78.18% | | `asynch/transaction/submit_and_wait.rs` | 0% | 72.87% | ## Test Plan All tests run against `rippleci/xrpld:develop` (CI's standalone rippled image). New test files / functions: - `tests/transactions/submit_and_wait.rs::test_submit_and_wait_payment` — autofill, sign, submit, poll loop, with a background `ledger_accept` driver. - `tests/utils.rs` — three new WS tests: `server_info` round-trip + close, `account_info` against genesis, three sequential requests on one connection (exercises id-based message routing). - `tests/transactions/sync_wrappers.rs` — 10 tests covering every sync wrapper in `src/{account,ledger,transaction}/mod.rs`, plus `clients::json_rpc::JsonRpcClient` and `wallet::faucet_generation`. Each owns a `tokio::runtime::Runtime` + `EnterGuard` so `reqwest` can find the reactor while `embassy_futures::block_on` parks the thread. - `tests/transactions/multisign_payment.rs::test_multisign_payment` — register a 2-of-2 signer list, autofill with `signers_count=2`, each signer signs a copy, `multisign()` merges, `submit()` confirms `tesSUCCESS`. Verified locally by running the same `cargo llvm-cov` invocation CI uses, with the same exclusion regex; the scoped report clears the 65% gate. --------- Co-authored-by: Phu Pham <ppham@ripple.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5a50e0f commit 2260c3f

15 files changed

Lines changed: 775 additions & 53 deletions

File tree

.github/workflows/integration_test.yml

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ env:
1313
# rippled binary was renamed to xrpld; use the new image name.
1414
# Tracks xrpl.js PR #3270 (https://github.com/XRPLF/xrpl.js/pull/3270).
1515
XRPLD_DOCKER_IMAGE: rippleci/xrpld:develop
16+
# Files measured by the integration coverage gate are the inverse of this regex.
17+
# Unit-test territory (models, core, utils, _serde) is excluded; integration territory
18+
# (CLI, asynch::clients, account/, ledger/, transaction/, faucet) is what remains.
19+
COVERAGE_IGNORE_REGEX: '(_serde|core|models|utils)/|constants\.rs$|macros\.rs$|lib\.rs$|wallet/(mod|exceptions)\.rs$|tests/'
1620

1721
jobs:
1822
integration_test:
@@ -36,6 +40,11 @@ jobs:
3640
3741
- uses: dtolnay/rust-toolchain@stable
3842

43+
- name: Install cargo-llvm-cov
44+
uses: taiki-e/install-action@65851e10cd6c377f11a60e600abc07cb08643468 # v2.79.3
45+
with:
46+
tool: cargo-llvm-cov
47+
3948
- name: Cache cargo registry
4049
uses: actions/cache@v4
4150
with:
@@ -47,11 +56,40 @@ jobs:
4756
restore-keys: |
4857
${{ runner.os }}-cargo-integration-
4958
50-
- name: Run integration tests
51-
run: cargo test --release --features std,json-rpc,helpers,integration --test integration_test -- --test-threads=1
59+
- name: Run integration tests under coverage
60+
run: |
61+
cargo llvm-cov --no-report --release \
62+
--features std,json-rpc,helpers,cli,websocket,integration \
63+
--test integration_test \
64+
--test cli_integration \
65+
--test funding \
66+
--test utils \
67+
--test test_utils \
68+
-- --test-threads=1
5269
env:
5370
RUST_BACKTRACE: 1
5471

72+
- name: Generate lcov report (scoped to integration territory)
73+
run: |
74+
cargo llvm-cov report --release \
75+
--lcov --output-path lcov.info \
76+
--ignore-filename-regex '${{ env.COVERAGE_IGNORE_REGEX }}'
77+
78+
- name: Upload coverage to Codecov
79+
uses: codecov/codecov-action@75cd11691c0faa626561e295848008c8a7dddffe # v5.5.4
80+
with:
81+
token: ${{ secrets.CODECOV_TOKEN }}
82+
files: lcov.info
83+
flags: integration
84+
fail_ci_if_error: true
85+
86+
- name: Upload coverage report
87+
if: always()
88+
uses: actions/upload-artifact@v4
89+
with:
90+
name: integration-lcov-report
91+
path: lcov.info
92+
5593
- name: Dump xrpld logs and stop container
5694
if: always()
5795
run: |

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1717
- New `xrpl::signing` module containing the pure-crypto signing helpers (`sign`, `multisign`, `prepare_transaction`) extracted from `asynch::transaction` and `transaction`. Available with just `core + models + wallet` features (no `helpers`/runtime/client dependency). The legacy paths `asynch::transaction::sign` and `transaction::multisign` are preserved as re-exports for backward compatibility.
1818
- Expanded unit-test coverage and raised CI thresholds: lines `73 → 83`, regions `75 → 85`, functions `67 → 73`.
1919
- Codecov integration with per-PR project (≥83%) and patch (≥80% on new/modified lines) gates.
20+
- Integration-test coverage gate: a CI workflow runs all five integration test binaries under `cargo-llvm-cov`, uploads to codecov under an `integration` flag, and gates the project at ≥65%.
2021

2122
### Changed
2223

2324
- Unit-test and integration-test coverage are now scoped via Cargo feature flags rather than path regex. The unit-test workflow builds with `--no-default-features --features std,core,utils,wallet,models`, so integration-territory code (CLI, async clients, sync wrappers, faucet) simply isn't compiled and doesn't appear in the unit coverage report.
2425
- Network-dependent inline tests in `src/asynch/transaction/` and `src/asynch/wallet/` (`test_autofill_txn`, `test_autofill_and_sign`, `test_submit_and_wait`, `test_generate_faucet_wallet`) are now gated behind `feature = "integration"` so `cargo test --release` is hermetic by default.
26+
- Codecov **patch** coverage is now gated per flag (separate `unit` and `integration` sections) rather than a single combined gate.
2527

2628
### Fixed
2729

2830
- `RipplePathFind::destination_amount` changed from `Currency<'a>` to `Amount<'a>` to match the XRPL wire format.
2931
- `NoRippleCheckRole` no longer serializes with the `#[serde(tag = "role")]` discriminator; now emits a plain `snake_case` string matching the XRPL wire format.
32+
- `is_success()` now reports success correctly for responses deserialized into typed `XRPLResult` variants (e.g. `ServerInfo`); it consults the preserved raw result JSON instead of the re-serialized typed value.
33+
- `get_latest_open_ledger_sequence` now uses the `ledger_current` request; it previously sent `ledger { ledger_index: "open" }`, which rippled rejects with `invalidParams`.
3034

3135
## [[v1.1.0]]
3236

CONTRIBUTING.md

Lines changed: 66 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,24 @@ From the `xrpl-rust` folder, run the following commands:
7676
docker run -p 5005:5005 -p 6006:6006 --rm -it --name xrpld_standalone \
7777
--volume "$PWD/.ci-config/:/etc/xrpld/" \
7878
rippleci/xrpld:develop --standalone
79-
cargo test --release --features integration,std,json-rpc,helpers
79+
cargo test --release \
80+
--features std,json-rpc,helpers,cli,websocket,integration \
81+
-- --test-threads=1
8082
```
8183

8284
To run a specific group of tests (e.g. escrow):
8385

8486
```bash
85-
cargo test --release --features integration,std,json-rpc,helpers escrow
87+
cargo test --release \
88+
--features std,json-rpc,helpers,cli,websocket,integration \
89+
escrow -- --test-threads=1
8690
```
8791

92+
The feature set matches `.github/workflows/integration_test.yml`; `cli` and
93+
`websocket` are required for `cli_integration.rs` and the websocket tests in
94+
`utils.rs` to compile. `--test-threads=1` matches CI and prevents concurrent
95+
tests from racing on the shared `xrpld` container.
96+
8897
Breaking down the `docker run` command:
8998

9099
- `-p 5005:5005 -p 6006:6006` exposes the HTTP JSON-RPC and WebSocket admin ports.
@@ -102,24 +111,69 @@ Breaking down the `docker run` command:
102111

103112
### Coverage
104113

105-
Coverage is measured with [`cargo-llvm-cov`](https://github.com/taiki-e/cargo-llvm-cov).
114+
Coverage is measured with [`cargo-llvm-cov`](https://github.com/taiki-e/cargo-llvm-cov)
115+
and uploaded to codecov under two separate flags:
116+
117+
- **unit** — pure-logic code (models, core, utils, `_serde`, signing). Built
118+
with a minimal feature set so network-bound modules are not compiled.
119+
- **integration** — network-bound code (CLI, async clients, faucet, helpers
120+
under `account/`, `ledger/`, `transaction/`). Scoped via
121+
`--ignore-filename-regex` so unit-territory files do not dilute the
122+
integration metric.
106123

107-
Install the tool and run a coverage report locally:
124+
Install the tool once:
108125

109126
```bash
110127
cargo install cargo-llvm-cov --locked
111-
cargo llvm-cov --summary-only
112128
```
113129

114-
The CI enforces the following minimum thresholds (current baseline is ~78% lines / ~68% regions / ~75% functions, measured with default features only — integration tests are excluded from coverage):
130+
#### Unit coverage
131+
132+
Matches `.github/workflows/unit_test.yml`:
133+
134+
```bash
135+
cargo llvm-cov \
136+
--no-default-features --features std,core,utils,wallet,models \
137+
--summary-only \
138+
--fail-under-lines 83 \
139+
--fail-under-regions 85 \
140+
--fail-under-functions 73
141+
```
142+
143+
The `--fail-under-*` flags mirror the thresholds CI enforces:
144+
145+
| Metric | Threshold |
146+
| --------- | --------- |
147+
| Lines | 83% |
148+
| Regions | 85% |
149+
| Functions | 73% |
150+
151+
#### Integration coverage
152+
153+
Requires the standalone `xrpld` container running (see [Integration Tests](#integration-tests)).
154+
Matches `.github/workflows/integration_test.yml`:
155+
156+
```bash
157+
# Collect coverage from the integration suite (writes raw profile data)
158+
cargo llvm-cov --no-report --release \
159+
--features std,json-rpc,helpers,cli,websocket,integration \
160+
--test integration_test --test cli_integration --test funding \
161+
--test utils --test test_utils \
162+
-- --test-threads=1
163+
164+
# Generate lcov scoped to integration territory
165+
cargo llvm-cov report --release --lcov --output-path lcov.info \
166+
--ignore-filename-regex '(_serde|core|models|utils)/|constants\.rs$|macros\.rs$|lib\.rs$|wallet/(mod|exceptions)\.rs$|tests/'
167+
```
168+
169+
The codecov integration target is 65%. The `--ignore-filename-regex` value
170+
mirrors `COVERAGE_IGNORE_REGEX` in the workflow; without it the integration
171+
metric would be dominated by unit-territory files the integration suite is
172+
not designed to exercise.
115173

116-
| Metric | Minimum |
117-
| --------- | ------- |
118-
| Lines | 75% |
119-
| Regions | 65% |
120-
| Functions | 72% |
174+
#### HTML report
121175

122-
To generate an HTML report and open it in a browser:
176+
For local exploration:
123177

124178
```bash
125179
cargo llvm-cov --open

codecov.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,22 @@ coverage:
66
threshold: 1%
77
flags:
88
- unit
9+
integration:
10+
target: 65%
11+
threshold: 1%
12+
flags:
13+
- integration
914
patch:
1015
default:
1116
target: 80%
1217
threshold: 1%
18+
flags:
19+
- unit
20+
integration:
21+
target: 80%
22+
threshold: 1%
23+
flags:
24+
- integration
1325

1426
comment:
1527
layout: "reach, diff, flags, files"
@@ -19,3 +31,6 @@ flags:
1931
unit:
2032
paths:
2133
- src/
34+
integration:
35+
paths:
36+
- src/

src/asynch/ledger/mod.rs

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use core::{cmp::min, convert::TryInto};
33
use alloc::string::ToString;
44

55
use crate::models::{
6-
requests::{fee::Fee, ledger::Ledger},
6+
requests::{fee::Fee, ledger::Ledger, ledger_current::LedgerCurrent},
77
results::{self},
88
XRPAmount,
99
};
@@ -38,26 +38,9 @@ pub async fn get_latest_validated_ledger_sequence(
3838
pub async fn get_latest_open_ledger_sequence(
3939
client: &impl XRPLAsyncClient,
4040
) -> XRPLHelperResult<u32> {
41-
let ledger_response = client
42-
.request(
43-
Ledger::new(
44-
None,
45-
None,
46-
None,
47-
None,
48-
None,
49-
None,
50-
Some("open".into()),
51-
None,
52-
None,
53-
None,
54-
)
55-
.into(),
56-
)
57-
.await?;
58-
let ledger_result: results::ledger::Ledger = ledger_response.try_into()?;
59-
60-
Ok(ledger_result.ledger_index)
41+
let response = client.request(LedgerCurrent::new(None).into()).await?;
42+
let result: results::ledger_current::LedgerCurrent = response.try_into()?;
43+
Ok(result.ledger_current_index)
6144
}
6245

6346
pub enum FeeType {

src/models/results/mod.rs

Lines changed: 80 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -886,18 +886,26 @@ impl<'a> TryInto<XRPLResult<'a>> for XRPLResponse<'a> {
886886
impl<'a> XRPLResponse<'a> {
887887
pub fn is_success(&self) -> bool {
888888
if let Some(status) = &self.status {
889-
status == &ResponseStatus::Success
890-
} else if let Some(result) = &self.result {
891-
match serde_json::to_value(result) {
892-
Ok(value) => match value.get("status") {
893-
Some(Value::String(status)) => status == "success",
894-
_ => false,
895-
},
896-
_ => false,
889+
return status == &ResponseStatus::Success;
890+
}
891+
// Typed `XRPLResult` variants (e.g. `ServerInfo`, `Ping`) drop
892+
// unknown fields like `status` during deserialization. `raw_result`
893+
// preserves the original JSON, so prefer it; fall back to
894+
// re-serializing `result` only for responses constructed without a
895+
// raw payload (mostly tests).
896+
if let Some(raw) = &self.raw_result {
897+
if let Some(Value::String(status)) = raw.get("status") {
898+
return status == "success";
897899
}
898-
} else {
899-
false
900900
}
901+
if let Some(result) = &self.result {
902+
if let Ok(value) = serde_json::to_value(result) {
903+
if let Some(Value::String(status)) = value.get("status") {
904+
return status == "success";
905+
}
906+
}
907+
}
908+
false
901909
}
902910
}
903911

@@ -1207,4 +1215,66 @@ mod tests {
12071215
};
12081216
assert!(!response.is_success());
12091217
}
1218+
1219+
#[test]
1220+
fn test_is_success_uses_raw_result_branch() {
1221+
// raw_result with `status: "success"` returns true even when `status`
1222+
// and `result` are absent — this is the path typed XRPLResult
1223+
// variants (e.g. ServerInfo) rely on, since they drop unknown fields
1224+
// like `status` during deserialization.
1225+
let response = XRPLResponse {
1226+
id: None,
1227+
error: None,
1228+
error_code: None,
1229+
error_message: None,
1230+
forwarded: None,
1231+
request: None,
1232+
result: None,
1233+
raw_result: Some(json!({"status": "success", "info": "x"})),
1234+
status: None,
1235+
r#type: None,
1236+
warning: None,
1237+
warnings: None,
1238+
};
1239+
assert!(response.is_success());
1240+
1241+
// raw_result without a `status` field falls through to false.
1242+
let response = XRPLResponse {
1243+
id: None,
1244+
error: None,
1245+
error_code: None,
1246+
error_message: None,
1247+
forwarded: None,
1248+
request: None,
1249+
result: None,
1250+
raw_result: Some(json!({"info": "no status"})),
1251+
status: None,
1252+
r#type: None,
1253+
warning: None,
1254+
warnings: None,
1255+
};
1256+
assert!(!response.is_success());
1257+
}
1258+
1259+
#[test]
1260+
fn test_is_success_falls_through_when_typed_result_lacks_status() {
1261+
// A typed result variant serializes without a `status` field, so the
1262+
// inner pattern in the `result` fallback branch never matches and
1263+
// control falls through to `false`.
1264+
let response = XRPLResponse {
1265+
id: None,
1266+
error: None,
1267+
error_code: None,
1268+
error_message: None,
1269+
forwarded: None,
1270+
request: None,
1271+
result: Some(XRPLResult::Fee(fee_result())),
1272+
raw_result: None,
1273+
status: None,
1274+
r#type: None,
1275+
warning: None,
1276+
warnings: None,
1277+
};
1278+
assert!(!response.is_success());
1279+
}
12101280
}

tests/cli_integration.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44
//! Genesis account is used for testing since it has unlimited XRP in standalone mode.
55
//! Faucet tests use the public testnet since Docker standalone doesn't have a faucet.
66
7+
#[cfg(all(feature = "integration", feature = "std", feature = "cli"))]
8+
mod common;
9+
710
#[cfg(all(feature = "integration", feature = "std", feature = "cli"))]
811
mod cli_tests {
912
use std::io::{self};
@@ -12,8 +15,10 @@ mod cli_tests {
1215

1316
/// Test-specific constants
1417
mod constants {
15-
// Docker standalone JSON-RPC endpoint
16-
pub const TEST_URL: &str = "http://localhost:5005";
18+
// Docker standalone JSON-RPC endpoint — canonical definition lives in
19+
// tests/common/constants.rs; re-exported here so existing TEST_URL
20+
// references keep working.
21+
pub use crate::common::constants::STANDALONE_URL as TEST_URL;
1722

1823
// Public testnet for faucet tests only
1924
pub const TESTNET_URL: &str = "https://s.altnet.rippletest.net:51234";

tests/common/constants.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,6 @@ pub const XRPL_WSS_TEST_NET: &'static str = "wss://testnet.xrpl-labs.com/";
88
pub const XRPL_WS_TEST_NET: &'static str = "wss://s.altnet.rippletest.net:51233/";
99

1010
pub const GENESIS_ACCOUNT: &str = "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh";
11+
12+
/// HTTP JSON-RPC endpoint for local Docker standalone rippled.
13+
pub const STANDALONE_URL: &str = "http://localhost:5005";

0 commit comments

Comments
 (0)