test(swift-sdk): remove scaffold tests and fix tautological assertions#3222
Draft
thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
Draft
test(swift-sdk): remove scaffold tests and fix tautological assertions#3222thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
Conversation
Collaborator
Author
|
@coderabbitai review |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRemoved multiple example/debug test files, tightened assertions in several unit tests, updated a C mock header and its returned version string handling, and adjusted memory-management and SDK version tests to perform stricter checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift (1)
70-72:⚠️ Potential issue | 🟡 MinorEarly return on signer creation failure should fail the test.
If signer creation fails, the test returns early without asserting a failure. This masks potential issues with the signer creation code path and could cause the test to appear to pass when it actually didn't execute the intended assertion.
🐛 Proposed fix
guard signerResult.error == nil, let signer = signerResult.data else { - print("Failed to create signer") - return + XCTFail("Failed to create signer: \(String(describing: signerResult.error))") + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift` around lines 70 - 72, The test currently prints an error and returns when signer creation fails, which masks failures; change the guard fallback to call XCTFail with a descriptive message instead of print (e.g., inside the guard for the signer variable in SDKMethodTests.swift replace print("Failed to create signer") and return with XCTFail("Failed to create signer: <optional details>") and then return or use XCTAssertNotNil on signer to ensure the test fails when signer is nil. Ensure you reference the existing 'signer' binding in the guard so the test fails explicitly if signer creation fails.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift (2)
84-87: Empty catch block silently accepts any error without validation.The catch block now only contains a comment and doesn't validate that the error is the expected error. If
transferCreditsfails for an unexpected reason (e.g., network issue, SDK initialization problem), the test still passes silently.Consider asserting on the error type or message to ensure the failure is for the expected reason:
♻️ Proposed fix to validate expected error
XCTFail("transferCredits should fail with dummy data") } catch { - // Expected: dummy data causes an error + // Expected: dummy data causes an error - verify it's the right kind of failure + let errorDescription = String(describing: error) + XCTAssertTrue( + errorDescription.contains("invalid") || errorDescription.contains("parameter") || errorDescription.contains("NotImplemented"), + "Unexpected error type: \(error)" + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift` around lines 84 - 87, The test currently swallows all errors in the catch and can pass for unexpected failures; change it to explicitly assert the expected failure from transferCredits by using XCTAssertThrowsError (or replace the empty catch with assertions) and validate the thrown error is the expected one (e.g., cast to the SDK error enum/class or check error.localizedDescription contains the expected message). Locate the transferCredits call and the surrounding test where XCTFail is used and update the test to assert the specific error type/message rather than leaving the catch block empty.
60-76: Consider usingTestSignerfor signer creation in tests.The signer is created directly via
dash_sdk_signer_create_from_private_keywith manual memory management. The coding guidelines specify usingTestSignerfor transaction signing in test files, which would improve consistency and reduce boilerplate.As per coding guidelines: "Use TestSigner for transaction signing in test files" for files matching
packages/swift-sdk/SwiftExampleApp/**/*Test*.swift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift` around lines 60 - 76, The test currently creates a signer manually by calling dash_sdk_signer_create_from_private_key, handling signerResult, SignerHandle and defer-destroy logic; replace that block with the project TestSigner helper per guidelines (use TestSigner for transaction signing in tests) so tests use TestSigner to create/init the signer from the same key variable and avoid manual memory management and explicit dash_sdk_signer_destroy/SignerHandle handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift`:
- Around line 227-238: The test testDoubleFreeProtection contains undefined
behavior by calling swift_dash_string_free(version) twice on the same pointer;
remove the double-free by either deleting the entire test function or rewriting
it to only free once and assert expected behavior (e.g., if the SDK guarantees
nullification, verify that the pointer becomes nil/invalid via documented API
rather than calling swift_dash_string_free a second time). Locate
testDoubleFreeProtection in MemoryManagementTests.swift and remove the second
swift_dash_string_free(version) call (or remove the test) and ensure any
verification follows the SDK's documented guarantees rather than invoking a
second free.
In `@packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift`:
- Around line 15-19: The test is freeing a pointer returned by the mock but the
mock currently returns a string literal, causing undefined behavior; update the
mock implementation of swift_dash_sdk_get_version() to return heap-allocated
memory (e.g., use strdup or malloc+memcpy to allocate and return a C string) so
that swift_dash_string_free(version) is valid, and ensure the mock's ownership
contract matches swift_dash_string_free; alternatively, if you prefer borrowed
semantics, remove the swift_dash_string_free(version) calls in SDKTests.swift
and treat the returned pointer as non-owned—but do one or the other so ownership
is consistent with swift_dash_string_free.
- Around line 29-31: The test currently uses XCTAssertTrue to match
versionString against a regex that allows only a single optional suffix; update
the pattern used in the XCTAssertTrue assertion (the check involving
versionString) so it accepts an optional pre-release segment followed optionally
by a build metadata segment (i.e., allow both a "-" pre-release and a "+" build
metadata, in that order), and ensure the allowed characters for those segments
follow SemVer rules (alphanumeric, hyphen and dot); adjust the regex used in
SDKTests.swift accordingly so strings like "1.2.3-alpha+001" pass.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift`:
- Around line 70-72: The test currently prints an error and returns when signer
creation fails, which masks failures; change the guard fallback to call XCTFail
with a descriptive message instead of print (e.g., inside the guard for the
signer variable in SDKMethodTests.swift replace print("Failed to create signer")
and return with XCTFail("Failed to create signer: <optional details>") and then
return or use XCTAssertNotNil on signer to ensure the test fails when signer is
nil. Ensure you reference the existing 'signer' binding in the guard so the test
fails explicitly if signer creation fails.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift`:
- Around line 84-87: The test currently swallows all errors in the catch and can
pass for unexpected failures; change it to explicitly assert the expected
failure from transferCredits by using XCTAssertThrowsError (or replace the empty
catch with assertions) and validate the thrown error is the expected one (e.g.,
cast to the SDK error enum/class or check error.localizedDescription contains
the expected message). Locate the transferCredits call and the surrounding test
where XCTFail is used and update the test to assert the specific error
type/message rather than leaving the catch block empty.
- Around line 60-76: The test currently creates a signer manually by calling
dash_sdk_signer_create_from_private_key, handling signerResult, SignerHandle and
defer-destroy logic; replace that block with the project TestSigner helper per
guidelines (use TestSigner for transaction signing in tests) so tests use
TestSigner to create/init the signer from the same key variable and avoid manual
memory management and explicit dash_sdk_signer_destroy/SignerHandle handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08370640-be57-4b02-b96f-f34579015b22
📒 Files selected for processing (11)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CrashDebugTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/DebugTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/MinimalAsyncTest.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletTests/TransactionTests.swiftpackages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.cpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
💤 Files with no reviewable changes (6)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletTests/TransactionTests.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CrashDebugTests.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/DebugTests.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/MinimalAsyncTest.swift
packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift
Outdated
Show resolved
Hide resolved
Comment on lines
+15
to
+19
| let version = swift_dash_sdk_get_version() | ||
| XCTAssertNotNil(version, "SDK should provide a version after initialization") | ||
| if let version = version { | ||
| swift_dash_string_free(version) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Mock version getter =="
fd 'SwiftDashSDKMock\.c$' packages/swift-sdk -x sed -n '70,95p' {}
echo
echo "== swift_dash_string_free definitions/usages =="
fd '.*\.(c|h|swift)$' packages/swift-sdk -x rg -n -C2 '\bswift_dash_string_free\s*\(' {}Repository: dashpay/platform
Length of output: 3327
Fix ownership contract: mock must return heap-allocated memory or test must not free the version pointer.
The mock's swift_dash_sdk_get_version() returns the string literal "2.0.0-mock", but the implementation of swift_dash_string_free() at line 381 calls free() on its input. The new test code at lines 18 and 33 invokes swift_dash_string_free(version) on this literal, which is undefined behavior.
Either modify the mock to return heap-allocated memory, or remove the swift_dash_string_free() calls and treat the returned pointer as borrowed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift` around
lines 15 - 19, The test is freeing a pointer returned by the mock but the mock
currently returns a string literal, causing undefined behavior; update the mock
implementation of swift_dash_sdk_get_version() to return heap-allocated memory
(e.g., use strdup or malloc+memcpy to allocate and return a C string) so that
swift_dash_string_free(version) is valid, and ensure the mock's ownership
contract matches swift_dash_string_free; alternatively, if you prefer borrowed
semantics, remove the swift_dash_string_free(version) calls in SDKTests.swift
and treat the returned pointer as non-owned—but do one or the other so ownership
is consistent with swift_dash_string_free.
Collaborator
Author
|
Addressed all 3 CodeRabbit findings:
@coderabbitai review |
Contributor
|
Triggering a fresh review now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift (1)
191-223: Strengthen this leak test back into a real stress loop.Lines 200-220 still only do 10 iterations per allocation path, so this is closer to a smoke test than a leak-regression guard. Bumping this back to a higher iteration count would make memory-growth regressions much easier to catch.
♻️ Suggested tightening
- // 1. Test string allocation and cleanup - for _ in 0..<10 { - let version = swift_dash_sdk_get_version() - if let version = version { - swift_dash_string_free(version) - } - } - - // 2. Test error allocation and cleanup - for _ in 0..<10 { - let result = swift_dash_identity_create(sdk, nil, 0) - if let error = result.error { - swift_dash_error_free(error) - } - } - - // 3. Test token supply allocation and cleanup - for _ in 0..<10 { - let supply = swift_dash_token_get_total_supply(sdk, "test_contract") - if let supply = supply { - swift_dash_string_free(supply) - } - } + for _ in 0..<100 { + let version = swift_dash_sdk_get_version() + if let version = version { + swift_dash_string_free(version) + } + + let result = swift_dash_identity_create(sdk, nil, 0) + if let error = result.error { + swift_dash_error_free(error) + } + + let supply = swift_dash_token_get_total_supply(sdk, "test_contract") + if let supply = supply { + swift_dash_string_free(supply) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift` around lines 191 - 223, Increase the stress level of testMemoryLeakPrevention by replacing the hard-coded 10 iterations with a larger, configurable iteration count (e.g., let iterations = 10_000) and wrap each allocation loop body in an autoreleasepool to force timely deallocation; apply this to the loops that call swift_dash_sdk_get_version/swift_dash_string_free, swift_dash_identity_create/swift_dash_error_free, and swift_dash_token_get_total_supply/swift_dash_string_free so the test exercises many allocations and frees while avoiding spurious retention from the test harness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift`:
- Around line 191-223: Increase the stress level of testMemoryLeakPrevention by
replacing the hard-coded 10 iterations with a larger, configurable iteration
count (e.g., let iterations = 10_000) and wrap each allocation loop body in an
autoreleasepool to force timely deallocation; apply this to the loops that call
swift_dash_sdk_get_version/swift_dash_string_free,
swift_dash_identity_create/swift_dash_error_free, and
swift_dash_token_get_total_supply/swift_dash_string_free so the test exercises
many allocations and frees while avoiding spurious retention from the test
harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 333b247b-a27a-4fc5-95b4-bf353750691f
📒 Files selected for processing (3)
packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.cpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.c
| let publicKey: [UInt8] = Array(repeating: 0x33, count: 33) | ||
|
|
||
Member
There was a problem hiding this comment.
lots of unrelated whitespace changes; please revert these, unless needed to satisfy linter in CI
Collaborator
Author
There was a problem hiding this comment.
Reverted all the whitespace changes — only the meaningful changes remain (added XCTAssertNotNil for alias resolution, removed the tautological else branch). See 74da739.
thepastaclaw
force-pushed
the
test/swift-sdk-remove-scaffold-tests
branch
from
74da739 to
24f9b3e
Compare
- Remove scaffold test files: CrashDebugTests, DebugTests, MinimalAsyncTest - Remove testSimpleAsync from StateTransitionTests (scaffold) - Remove testTransactionBuilderAddOutput and testTransactionBuilderChangeAddress (properties not accessible through public API, tests were tautological) - Replace XCTAssertTrue(true) with proper assertions throughout - Fix SDKMethodTests to XCTFail on unexpected success - Fix SDKTests version check: use semver regex instead of hardcoded '2.0.0' - Add missing swift_dash_string_free calls in SDKTests
- Fix mock swift_dash_sdk_get_version() to return heap-allocated memory via strdup() instead of a string literal, matching the ownership contract of swift_dash_string_free() which calls free() - Replace double-free UB test with XCTSkip explaining why it cannot be safely tested without API changes (pointer-to-pointer free pattern) - Update SemVer regex to accept full spec: optional pre-release segment followed by optional build metadata (e.g. 1.2.3-alpha+001)
Address CodeRabbit nitpick: consolidate the three separate 10-iteration loops into a single 100-iteration loop. This makes the test a proper stress test rather than a smoke test, improving leak detection coverage.
thepastaclaw
force-pushed
the
test/swift-sdk-remove-scaffold-tests
branch
from
24f9b3e to
f993e29
Compare
…ests Keeps only the meaningful changes (XCTAssertNotNil for alias resolution, removal of tautological else branch) and reverts all cosmetic whitespace modifications as requested in review.
thepastaclaw
force-pushed
the
test/swift-sdk-remove-scaffold-tests
branch
from
f993e29 to
36ad5fb
Compare
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.
Issue Being Fixed
Addresses test quality issues identified in the Swift SDK test audit. Retargets the work from the now-deleted
feat/iOSSupportbranch tov3.1-devper ZocoLini's request on PR #3137.What Was Changed
Files Deleted (SW-1: Debug scaffold files)
SwiftExampleApp/SwiftExampleAppTests/CrashDebugTests.swift— debugging artifact with print statements and no assertionsSwiftExampleApp/SwiftExampleAppTests/DebugTests.swift— 6/9 tests were justXCTAssertTrue(true)SwiftExampleApp/SwiftExampleAppTests/MinimalAsyncTest.swift— 4 tests that sleep thenXCTAssertTrue(true)~270 lines of zero-value test code removed.
MemoryManagementTests.swift
XCTAssertTrue(true, "should not crash")assertionsSDKTests.swift
testSDKInitialization()now verifies the SDK returns a valid version pointertestSDKVersion()now frees the version string to fix memory leaktestSDKDestroyNullHandle()IdentityTests.swift & DataContractTests.swift
else { XCTAssertTrue(true) }branchesStateTransitionTests.swift
testSimpleAsync()and tautological assertionSDKMethodTests.swift
XCTAssertTrue(true)from catch block — errors should propagateTransactionTests.swift
throw XCTSkip(...)to dead-code testsSummary
Rebased onto
v3.1-devfrom the deletedfeat/iOSSupportbranch. Supersedes #3137.Summary by CodeRabbit