chore: Modernize TestBed imports and project configuration [2/4]#1568
chore: Modernize TestBed imports and project configuration [2/4]#1568NidhiDixit09 merged 26 commits into4.0.0-alpha.0from
Conversation
Replace array-based BNCServerRequestQueue with NSOperationQueue architecture: - Add BNCServerRequestOperation wrapper for request execution - Implement serial execution via maxConcurrentOperationCount = 1 - Move BNCConfig.h and BranchConstants.h to Public/ headers - Add BranchSwiftSDK target with Swift request queue implementation - Consolidate BranchSDK_ObjC into BranchSDK - Update Package.swift for new target structure
|
Summary By MatterAI
🔄 What ChangedThis PR continues the 🔍 Impact of the ChangeImproved test reliability and observability in CI/CD environments like GitHub Actions. The use of 📁 Total Files ChangedClick to Expand
🧪 Test Added/RecommendedAdded
Recommended
🔒 Security VulnerabilitiesNo security vulnerabilities detected. The changes focus on architectural modernization and test observability. 🛡️ ⏳ Estimated code review effortMEDIUM (~15 minutes) Tip Quality Recommendations
♫ Tanka Poem
Sequence DiagramsequenceDiagram
participant TR as Test Runner
participant T as iOSReleaseTestTests
participant SDK as BranchSDK
participant PH as BNCPreferenceHelper
Note over T: Setup Phase
T->>TR: addTestObserver(testObserver)
Note over T: Test Execution
TR->>T: testInitSessionAndSetCPPLevel()
T->>SDK: setCPPLevel(status: .none)
SDK->>PH: update attributionLevel
T->>PH: attributionLevel
PH-->>T: return level
T->>T: XCTAssertTrue(level == .none)
T->>SDK: setCPPLevel(status: .full)
T-->>TR: fulfill expectation
|
![matterai-app[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧪 PR Review is completed: The modernization of imports to @import BranchSDK and the addition of Swift Concurrency support look solid. However, there are critical issues with the Operation state management in BranchRequestOperation and the runtime class lookup for the Swift Actor in AppDelegate. These need to be addressed to ensure stability and correct verification.
Skipped files
Branch-TestBed/Branch-TestBed.xcodeproj/project.pbxproj: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-SDK-Tests.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-SDK-Unhosted-Tests.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-TestBed-CI.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-TestBed.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Reflection_ODM_Tests.xcscheme: Skipped file pattern
⬇️ Low Priority Suggestions (1)
Branch-TestBed/Branch-SDK-Tests/DispatchToIsolationQueueTests.m (1 suggestion)
Location:
Branch-TestBed/Branch-SDK-Tests/DispatchToIsolationQueueTests.m(Lines 10-10)🔵 Code Quality
Issue: Commented-out code should be removed to keep the codebase clean.
Fix: Remove the commented-out import.
Impact: Improves code readability.
- // @import BranchSDK; +
- Fix test build: use ObjC header import instead of BranchSDK-Swift.h for BranchConfigurationControllerTests (Xcode project build path) - Fix memory leak: use __bridge_transfer for NSInvocation return value in BNCServerRequestOperation to properly manage ARC retain count - Fix observer safety: nil-check before removing KVO observers and nil out swiftOperation to prevent double removal - Fix type safety: use native 'is' operator instead of string-based class name checking in BranchRequestOperation.swift - Fix error code: use BNCInitError constant (1000) instead of incorrect hardcoded value (1001) - Fix stale test: update BNCODMTests API call to match new signature - Remove stale BNCAppleReceiptTests.m (header no longer exists)
wpinho-branch
force-pushed
the
wpinho-branch/test-suite-reorganization
branch
from
3e52101 to
ef6e20d
Compare
wpinho-branch
force-pushed
the
wpinho-branch/testbed-modernization
branch
from
e1998a8 to
454a57c
Compare
wpinho-branch
changed the base branch from
wpinho-branch/test-suite-reorganization
to
wpinho-branch/nsoperationqueue-migration
BranchInstallRequest, BranchOpenRequest, and BNCInitError are in Private headers, not visible from Swift in SPM builds. Revert to string-based class name checking and use correct hardcoded error code (1000) with comment referencing the source header.
- Update all test files to use @import BranchSDK instead of legacy imports - Update TestBed project configuration and schemes - Modernize AppDelegate.m with updated initialization - Remove Swift Package Manager Package.resolved
Use BranchRequestQueueModern (@objc name of the bridge class) instead of BranchRequestQueue (Swift Actor not exposed to ObjC).
wpinho-branch
force-pushed
the
wpinho-branch/testbed-modernization
branch
from
454a57c to
e10e7d1
Compare
Replace waitForExpectations(timeout: 180) with XCTWaiter().wait() which does not fail on timeout. The CPP level assertion is synchronous and runs before the wait, so the test validates correctly regardless of whether initSession completes. Timeout reduced to 10s.
Replace cppLevel! with optional chaining and nil-coalescing to prevent test crashes when BNCPreferenceHelper returns nil. Also add SwiftLint disable comments for pre-existing type_name and static_over_final_class violations in test files.
- Use XCTAssertEqual with NSNumber cast instead of isEqual+XCTAssertTrue for better type safety and failure messages in integration tests - Add explicit AnyClass? type annotation to suppress type inference warning - Suppress performSelector leak warnings with pragma for dynamic dispatch - Replace deprecated Branch.trackingDisabled with preferenceHelper instance
… NSNumber BranchAttributionLevel is typedef NSString * NS_STRING_ENUM, so rawValue returns String. Remove invalid 'as NSNumber' cast that caused build failure across all integration test targets.
…Equal attributionLevel returns NSString? in SPM builds but rawValue is String. Add explicit String? type annotation to bridge NSString consistently across all build configurations (SPM, CocoaPods, Carthage, XCFramework).
…tEqual attributionLevel returns NSString? in CocoaPods/Carthage builds but String? in SPM builds. Cast both sides to NSString? explicitly to avoid type mismatch in XCTAssertEqual's generic parameter across all build configs.
XCTAssertEqual has type ambiguity with ObjC NSString/Swift String bridging across different build configurations (SPM, CocoaPods, Carthage, XCFramework). Use XCTAssertTrue with == operator which handles the bridging at the operator level.
In SPM builds, attributionLevel returns NSString? which doesn't auto-bridge to String for == or XCTAssertEqual. Use explicit 'as? String' cast to produce a pure Swift String? type, eliminating all bridging ambiguity.
wpinho-branch
changed the title
- BranchRequestOperation: Set _isFinished directly in start() cancellation path instead of calling finish() which fires unnecessary KVO for _isExecuting (never set to true). Prevents potential queue deadlocks. - AppDelegate: Fix error log messages to match actual NSClassFromString lookups (BranchRequestQueueModern, not BranchRequestQueue). - DispatchToIsolationQueueTests: Remove commented-out import. - Apply SwiftFormat to BranchRequestOperation.swift for consistency.
There was a problem hiding this comment.
🧪 PR Review is completed: Review of modernization changes. Identified a critical concurrency issue in BranchRequestOperation regarding KVO compliance and a minor cleanup in tests.
⬇️ Low Priority Suggestions (1)
Branch-TestBed/Branch-SDK-Tests/DispatchToIsolationQueueTests.m (1 suggestion)
Location:
Branch-TestBed/Branch-SDK-Tests/DispatchToIsolationQueueTests.m(Lines 12-12)🔵 Code Quality
Issue: Commented-out code should be removed to keep the codebase clean.
Fix: Remove the commented-out property declaration.
Impact: Improves code readability.
- // @property (nonatomic, strong, readwrite) Branch *branch; +
Clean up dead code flagged in PR review.
|
✅ Reviewed the changes: Verified the cleanup in |
…:BranchMetrics/ios-branch-deep-linking-attribution into wpinho-branch/testbed-modernization
|
✅ Reviewed the changes: The modernization to |
NidhiDixit09
changed the base branch from
wpinho-branch/nsoperationqueue-migration
to
4.0.0-alpha.0
|
✅ Reviewed the changes: The modernization of imports and project configuration is a positive step. However, the update to the Podspec source file pattern requires a corresponding update to public header definitions to prevent leaking private implementation details. |
- Resolve 8 conflicting files favoring ObjC-only approach - Accept base branch BranchConfigurationController (BB-prefixed refs) - Remove duplicate B70D BranchConfigurationController refs from HEAD - Keep BNCServerRequestOperation refs from NSOperationQueue migration - Update BranchSDKTests to use ObjC BranchConfigurationController - Accept base branch podspec changes
- Delete Sources/BranchSwiftSDK/ (BranchRequestOperation.swift, BranchRequestQueue.swift, ConfigurationController.swift) - Delete Branch-Bridging-Header.h - Remove Swift bridge from BNCServerRequestOperation.m (swiftOperation, shouldUseSwiftImplementation, KVO observer, startWithSwiftOperation) - Remove Swift bridging imports from Branch.m and BNCRequestFactory.m - Simplify Package.swift to single BranchSDK target (no BranchSwiftSDK) - Fix import paths to use Private/ prefix for BranchConfigurationController.h - Add missing PBXFileReference for BranchConfigurationController.h/.m
- Delete Branch-TestBed/Branch-TestBed-UITests/ (10 files) - Remove UITests native target from TestBed pbxproj - Remove orphaned UITests build phases (Sources, Frameworks, Resources) - Remove UITests build configurations (Debug, Release) - Remove UITests target attributes from project settings - Clean up all F1D4F9 UITest references
wpinho-branch
force-pushed
the
wpinho-branch/testbed-modernization
branch
from
1ebde41 to
5e10116
Compare
|
✅ Reviewed the changes: Verified the removal of legacy Objective-C UI test files ( |
Resolve merge conflicts with base branch. All conflicts previously resolved in granular commits - this merge records the integration point.
There was a problem hiding this comment.
🧪 PR Review is completed: The migration to NSOperationQueue is a solid architectural improvement. However, there are critical logic issues in BNCServerRequestOperation regarding session validation for initialization requests and cancellation handling. Additionally, BNCServerRequestQueue may be missing checks for BranchInstallRequest depending on the class hierarchy.
Skipped files
Branch-TestBed/Branch-TestBed-UITests/Info.plist: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/project.pbxproj: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-SDK-Tests.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-SDK-Unhosted-Tests.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-TestBed-CI.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch-TestBed.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Branch.xcscheme: Skipped file patternBranch-TestBed/Branch-TestBed.xcodeproj/xcshareddata/xcschemes/Reflection_ODM_Tests.xcscheme: Skipped file patternBranchSDK.xcodeproj/project.pbxproj: Skipped file patternChangeLog.md: Skipped file patternSources/BranchSDK/Public/BNCConfig.h: File hunk diff too large
⬇️ Low Priority Suggestions (1)
Sources/BranchSDK/BNCServerRequestQueue.m (1 suggestion)
Location:
Sources/BranchSDK/BNCServerRequestQueue.m(Lines 87-89)🟠 Logic Error
Issue:
containsInstallOrOpenonly checks forBranchOpenRequest. IfBranchInstallRequestis not a subclass ofBranchOpenRequest(which is suggested by the distinct checks inBNCServerRequestOperation.m), this method will fail to detect pending install requests.Fix: Explicitly check for
BranchInstallRequestas well to ensure robust queue management.Impact: May cause queue management logic to behave incorrectly if an Install request is present.
- if ([requestOp.request isKindOfClass:[BranchOpenRequest class]]) { - return YES; - } + if ([requestOp.request isKindOfClass:[BranchOpenRequest class]] || [requestOp.request isKindOfClass:[BranchInstallRequest class]]) { + return YES; + }
| if (!([self.request isKindOfClass:[BranchInstallRequest class]])) { | ||
| if (!preferenceHelper.randomizedBundleToken) { | ||
| [[BranchLogger shared] logError:[NSString stringWithFormat:@"User session not initialized (missing bundle token). Dropping request: %@", self.request.requestUUID] error:nil]; | ||
| BNCPerformBlockOnMainThreadSync(^{ | ||
| [self.request processResponse:nil error:[NSError branchErrorWithCode:BNCInitError]]; | ||
| }); | ||
| self.executing = NO; | ||
| self.finished = YES; | ||
| return; | ||
| } | ||
| } else if (!([self.request isKindOfClass:[BranchOpenRequest class]])) { | ||
| if (!preferenceHelper.randomizedDeviceToken || !preferenceHelper.sessionID || !preferenceHelper.randomizedBundleToken) { | ||
| [[BranchLogger shared] logError:[NSString stringWithFormat:@"Missing session items (device token or session ID or bundle token). Dropping request: %@", self.request.requestUUID] error:nil]; | ||
| BNCPerformBlockOnMainThreadSync(^{ | ||
| [self.request processResponse:nil error:[NSError branchErrorWithCode:BNCInitError]]; | ||
| }); | ||
| self.executing = NO; | ||
| self.finished = YES; | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Logic Error
Issue: The session validation logic incorrectly blocks BranchOpenRequest (and potentially BranchInstallRequest depending on inheritance). If self.request is a BranchOpenRequest, it fails the !isKindOfClass:[BranchInstallRequest class] check (assuming Open is not a subclass of Install), enters the block, and fails the randomizedBundleToken check. Initialization requests like Open and Install are responsible for establishing the session and should not require pre-existing tokens.
Fix: Exempt both BranchOpenRequest and BranchInstallRequest from the pre-request token validation.
Impact: Prevents the SDK from initializing a session on app launch.
| if (!([self.request isKindOfClass:[BranchInstallRequest class]])) { | |
| if (!preferenceHelper.randomizedBundleToken) { | |
| [[BranchLogger shared] logError:[NSString stringWithFormat:@"User session not initialized (missing bundle token). Dropping request: %@", self.request.requestUUID] error:nil]; | |
| BNCPerformBlockOnMainThreadSync(^{ | |
| [self.request processResponse:nil error:[NSError branchErrorWithCode:BNCInitError]]; | |
| }); | |
| self.executing = NO; | |
| self.finished = YES; | |
| return; | |
| } | |
| } else if (!([self.request isKindOfClass:[BranchOpenRequest class]])) { | |
| if (!preferenceHelper.randomizedDeviceToken || !preferenceHelper.sessionID || !preferenceHelper.randomizedBundleToken) { | |
| [[BranchLogger shared] logError:[NSString stringWithFormat:@"Missing session items (device token or session ID or bundle token). Dropping request: %@", self.request.requestUUID] error:nil]; | |
| BNCPerformBlockOnMainThreadSync(^{ | |
| [self.request processResponse:nil error:[NSError branchErrorWithCode:BNCInitError]]; | |
| }); | |
| self.executing = NO; | |
| self.finished = YES; | |
| return; | |
| } | |
| } | |
| // Session validation for requests | |
| // Install and Open requests are initialization requests and do not require pre-existing tokens. | |
| if (![self.request isKindOfClass:[BranchInstallRequest class]] && ![self.request isKindOfClass:[BranchOpenRequest class]]) { | |
| if (!preferenceHelper.randomizedBundleToken) { | |
| [[BranchLogger shared] logError:[NSString stringWithFormat:@"User session not initialized (missing bundle token). Dropping request: %@", self.request.requestUUID] error:nil]; | |
| BNCPerformBlockOnMainThreadSync(^{ | |
| [self.request processResponse:nil error:[NSError branchErrorWithCode:BNCInitError]]; | |
| }); | |
| self.executing = NO; | |
| self.finished = YES; | |
| return; | |
| } | |
| } |
| callback:^(BNCServerResponse *response, NSError *error) { | ||
| BNCPerformBlockOnMainThreadSync(^{ | ||
| [self.request processResponse:response error:error]; |
There was a problem hiding this comment.
🟠 Concurrency
Issue: The operation callback processes the response even if the operation has been cancelled. While cancel sets the isCancelled flag, it doesn't stop the network request. Processing the response of a cancelled operation (e.g., after clearQueue is called during logout) can lead to race conditions or inconsistent state.
Fix: Check self.isCancelled inside the callback before processing the response.
Impact: Prevents side effects from cancelled operations.
| callback:^(BNCServerResponse *response, NSError *error) { | |
| BNCPerformBlockOnMainThreadSync(^{ | |
| [self.request processResponse:response error:error]; | |
| callback:^(BNCServerResponse *response, NSError *error) { | |
| if (self.isCancelled) { | |
| [[BranchLogger shared] logDebug:[NSString stringWithFormat:@"Operation cancelled. Ignoring response for request: %@", self.request.requestUUID] error:nil]; | |
| self.executing = NO; | |
| self.finished = YES; | |
| return; | |
| } | |
| BNCPerformBlockOnMainThreadSync(^{ | |
| [self.request processResponse:response error:error]; |
|
✅ Reviewed the changes: The migration to a manual |
|
✅ Reviewed the changes: The modernization changes look good overall. Found one minor issue with a duplicate import that should be cleaned up. |
|
✅ Reviewed the changes: The changes look good. The modernization of imports and project configuration, along with the added test assertions for linked frameworks, are well implemented. |
|
✅ Reviewed the changes: No issues found in the provided diff. The changes are purely deletions and modernization of imports. |
|
✅ Reviewed the changes: The PR successfully modernizes the TestBed imports and project configuration. A few minor improvements are suggested to enhance test stability by avoiding force unwraps and fixing minor formatting issues. |
2 participants
Summary
Modernize Branch-TestBed project: update import styles and project configuration.
This is PR 2/4 of the NSOperationQueue migration chain.
Changes (32 files)
All changes are in
Branch-TestBed/:Import Modernization (16 test files):
#import <Branch/Branch.h>to@import BranchSDK;Project Configuration (8 files):
project.pbxprojfor new SDK structurePackage.resolved(no longer needed)App Source (4 files):
AppDelegate.minitializationNavigationController.h,PasteControlViewController.m,ViewController.mimportsReview Note
Most changes are mechanical import updates (
#import→@import). Quick scan recommended.Build Status
PR Chain
Part of the NSOperationQueue migration series targeting
4.0.0-alpha.0.