chore(swift-sdk): reduce swift-sdk test time in CI#3869
Conversation
📝 WalkthroughWalkthroughThis PR optimizes iOS release builds with fat link-time optimization, consolidates test execution scripts to use a new ChangesSwift SDK Build and UI Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 28ce415) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/swift-sdk/build_ios.sh`:
- Line 85: The help text returned by the show_help() function omits the newly
added --target value "tests" even though the script accepts it (see the case
branch that sets BUILD_SIM=true; BUILD_MAC=true for tests); update show_help()
to list "tests" among valid --target options and update any related usage lines
that enumerate targets so CLI users and CI see the new tests option consistent
with the case handling for BUILD_SIM/BUILD_MAC.
🪄 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: d6da8bea-fa00-449a-95b4-53d4465a29d5
📒 Files selected for processing (4)
Cargo.tomlpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/build_ios.shpackages/swift-sdk/run_tests.sh
💤 Files with no reviewable changes (1)
- Cargo.toml
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR introduces a real regression in the published PR artifact: run_tests.sh now invokes build_ios.sh --target tests, which only builds the simulator and macOS slices, yet swift-sdk-build.yml continues to zip the resulting DashSDKFFI.xcframework as the PR artifact and recommends it as a SwiftPM .binaryTarget — consumers targeting physical iOS devices cannot link it. Two minor follow-ups: build_ios.sh show_help() does not advertise the new tests target, and the SwiftExampleAppTests/UITests targets are no longer exercised anywhere in CI.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
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/run_tests.sh`:
- [BLOCKING] packages/swift-sdk/run_tests.sh:7: PR-artifact XCFramework no longer contains the iOS device slice
run_tests.sh now calls `build_ios.sh --target tests`, which only sets BUILD_SIM=true and BUILD_MAC=true (build_ios.sh:85). The downstream `xcodebuild -create-xcframework` step (build_ios.sh:236–240) is driven by IOS_LIB/SIM_LIB/MAC_LIB, so the resulting DashSDKFFI.xcframework contains only `ios-arm64-simulator` and `macos-arm64` — no `ios-arm64` device slice. swift-sdk-build.yml then zips that same xcframework and uploads it as the PR artifact (lines 89–107) and posts a PR comment instructing consumers to wire it up as a SwiftPM `.binaryTarget` (lines 143–158). Any consumer following those instructions and linking the artifact into an app targeting a physical iOS device will fail to link, and device-only FFI build breakages will no longer be caught at PR time (release builds still use `--target all`, so production releases are unaffected, but the PR-level deliverable is silently incomplete). Either build the iOS device slice in CI before the upload step (e.g. invoke `build_ios.sh --target ios --profile dev` after `run_tests.sh`, or split the PR upload step off `--target all`) or update the PR comment template to declare the artifact as simulator+mac only.
- [SUGGESTION] packages/swift-sdk/run_tests.sh:6-9: SwiftExampleAppTests/UITests have zero CI coverage after this change
Removing the `xcodebuild test -scheme SwiftExampleApp` invocation eliminates the only place SwiftExampleAppTests (KeyManagerTests, StateTransitionTests, ValidationTests, CreateIdentityResumableTests, ShieldedSyncGenerationTests, WalletTests/*, etc.) and SwiftExampleAppUITests were exercised. `swift test` only runs the SwiftPM-defined test targets, not Xcode app test bundles. The PR description acknowledges this and promises a follow-up migration PR, but until that lands these tests will silently rot and regressions will not be caught. Land the migration in the same PR, or keep a slimmed `xcodebuild test` invocation (e.g. with `-skip-testing` filters on the slow cases) so coverage is not dropped to zero in the interim.
|
✅ 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: "08697fb7c90de689d57fae0dc5b6e55a5a3faf5193491f637b44d051682ca0df"
)Xcode manual integration:
|
Ci was being slowed down when trying to execute the build and test step of the swift-sdk, to improve the job execution time I changed:
Checklist:
For repository code-owners and collaborators only