test(rs-sdk): relocate DPNS network tests from src/ to tests/#3721
Open
lklimek wants to merge 5 commits into
Open
test(rs-sdk): relocate DPNS network tests from src/ to tests/#3721lklimek wants to merge 5 commits into
lklimek wants to merge 5 commits into
Conversation
…ORM_* vars Five DPNS network tests (one in queries.rs, four in contested_queries.rs) hardcoded https://52.12.176.90:1443 (a testnet HPMN now half-up — TCP listens, TLS hangs), which silently bypassed packages/rs-sdk/tests/.env and made vector regeneration against a local devnet or SSH tunnel impossible. Introduce a shared #[cfg(test)] helper test_address::test_address_list() that loads tests/.env (best-effort) and builds an AddressList from DASH_SDK_PLATFORM_HOST / DASH_SDK_PLATFORM_PORT / DASH_SDK_PLATFORM_SSL — matching the existing Config schema used by tests/fetch/config.rs. The five hardcoded blocks now call this helper. Network/with_network(Testnet) is left untouched.
…and-aid endpoint helper
Five #[cfg(test)] blocks in src/platform/dpns_usernames/{queries,contested_queries}.rs
were really network-integration tests — they only used pub methods on Sdk (search,
availability, resolve, contested-vote-state, etc.) and shipped their own band-aid
test_address.rs helper to read DASH_SDK_PLATFORM_* env vars. They belong in tests/,
where the existing fetch::Config harness already loads tests/.env via dotenvy/envy
and exposes address_list() against the very same env vars.
This commit:
- Relocates all five #[ignore] network tests into a new tests/dpns_usernames.rs
integration binary that re-mounts fetch::config + fetch::generated_data
(#[path] sub-modules) so it can call Config::new().address_list() instead of
parsing a hardcoded URL.
- Folds the previously hardcoded tests/dpns_queries_test.rs (which also pinned
https://52.12.176.90:1443) into the same file via the shared
build_testnet_sdk() helper, deleting the old file.
- Deletes src/platform/dpns_usernames/test_address.rs (the band-aid) and the
five #[cfg(test)] mod tests blocks. No public-API change — the production
helpers all stay where they were.
Net: -test_address.rs, -2 inline mod tests, -dpns_queries_test.rs,
+1 unified tests/dpns_usernames.rs, +consistent .env-driven endpoint config.
Run with:
cargo test -p dash-sdk --features generate-test-vectors -- --include-ignored
… local Core RPC quorum lookup build_testnet_sdk used TrustedHttpContextProvider::new(Network::Testnet, ...), which fetches quorum info from a hardcoded testnet HTTP endpoint. Three of the seven relocated DPNS tests (test_dpns_queries, test_dpns_queries_from_docs, test_dpns_search_variations) panicked with `Failed to find quorum: Quorum not found for type 106 and hash ...` when run against a local devnet or SSH tunnel — the testnet quorums simply don't match the local chain. Every other network test in `tests/fetch/*` solves this by going through `Config::setup_api(namespace)`, which wires the SDK with `SdkBuilder::with_core(host, port, user, password)`. Quorums are then resolved via the local Dash Core RPC the user is already running. This commit: - Replaces build_testnet_sdk with a one-line async helper that delegates to `Config::new().setup_api(namespace).await`. - Drops TrustedHttpContextProvider, build_testnet_context_provider, and the manual DPNS system-contract pre-load (Core RPC + standard fetch path is enough — no test depended on the pre-loaded contract for assertions). - Gives each test a unique namespace so per-test vector dump dirs don't collide when running with `--features generate-test-vectors`. The Network::Testnet hint is dropped along with the trusted context provider — `Config::setup_api` doesn't expose the network field, and the canonical `tests/fetch/*` suite operates without it just fine for proof verification via Core RPC.
…(chain-agnostic)
The relocated test asserted testnet-specific data: that "alice" was registered,
that it resolved to a hardcoded identity, that a prefix search returned at
least one row. Against a local devnet or any reset chain those assertions
fail — the chain simply doesn't have that data.
Downgrade to a call-and-log smoke test (renamed test_dpns_queries_smoke):
each SDK call still uses .expect("…should succeed") so transport or
proof-verification failures still crash loudly, but the existence of
specific on-chain rows is logged, not asserted. The test now exercises the
DPNS query code paths against any network.
Contributor
📝 WalkthroughWalkthroughInline DPNS integration tests scattered across source files and an earlier test module are consolidated into a new dedicated test file. The consolidation removes test blocks from ChangesDPNS Integration Test Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6 tasks
lklimek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk/tests/dpns_usernames.rs (1)
37-653:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid live network calls in integration tests; use mocked dependencies.
These tests directly hit Platform/Core RPC. Even with
#[ignore], this violates the test isolation rule and keeps this suite environment-coupled.As per coding guidelines, "Unit and integration tests should not perform network calls; mock dependencies."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk/tests/dpns_usernames.rs` around lines 37 - 653, Tests currently perform live network calls (they call build_testnet_sdk and then methods like search_dpns_names, check_dpns_name_availability, resolve_dpns_name_to_identity, get_dpns_usernames_by_identity, get_contested_dpns_normalized_usernames, get_contested_dpns_vote_state, get_contested_dpns_usernames_by_identity, get_contested_dpns_identity_votes, get_current_dpns_contests, get_contested_non_resolved_usernames, get_non_resolved_dpns_contests_for_identity), which must be replaced with mocked dependencies; refactor by introducing a build_mock_sdk (or extend build_testnet_sdk to accept a Transport/Client trait impl) and update the tests (test_dpns_queries, test_contested_queries, test_get_current_dpns_contests, test_get_contested_non_resolved_usernames, test_get_non_resolved_dpns_contests_for_identity, test_dpns_queries_smoke, test_dpns_search_variations) to use the mock that stubs expected RPC responses (use a mocking crate like mockall or wiremock), return deterministic data for the SDK methods listed above, and remove live network usage so tests run in isolation and deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-sdk/tests/dpns_usernames.rs`:
- Around line 74-90: Replace the current logging-only error handling for SDK
calls with fail-fast behavior: instead of printing errors in the Err arm for
get_contested_dpns_normalized_usernames (and the other similar calls listed),
assert or unwrap the Result so the test fails on transport/proof/query errors;
locate uses of sdk.get_contested_dpns_normalized_usernames (and the other SDK
calls at the referenced ranges) and change the match/Err branches to call
unwrap() or use assert!(result.is_ok(), "...") with a concise message so
failures stop the test run immediately.
---
Outside diff comments:
In `@packages/rs-sdk/tests/dpns_usernames.rs`:
- Around line 37-653: Tests currently perform live network calls (they call
build_testnet_sdk and then methods like search_dpns_names,
check_dpns_name_availability, resolve_dpns_name_to_identity,
get_dpns_usernames_by_identity, get_contested_dpns_normalized_usernames,
get_contested_dpns_vote_state, get_contested_dpns_usernames_by_identity,
get_contested_dpns_identity_votes, get_current_dpns_contests,
get_contested_non_resolved_usernames,
get_non_resolved_dpns_contests_for_identity), which must be replaced with mocked
dependencies; refactor by introducing a build_mock_sdk (or extend
build_testnet_sdk to accept a Transport/Client trait impl) and update the tests
(test_dpns_queries, test_contested_queries, test_get_current_dpns_contests,
test_get_contested_non_resolved_usernames,
test_get_non_resolved_dpns_contests_for_identity, test_dpns_queries_smoke,
test_dpns_search_variations) to use the mock that stubs expected RPC responses
(use a mocking crate like mockall or wiremock), return deterministic data for
the SDK methods listed above, and remove live network usage so tests run in
isolation and deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cb51ba8-f2b6-45b4-b12a-d0d4a2ba4e1c
📒 Files selected for processing (4)
packages/rs-sdk/src/platform/dpns_usernames/contested_queries.rspackages/rs-sdk/src/platform/dpns_usernames/queries.rspackages/rs-sdk/tests/dpns_queries_test.rspackages/rs-sdk/tests/dpns_usernames.rs
💤 Files with no reviewable changes (3)
- packages/rs-sdk/src/platform/dpns_usernames/queries.rs
- packages/rs-sdk/tests/dpns_queries_test.rs
- packages/rs-sdk/src/platform/dpns_usernames/contested_queries.rs
Comment on lines
+74
to
+90
| println!("Testing get_contested_dpns_usernames..."); | ||
| let all_contested = sdk | ||
| .get_contested_dpns_normalized_usernames(Some(5), None) | ||
| .await; | ||
| match &all_contested { | ||
| Ok(names) => { | ||
| println!("Successfully queried contested DPNS usernames"); | ||
| println!("Found {} contested DPNS usernames", names.len()); | ||
| for name in names { | ||
| println!(" - {}", name); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| println!("Could not fetch contested names (may not exist): {}", e); | ||
| println!("This is expected if there are no contested names on testnet."); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on SDK call errors instead of logging and passing.
Several tests currently treat transport/proof/query failures as informational logs, which can produce green runs even when the API path is broken.
Proposed fix pattern
- let all_contested = sdk
- .get_contested_dpns_normalized_usernames(Some(5), None)
- .await;
- match &all_contested {
- Ok(names) => {
- println!("Successfully queried contested DPNS usernames");
- println!("Found {} contested DPNS usernames", names.len());
- }
- Err(e) => {
- println!("Could not fetch contested names (may not exist): {}", e);
- println!("This is expected if there are no contested names on testnet.");
- }
- }
+ let all_contested = sdk
+ .get_contested_dpns_normalized_usernames(Some(5), None)
+ .await
+ .expect("get_contested_dpns_normalized_usernames should succeed");
+ println!("Successfully queried contested DPNS usernames");
+ println!("Found {} contested DPNS usernames", all_contested.len());Also applies to: 255-259, 282-285, 307-308, 341-343, 413-416, 517-520, 536-538, 547-550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-sdk/tests/dpns_usernames.rs` around lines 74 - 90, Replace the
current logging-only error handling for SDK calls with fail-fast behavior:
instead of printing errors in the Err arm for
get_contested_dpns_normalized_usernames (and the other similar calls listed),
assert or unwrap the Result so the test fails on transport/proof/query errors;
locate uses of sdk.get_contested_dpns_normalized_usernames (and the other SDK
calls at the referenced ranges) and change the match/Err branches to call
unwrap() or use assert!(result.is_ok(), "...") with a concise message so
failures stop the test run immediately.
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "64e3ebb2483e76e373b3a1c19f92c6f632169191462af5a3165486f9407ae3f2"
)Xcode manual integration:
|
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the only agent finding against 67716a719e32a708fe5e87d8e9775c9a9796290f and do not see a reviewable defect in this PR. The reduction from fixture-based assertions to smoke checks in test_dpns_queries_smoke is intentional, documented in the code and commit history, and necessary for the test's new goal of running against arbitrary networks configured via tests/.env rather than a hardcoded testnet endpoint.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Relocates 5 network-integration tests for DPNS username helpers out of
src/platform/dpns_usernames/{queries.rs,contested_queries.rs}(where they sat as#[cfg(test)] mod testsblocks) into a newpackages/rs-sdk/tests/dpns_usernames.rs, and wires them through the existing test harness (Config::new().setup_api(namespace).awaitfromtests/fetch/config.rs).Split off from #3711 so the V0/V1 wire compatibility fix can land focused; this PR carries the pure test-organization cleanup.
What's wrong today (on v3.1-dev)
src/even though they exercise onlypub Sdkextension methods. They belong intests/.https://52.12.176.90:1443(a testnet HPMN, currently half-up — TCP listens, TLS dead). The endpoint is invisible totests/.env/Config.TrustedHttpContextProvider::new(Network::Testnet, ...)which fetches quorum info from a hardcoded testnet HTTP endpoint. Against a local devnet (e.g.,dashmate+SDK_TEST_DATA=true), quorums don't match →Failed to find quorum: Quorum not found for type 106 ….test_dpns_queries_from_docsasserts the testnet identity5DbLwAxGBzUzo81VewMUwn4b5P4bpv9FNFybi25XB5Bkexists — fails on any reset chain.tests/dpns_queries_test.rsrepeats the same hardcoded endpoint pattern; another file to maintain.What this PR does
09df598f3src/.../test_address.rsreadingDASH_SDK_PLATFORM_HOST/PORT/SSLfromtests/.envto unblock vector regen on local devnets9745211854test_address.rs+ the two#[cfg(test)] mod testsblocks insrc/.../. Deletetests/dpns_queries_test.rs. Createtests/dpns_usernames.rswith all 7 relocated tests sharing onebuild_testnet_sdk()helper. Re-mounttests/fetch/config.rs+tests/fetch/generated_data.rsvia#[path = "fetch/…"]bd7117a1f7build_testnet_sdk()throughConfig::new().setup_api(namespace).await— switches the SDK offTrustedHttpContextProvideronto the canonicalwith_core(host, port, user, password)path that resolves quorums via local Dash Core RPC75ed1d4203test_dpns_queries_from_docs→test_dpns_queries_smoke: all four existence assertions downgraded toprintln!. SDK calls still use.expect(…)so transport/proof errors panic loudly. Smoke guarantee is now "every fetch path returnedOk," not "the chain has alice."Net effect: 5 tests moved out of
src/, 1 file deleted (tests/dpns_queries_test.rs— folded in), 1 file created (tests/dpns_usernames.rs), 0 production-code changes. The methods under test were alreadypubonSdk; no surface change.The band-aid commit (
09df598f3) is preserved in history rather than squashed — it's the smallest possible step that unblocked vector regen, useful as a bisect waypoint. If you prefer a single commit, squash on merge.Test plan
cargo check -p dash-sdk --testscargo clippy -p dash-sdk --tests -- -D warningscargo fmt --alltests/.envpopulated:cargo test -p dash-sdk --features generate-test-vectors dpns_usernames -- --include-ignoredSDK_TEST_DATA=true— see Seed DPNS contested-name prerequisites via SDK_TEST_DATA (extend create_sdk_test_data) #3720 for the prerequisite gap ontest_contested_queries).Out of scope
create_sdk_test_dataextension to seed DPNS contested-name prerequisites fortests/fetch/contested_resource.rs— tracked separately in Seed DPNS contested-name prerequisites via SDK_TEST_DATA (extend create_sdk_test_data) #3720.Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit