test(swift-sdk): move swift-sdk unit test defined in the example app to the swift-sdk unit tests suite#3917
test(swift-sdk): move swift-sdk unit test defined in the example app to the swift-sdk unit tests suite#3917ZocoLini wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThree wallet test files ( ChangesWallet Test Cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
✅ Review complete (commit d7721e5) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR moves 8 Swift SDK test files from the example app test target to the SwiftDashSDK SPM test target and removes 3 skip-only stub files. The move is mechanically clean (target exists, only @testable imports are SwiftDashSDK, no stale references remain in the Xcode project). However, one of the moved tests is a live testnet integration test that will now run as part of swift test, undermining the PR's goal of producing a fast, simulator/network-free unit suite. Head verified at a2e346c.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/SDKMethodTests.swift`:
- [BLOCKING] packages/swift-sdk/SwiftTests/SwiftDashSDKTests/SDKMethodTests.swift:90-110: Live testnet fetch moved into the fast unit test target
`testSimpleIdentityFetch` constructs a real `.testnet` `SDK`, calls `sdk.identityGet` against a hard-coded testnet identity (`5DbLwAxGBzUzo81VewMUwn4b5P4bpv9FNFybi25XB5Bk`), and rethrows on failure. The file now lives under `SwiftTests/SwiftDashSDKTests`, which `packages/swift-sdk/run_tests.sh` exercises via `swift test` before the simulator pass. That converts the SDK unit suite into something dependent on testnet DNS, masternode availability, and the continued existence of that identity — exactly the kind of external dependency this reorganization was supposed to keep out of the fast path. Either gate this test behind an environment flag, move it to a dedicated integration target, or rewrite it against a mocked transport so `swift test` stays deterministic.
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: Implementation verified for system consistency and engineering integrity.
…o the swift-sdk unit tests suite
a2e346c to
d7721e5
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward review of PR #3917 at d7721e5. The PR is a clean test-suite reorganization (moves 7 tests into the swift-sdk SwiftTests target, deletes 3 skip-only Wallet test stubs). No new in-scope issues surfaced in the latest delta. The single prior finding — a live testnet identity fetch promoted into the fast swift test target — remains STILL VALID at HEAD and is carried forward as blocking. Verified directly: SDKMethodTests.swift:90-110 constructs SDK(network: .testnet) and awaits sdk.identityGet on a hard-coded identity, and run_tests.sh executes swift test (line 27) before the simulator pass, so the supposedly fast/emulator-free suite now depends on external testnet availability.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/SDKMethodTests.swift`:
- [BLOCKING] packages/swift-sdk/SwiftTests/SwiftDashSDKTests/SDKMethodTests.swift:90-110: Live testnet fetch lives in the fast `swift test` target the PR is trying to make emulator-free
Carried forward from the prior review and re-verified at HEAD d7721e58. The relocated `testSimpleIdentityFetch` constructs a real `SDK(network: .testnet)` and awaits `sdk.identityGet(identityId: "5DbLwAxGBzUzo81VewMUwn4b5P4bpv9FNFybi25XB5Bk")`, rethrowing any failure. Because the file now sits under `SwiftTests/SwiftDashSDKTests/`, `packages/swift-sdk/run_tests.sh` runs it via `swift test` (line 27) before the simulator pass. That directly contradicts this PR's stated goal of making the unit-test suite emulator-/network-free: the suite now depends on (1) outbound testnet connectivity from the CI runner, (2) DNS for testnet seed nodes, (3) masternode availability, and (4) the specific identity `5DbLwAxGBzUzo81VewMUwn4b5P4bpv9FNFybi25XB5Bk` continuing to exist on testnet. Any of those breaking flips the suite red with no signal about the SDK code itself, and over time normalizes ignoring its failures — masking real FFI/SDK regressions. The two sibling tests in the same file (`testSDKMethodsAvailability`, `testDirectMethodCall`) are correctly hermetic — only this one is the outlier. Gate it behind an env flag (e.g. `XCTSkipUnless` on `DASH_RUN_LIVE_TESTNET_TESTS=1`), move it to a dedicated integration target excluded from `swift test`, or rewrite it against a mocked transport.
Just moved some tests that can belong to the swift-sdk test suite to their place. This should speed up CI since no emulator is needed to execute them.
I also removed 3 empty test files that were triggering an emulator start for nothing
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
XCTSkipbehavior.