fix: add example test folder inside sample and fix tests #165
Conversation
islameldesoky95
force-pushed
the
chore/MSDK-2613-fix-ios-sample-tests
branch
2 times, most recently
from
a7662a7 to
a179e23
Compare
WalkthroughAdd macOS-14 / Xcode 16.1 iOS CI using Ruby 3.2 and Bundler-driven CocoaPods; bump sample Gemfile; add a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as macOS-14 Runner
participant Ruby as Ruby / Bundler
participant Pods as CocoaPods (bundle exec)
participant Xcode as xcodebuild
note right of Runner #F8FDF3: CI steps for iOS test job
GH->>Runner: start iOS job
Runner->>Runner: select Xcode 16.1, set env flags
Runner->>Ruby: setup ruby/setup-ruby@v1 (3.2) & bundler cache
Runner->>Ruby: bundle install --path vendor/bundle
Runner->>Pods: pod deintegrate / pod cache clean
Runner->>Pods: bundle exec pod install --repo-update
Runner->>Runner: remove DerivedData
Runner->>Xcode: xcodebuild test (./sample/ios) dest=latest,iPhone 16 Pro
Xcode-->>Runner: raw test output
Runner->>Runner: pipe output to xcpretty (preserve exit code)
Runner-->>GH: job result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (8)
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 |
islameldesoky95
force-pushed
the
chore/MSDK-2613-fix-ios-sample-tests
branch
3 times, most recently
from
3e3fd4f to
39bfbd4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (25)
sample/ios/sampleTests/UsercentricsAnalyticsEventTypeTests.swift (1)
6-11: Consider expanding test coverage.The test correctly verifies that integer value
1maps toacceptAllFirstLayer. However, consider the following improvements:
- Add tests for all enum cases to ensure complete mapping coverage
- Document what the integer values represent (likely for JS ↔ native serialization)
- Test edge cases like invalid input values
Example expansion:
func testInitializeAcceptAllFirstLayer() { XCTAssertEqual(UsercentricsAnalyticsEventType.acceptAllFirstLayer, UsercentricsAnalyticsEventType.initialize(from: 1)) } func testInitializeOtherCases() { // Add tests for other enum cases // XCTAssertEqual(UsercentricsAnalyticsEventType.someOtherCase, UsercentricsAnalyticsEventType.initialize(from: 2)) } func testInitializeInvalidValue() { // Test behavior with invalid input }sample/ios/sampleTests/UsercentricsUserInteractionDictTest.swift (1)
7-15: LGTM! Consider renaming for clarity.The test logic is correct and comprehensively validates all four enum-to-int mappings against the implementation.
The class name
UsercentricsUserInteractionDictTestis slightly misleading since it's testing enum integer mappings rather than dictionary operations. Consider renaming to something likeUsercentricsUserInteractionMappingTestorUsercentricsUserInteractionIntTestfor better clarity.sample/ios/Podfile (1)
39-51: Excellent defensive programming and best practice use of$(inherited).The refactored post_install hook demonstrates solid defensive coding:
- Safe navigation and nil checks prevent crashes if pod structure is unexpected
- Skipping test targets (line 43) correctly isolates them from C++ configuration
- Using
$(inherited)preserves parent configurations while enforcing C++20 standardMinor refinement: Line 43 could use
end_with?("Tests")instead ofinclude?("Tests")for more precise matching, though the current approach is acceptable given CocoaPods naming conventions.sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift (1)
69-80: Consider addressing minor inconsistencies.Two minor inconsistencies observed:
Line 71:
AdTechProviderDecision.mock()usesconsent: true, while all other mock methods usefalsefor consent/legitimateInterestConsent. If this difference is intentional to reflect different semantics, consider adding a comment explaining why.Line 77: The
consentfield is not cast withas Any, unlike similar boolean fields in othertoDictionary()methods (lines 33, 34, 48, 63, 64). While this works sinceconsentappears to be non-optional, maintaining consistency would improve code clarity.Apply this diff for consistency:
func toDictionary() -> NSDictionary { return [ "id": self.id as Any, - "consent": self.consent + "consent": self.consent as Any ] }sample/ios/sampleTests/TCFDecisionUILayerTests.swift (1)
8-11: Method name inconsistency.The test method is named
testToIntMethodbut it's actually testing theinitialize(from:)method (converting from Int to enum). Consider renaming totestInitializeFromIntortestFromIntMethodfor clarity.Apply this diff:
- func testToIntMethod() { + func testInitializeFromInt() { XCTAssertEqual(.firstLayer, TCFDecisionUILayer.initialize(from: 0)) XCTAssertEqual(.secondLayer, TCFDecisionUILayer.initialize(from: 1)) }sample/ios/sampleTests/UsercentricsConsentTypeTests.swift (1)
8-11: Method name inconsistency.The test method is named
testToIntMethodbut it's testinginitialize(from:)(converting from Int to enum). Consider renaming totestInitializeFromIntortestFromIntMethodfor consistency and clarity.Apply this diff:
- func testToIntMethod() { + func testInitializeFromInt() { XCTAssertEqual(.explicit_, UsercentricsConsentType.initialize(from: 0)) XCTAssertEqual(.implicit, UsercentricsConsentType.initialize(from: 1)) }sample/ios/sampleTests/UserDecisionDictTests.swift (1)
8-15: Consider using XCTUnwrap for safer assertions.The test force-unwraps the optional
decisionafter asserting it's not nil. While this is safe, usingXCTUnwrapis more idiomatic and provides better error messages if the unwrap fails.Apply this diff:
func testFromDictWithCorrectValues() { let dict: NSDictionary = ["serviceId": "abc", "consent": false] let decision = UserDecision(from: dict) - XCTAssertNotNil(decision) - XCTAssertEqual("abc", decision!.serviceId) - XCTAssertEqual(false, decision!.consent) + let unwrappedDecision = try XCTUnwrap(decision, "UserDecision should not be nil") + XCTAssertEqual("abc", unwrappedDecision.serviceId) + XCTAssertEqual(false, unwrappedDecision.consent) }sample/ios/sampleTests/Mock/UsercentricsOptions+Mock.swift (1)
17-30: Simplify optional handling in asDict.Line 18 uses a ternary operator followed by a force unwrap, which is confusing. The force unwrap is technically safe but the pattern can be simplified for better readability.
Apply this diff:
static func asDict(mockObject: UsercentricsOptions? = nil) -> NSDictionary { - let expectedOptions = (mockObject == nil ? self.mock() : mockObject)! + let expectedOptions = mockObject ?? self.mock() return [ "settingsId": expectedOptions.settingsId, "ruleSetId": expectedOptions.ruleSetId, "loggerLevel": expectedOptions.loggerLevel.ordinal, "timeoutMillis": expectedOptions.timeoutMillis, "version": expectedOptions.version, "defaultLanguage": expectedOptions.defaultLanguage, "consentMediation": expectedOptions.consentMediation, "initTimeoutMillis": expectedOptions.initTimeoutMillis ] }sample/Gemfile (1)
7-10: Optional: Consider alphabetizing gems.RuboCop suggests alphabetizing gems within sections for consistency. While not critical, this improves readability and maintainability.
Apply this diff to alphabetize:
-gem 'cocoapods', '~> 1.16.2' -gem 'xcodeproj', '~> 1.27.0' gem 'activesupport', '>= 7.0' +gem 'cocoapods', '~> 1.16.2' gem 'concurrent-ruby' +gem 'xcodeproj', '~> 1.27.0'sample/ios/sampleTests/FirstLayerOptionsDictTests.swift (2)
38-48: Address TODO comment.The TODO on line 46 indicates that ButtonSettings properties need to be exposed for testing. This limits test coverage for button configuration.
Would you like me to open an issue to track exposing ButtonSettings properties for test verification?
57-102: Optional: Add XCTFail messages for clarity.Adding descriptive messages to XCTFail calls (lines 66, 79, 93) would improve test failure diagnostics.
Example for line 66:
default: - XCTFail() + XCTFail("Expected .row layout, got \(buttonLayout!)").github/workflows/ci.yml (2)
77-83: Use Bundler for all CocoaPods invocations; avoid nuking pod cache unless debuggingEnsure consistent gem versions and faster CI. Only clean cache when needed.
- pod deintegrate - pod cache clean --all - bundle exec pod install --repo-update + bundle exec pod deintegrate + # bundle exec pod cache clean --all # Uncomment only when troubleshooting + bundle exec pod install --repo-update
84-86: Don’t wipe global DerivedData; prefer a job-scoped derived data pathWiping ~/Library/... is unnecessary on ephemeral runners and slows builds. Point xcodebuild to a per-run path instead.
Example change in the test step:
- xcodebuild \ + xcodebuild \ + -derivedDataPath .build/DerivedData \ -workspace sample.xcworkspace \Then drop the DerivedData cleanup step.
- - name: Clean Xcode Build Artifacts - run: | - rm -rf ~/Library/Developer/Xcode/DerivedData/* + # Removed: global DerivedData cleanup not needed on GitHub-hosted runnersAlso consider upgrading actions/checkout and actions/setup-node to v4 for Node runtime compatibility in 2025.
sample/ios/sampleTests/RNUsercentricsModuleTests.swift (6)
55-76: Replace force casts with XCTUnwrap/optional casts; add failure messageRepeated “as!” casts risk crashes and violate SwiftLint. Prefer XCTUnwrap or safe casts, and include a message in XCTFail.
Example within this block:
- else { - XCTFail() - return - } + else { + XCTFail("Invalid isReady payload shape") + return + } @@ - XCTAssertEqual("settingsId", geolocationRulesetMap["activeSettingsId"] as! String) + XCTAssertEqual(try! XCTUnwrap(geolocationRulesetMap["activeSettingsId"] as? String), "settingsId") @@ - XCTAssertEqual("1.2.3", consent["version"] as! String) + XCTAssertEqual(try! XCTUnwrap(consent["version"] as? String), "1.2.3")Apply the same pattern across similar assertions in this file to remove “as!” and add explicit failure messages. Use XCTAssertTrue/False for booleans (e.g., XCTAssertFalse(shouldCollectConsent)). Based on SwiftLint hints.
485-490: Replace unused closure parameter with underscore to satisfy SwiftLintThe result parameter isn’t used.
- module.showFirstLayer(serializedSettings) { result in + module.showFirstLayer(serializedSettings) { _ in
494-499: Replace unused closure parameter with underscore (empty banner settings case)- module.showFirstLayer(emptyDictionary) { result in + module.showFirstLayer(emptyDictionary) { _ in
505-510: Replace unused closure parameter with underscore (second layer with settings)- module.showSecondLayer(serializedSettings) { result in + module.showSecondLayer(serializedSettings) { _ in
515-520: Replace unused closure parameter with underscore (second layer empty settings)- module.showSecondLayer(emptyDictionary) { result in + module.showSecondLayer(emptyDictionary) { _ in
593-599: Test name casing nitFor consistency with XCTest naming, consider renaming to testClearUserSession.
- func testclearUserSession() { + func testClearUserSession() {sample/ios/sampleTests/Fake/FakeUsercentricsManager.swift (6)
33-41: Ensure mutually exclusive callbacks (don’t call both success and failure)Current fake can invoke both closures if both fields are set. Mirror real API by choosing one.
func isReady(onSuccess: @escaping ((UsercentricsReadyStatus) -> Void), onFailure: @escaping ((Error) -> Void)) { - if let isReadySuccessCompletion = isReadySuccessCompletion { - onSuccess(isReadySuccessCompletion) - } - - if let isReadyErrorCompletion = isReadyErrorCompletion { - onFailure(isReadyErrorCompletion) - } + if let success = isReadySuccessCompletion { + onSuccess(success) + } else if let error = isReadyErrorCompletion { + onFailure(error) + } }Apply the same pattern to restoreUserSession and clearUserSession below.
48-58: Mirror exclusivity for restoreUserSession callbacks- if let restoreUserSessionSuccess = restoreUserSessionSuccess { - onSuccess(restoreUserSessionSuccess) - } - - if let restoreUserSessionError = restoreUserSessionError { - onFailure(restoreUserSessionError) - } + if let success = restoreUserSessionSuccess { + onSuccess(success) + } else if let error = restoreUserSessionError { + onFailure(error) + }
195-204: Mirror exclusivity for clearUserSession callbacks- if let clearUserSessionSuccess = clearUserSessionSuccess { - onSuccess(clearUserSessionSuccess) - } - - if let clearUserSessionError = clearUserSessionError { - onError(clearUserSessionError) - } + if let success = clearUserSessionSuccess { + onSuccess(success) + } else if let error = clearUserSessionError { + onError(error) + }
68-70: Return optional for getABTestingVariant (avoid force unwrap, allow nil path testing)This method is optional in the real API; force unwrapping prevents testing nil.
- func getABTestingVariant() -> String? { - return getABTestingVariantValue! - } + func getABTestingVariant() -> String? { + return getABTestingVariantValue + }
108-110: Remove redundant optional initialization to nil- var changeLanguageValue: String? = nil - var changeLanguageError: Error? = nil + var changeLanguageValue: String? + var changeLanguageError: Error?
190-193: Prefer shorthand array syntax- var trackCalls: Array<UsercentricsAnalyticsEventType> = [] + var trackCalls: [UsercentricsAnalyticsEventType] = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonsample/ios/Podfile.lockis excluded by!**/*.locksample/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.github/workflows/ci.yml(3 hunks)sample/Gemfile(1 hunks)sample/ios/Podfile(2 hunks)sample/ios/sample.xcodeproj/project.pbxproj(14 hunks)sample/ios/sampleTests/BannerSettingsDictTests.swift(1 hunks)sample/ios/sampleTests/Fake/FakeDispatchQueue.swift(1 hunks)sample/ios/sampleTests/Fake/FakePresentationViewController.swift(1 hunks)sample/ios/sampleTests/Fake/FakeUsercentricsManager.swift(1 hunks)sample/ios/sampleTests/FirstLayerOptionsDictTests.swift(1 hunks)sample/ios/sampleTests/Mock/CMPData+Mock.swift(1 hunks)sample/ios/sampleTests/Mock/TCFData+Mock.swift(1 hunks)sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift(1 hunks)sample/ios/sampleTests/Mock/UserDecisions+Mock.swift(1 hunks)sample/ios/sampleTests/Mock/UsercentricsIsReady+Mock.swift(1 hunks)sample/ios/sampleTests/Mock/UsercentricsOptions+Mock.swift(1 hunks)sample/ios/sampleTests/RNUsercentricsModuleTests.swift(1 hunks)sample/ios/sampleTests/ReadyStatusDictTests.swift(1 hunks)sample/ios/sampleTests/TCFDecisionUILayerTests.swift(1 hunks)sample/ios/sampleTests/UserDecisionDictTests.swift(1 hunks)sample/ios/sampleTests/UsercentricsAnalyticsEventTypeTests.swift(1 hunks)sample/ios/sampleTests/UsercentricsConsentTypeTests.swift(1 hunks)sample/ios/sampleTests/UsercentricsOptionsDictTests.swift(1 hunks)sample/ios/sampleTests/UsercentricsUserInteractionDictTest.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
sample/ios/sampleTests/Mock/TCFData+Mock.swift (2)
src/models/TCFData.tsx (2)
TCFVendorRestriction(279-291)VendorUrl(262-277)example/ios/exampleTests/Mock/TCFData+Mock.swift (1)
mock(75-102)
sample/ios/sampleTests/Mock/UserDecisions+Mock.swift (2)
sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift (1)
mockToDict(12-20)src/models/UserDecision.tsx (1)
UserDecision(1-10)
sample/ios/sampleTests/UsercentricsUserInteractionDictTest.swift (3)
sample/ios/sampleTests/TCFDecisionUILayerTests.swift (1)
testToIntMethod(8-11)sample/ios/sampleTests/UsercentricsConsentTypeTests.swift (1)
testToIntMethod(8-11)ios/Extensions/UsercentricsUserInteraction+String.swift (1)
toInt(5-17)
sample/ios/sampleTests/UsercentricsAnalyticsEventTypeTests.swift (1)
src/Usercentrics.tsx (1)
Usercentrics(23-145)
sample/ios/sampleTests/FirstLayerOptionsDictTests.swift (1)
src/models/FirstLayerSettings.tsx (11)
ButtonSettings(135-162)ButtonLayout(106-127)row(116-118)column(120-122)grid(124-126)MessageSettings(82-104)TitleSettings(67-80)HeaderImageSettings(34-65)extended(54-56)hidden(62-64)logo(58-60)
sample/ios/sampleTests/Mock/UsercentricsIsReady+Mock.swift (4)
src/models/UsercentricsReadyStatus.tsx (1)
UsercentricsReadyStatus(4-20)src/models/GeolocationRuleset.tsx (1)
GeolocationRuleset(1-10)src/models/UsercentricsLocation.tsx (1)
UsercentricsLocation(1-32)src/models/UsercentricsServiceConsent.tsx (2)
UsercentricsServiceConsent(3-24)UsercentricsConsentHistoryEntry(26-37)
sample/ios/sampleTests/Mock/UsercentricsOptions+Mock.swift (1)
src/models/UsercentricsOptions.tsx (1)
UsercentricsOptions(3-45)
sample/ios/sampleTests/Mock/CMPData+Mock.swift (1)
src/models/UsercentricsLocation.tsx (1)
UsercentricsLocation(1-32)
sample/ios/sampleTests/UsercentricsOptionsDictTests.swift (2)
src/Usercentrics.tsx (1)
Usercentrics(23-145)src/models/UsercentricsOptions.tsx (1)
UsercentricsOptions(3-45)
sample/ios/sampleTests/RNUsercentricsModuleTests.swift (9)
src/Usercentrics.tsx (1)
Usercentrics(23-145)ios/RNUsercentricsModule.swift (1)
moduleName(23-25)sample/ios/sampleTests/Mock/CMPData+Mock.swift (16)
mock(5-11)mock(16-51)mock(56-104)mock(109-112)mock(117-217)mock(222-231)mock(236-238)mock(243-252)mock(258-276)mock(281-343)mock(348-356)mock(361-367)mock(372-398)mock(403-407)mock(412-415)mock(420-432)sample/ios/sampleTests/Fake/FakeUsercentricsManager.swift (25)
configure(43-46)isReady(33-41)restoreUserSession(48-58)getControllerId(64-66)getABTestingVariant(68-70)getConsents(73-75)getUserSessionData(83-85)getUSPData(88-90)changeLanguage(110-117)acceptAll(130-133)acceptAllForTCF(122-126)denyAll(146-149)denyAllForTCF(138-142)saveDecisionsForTCF(152-154)saveDecisions(157-159)saveOptOutForCCPA(162-164)getTCFData(93-95)setCMPId(167-169)setABTestingVariant(172-174)showFirstLayer(178-182)showSecondLayer(185-188)track(191-193)getCMPData(78-80)getAdditionalConsentModeData(98-100)clearUserSession(195-204)sample/ios/sampleTests/Mock/UsercentricsOptions+Mock.swift (1)
asDict(17-29)src/models/CCPAData.tsx (1)
CCPAData(1-32)sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift (5)
mockToDict(12-20)toDictionary(30-36)toDictionary(45-50)toDictionary(60-66)toDictionary(74-79)sample/ios/sampleTests/Mock/UserDecisions+Mock.swift (1)
mockToDict(5-11)src/models/AdditionalConsentModeData.tsx (2)
AdditionalConsentModeData(1-13)AdTechProvider(15-33)
sample/ios/sampleTests/TCFDecisionUILayerTests.swift (3)
src/Usercentrics.tsx (1)
Usercentrics(23-145)sample/ios/sampleTests/UsercentricsConsentTypeTests.swift (1)
testToIntMethod(8-11)sample/ios/sampleTests/UsercentricsUserInteractionDictTest.swift (1)
testToIntMethod(9-14)
sample/ios/sampleTests/BannerSettingsDictTests.swift (2)
sample/ios/sampleTests/UserDecisionDictTests.swift (1)
testFromDictWithCorrectValues(8-15)src/models/BannerSettings.tsx (1)
BannerSettings(104-117)
sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift (2)
src/models/TCFUserDecisions.tsx (1)
TCFUserDecisions(1-24)sample/ios/sampleTests/Mock/UserDecisions+Mock.swift (1)
mockToDict(5-11)
sample/ios/sampleTests/UsercentricsConsentTypeTests.swift (2)
sample/ios/sampleTests/TCFDecisionUILayerTests.swift (1)
testToIntMethod(8-11)sample/ios/sampleTests/UsercentricsUserInteractionDictTest.swift (1)
testToIntMethod(9-14)
sample/ios/sampleTests/ReadyStatusDictTests.swift (5)
src/models/UsercentricsServiceConsent.tsx (2)
UsercentricsConsentHistoryEntry(26-37)UsercentricsServiceConsent(3-24)src/models/UsercentricsReadyStatus.tsx (1)
UsercentricsReadyStatus(4-20)src/models/GeolocationRuleset.tsx (1)
GeolocationRuleset(1-10)src/models/UsercentricsLocation.tsx (1)
UsercentricsLocation(1-32)sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift (4)
toDictionary(30-36)toDictionary(45-50)toDictionary(60-66)toDictionary(74-79)
sample/ios/sampleTests/UserDecisionDictTests.swift (2)
sample/ios/sampleTests/BannerSettingsDictTests.swift (1)
testFromDictWithCorrectValues(8-21)src/models/UserDecision.tsx (1)
UserDecision(1-10)
sample/ios/sampleTests/Fake/FakeUsercentricsManager.swift (2)
src/Usercentrics.tsx (1)
Usercentrics(23-145)src/models/UsercentricsConsentUserResponse.tsx (1)
UsercentricsConsentUserResponse(3-14)
🪛 RuboCop (1.81.1)
sample/Gemfile
[convention] 9-9: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem activesupport should appear before xcodeproj.
(Bundler/OrderedGems)
[convention] 15-15: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem benchmark should appear before logger.
(Bundler/OrderedGems)
🪛 SwiftLint (0.57.0)
sample/ios/sampleTests/FirstLayerOptionsDictTests.swift
[Warning] 46-46: TODOs should be resolved (Expose ButtonSettings properti...)
(todo)
[Warning] 66-66: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 79-79: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 93-93: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 205-205: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 217-217: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 229-229: An XCTFail call should include a description of the assertion
(xctfail_message)
sample/ios/sampleTests/Mock/CMPData+Mock.swift
[Error] 110-110: Force casts should be avoided
(force_cast)
sample/ios/sampleTests/RNUsercentricsModuleTests.swift
[Error] 62-62: Force casts should be avoided
(force_cast)
[Error] 63-63: Force casts should be avoided
(force_cast)
[Error] 65-65: Force casts should be avoided
(force_cast)
[Error] 66-66: Force casts should be avoided
(force_cast)
[Error] 68-68: Force casts should be avoided
(force_cast)
[Error] 69-69: Force casts should be avoided
(force_cast)
[Error] 70-70: Force casts should be avoided
(force_cast)
[Error] 71-71: Force casts should be avoided
(force_cast)
[Error] 72-72: Force casts should be avoided
(force_cast)
[Error] 103-103: Force casts should be avoided
(force_cast)
[Error] 104-104: Force casts should be avoided
(force_cast)
[Error] 105-105: Force casts should be avoided
(force_cast)
[Error] 106-106: Force casts should be avoided
(force_cast)
[Error] 107-107: Force casts should be avoided
(force_cast)
[Error] 154-154: Force casts should be avoided
(force_cast)
[Error] 155-155: Force casts should be avoided
(force_cast)
[Error] 156-156: Force casts should be avoided
(force_cast)
[Error] 157-157: Force casts should be avoided
(force_cast)
[Error] 158-158: Force casts should be avoided
(force_cast)
[Error] 190-190: Force casts should be avoided
(force_cast)
[Error] 191-191: Force casts should be avoided
(force_cast)
[Error] 192-192: Force casts should be avoided
(force_cast)
[Error] 193-193: Force casts should be avoided
(force_cast)
[Error] 194-194: Force casts should be avoided
(force_cast)
[Error] 230-230: Force casts should be avoided
(force_cast)
[Error] 231-231: Force casts should be avoided
(force_cast)
[Error] 232-232: Force casts should be avoided
(force_cast)
[Error] 233-233: Force casts should be avoided
(force_cast)
[Error] 234-234: Force casts should be avoided
(force_cast)
[Error] 263-263: Force casts should be avoided
(force_cast)
[Error] 264-264: Force casts should be avoided
(force_cast)
[Error] 265-265: Force casts should be avoided
(force_cast)
[Error] 266-266: Force casts should be avoided
(force_cast)
[Error] 267-267: Force casts should be avoided
(force_cast)
[Error] 299-299: Force casts should be avoided
(force_cast)
[Error] 300-300: Force casts should be avoided
(force_cast)
[Error] 301-301: Force casts should be avoided
(force_cast)
[Error] 302-302: Force casts should be avoided
(force_cast)
[Error] 303-303: Force casts should be avoided
(force_cast)
[Error] 332-332: Force casts should be avoided
(force_cast)
[Error] 333-333: Force casts should be avoided
(force_cast)
[Error] 334-334: Force casts should be avoided
(force_cast)
[Error] 335-335: Force casts should be avoided
(force_cast)
[Error] 336-336: Force casts should be avoided
(force_cast)
[Error] 369-369: Force casts should be avoided
(force_cast)
[Error] 370-370: Force casts should be avoided
(force_cast)
[Error] 371-371: Force casts should be avoided
(force_cast)
[Error] 372-372: Force casts should be avoided
(force_cast)
[Error] 373-373: Force casts should be avoided
(force_cast)
[Error] 390-390: Force casts should be avoided
(force_cast)
[Error] 391-391: Force casts should be avoided
(force_cast)
[Error] 392-392: Force casts should be avoided
(force_cast)
[Error] 393-393: Force casts should be avoided
(force_cast)
[Error] 394-394: Force casts should be avoided
(force_cast)
[Error] 412-412: Force casts should be avoided
(force_cast)
[Error] 413-413: Force casts should be avoided
(force_cast)
[Error] 414-414: Force casts should be avoided
(force_cast)
[Error] 415-415: Force casts should be avoided
(force_cast)
[Error] 416-416: Force casts should be avoided
(force_cast)
[Error] 579-579: Force casts should be avoided
(force_cast)
[Error] 584-584: Force casts should be avoided
(force_cast)
[Error] 585-585: Force casts should be avoided
(force_cast)
[Error] 586-586: Force casts should be avoided
(force_cast)
[Error] 587-587: Force casts should be avoided
(force_cast)
[Error] 607-607: Force casts should be avoided
(force_cast)
[Error] 608-608: Force casts should be avoided
(force_cast)
[Error] 609-609: Force casts should be avoided
(force_cast)
[Error] 610-610: Force casts should be avoided
(force_cast)
[Error] 611-611: Force casts should be avoided
(force_cast)
[Warning] 210-210: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 485-485: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 494-494: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 505-505: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 515-515: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 56-56: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 98-98: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 150-150: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 170-170: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 186-186: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 226-226: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 259-259: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 295-295: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 328-328: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 365-365: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 386-386: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 408-408: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 428-428: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 534-534: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 575-575: An XCTFail call should include a description of the assertion
(xctfail_message)
[Warning] 602-602: An XCTFail call should include a description of the assertion
(xctfail_message)
sample/ios/sampleTests/ReadyStatusDictTests.swift
[Error] 25-25: Force casts should be avoided
(force_cast)
[Error] 28-28: Force casts should be avoided
(force_cast)
[Error] 29-29: Force casts should be avoided
(force_cast)
[Error] 30-30: Force casts should be avoided
(force_cast)
[Error] 32-32: Force casts should be avoided
(force_cast)
[Error] 33-33: Force casts should be avoided
(force_cast)
[Error] 34-34: Force casts should be avoided
(force_cast)
[Error] 48-48: Force casts should be avoided
(force_cast)
[Error] 49-49: Force casts should be avoided
(force_cast)
[Error] 50-50: Force casts should be avoided
(force_cast)
[Error] 51-51: Force casts should be avoided
(force_cast)
[Error] 52-52: Force casts should be avoided
(force_cast)
[Error] 53-53: Force casts should be avoided
(force_cast)
[Error] 65-65: Force casts should be avoided
(force_cast)
[Error] 71-71: Force casts should be avoided
(force_cast)
[Error] 72-72: Force casts should be avoided
(force_cast)
[Error] 73-73: Force casts should be avoided
(force_cast)
sample/ios/sampleTests/Fake/FakeUsercentricsManager.swift
[Warning] 108-108: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 109-109: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 190-190: Shorthand syntactic sugar should be used, i.e. [Int] instead of Array
(syntactic_sugar)
🔇 Additional comments (24)
sample/ios/sampleTests/UsercentricsAnalyticsEventTypeTests.swift (1)
1-4: LGTM!The imports and testable declarations are correct and provide the necessary access to internal members for testing.
sample/ios/sampleTests/UsercentricsUserInteractionDictTest.swift (1)
1-6: LGTM!The import statements are correct and include all necessary dependencies for testing the
UsercentricsUserInteractionenum.sample/ios/Podfile (1)
27-30: LGTM! Standard test target pattern.Inheriting only search paths is a recommended approach for isolating test configuration from the main target while still allowing tests to locate necessary headers.
sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift (4)
1-3: LGTM!The imports are appropriate for this test mock file.
23-51: LGTM!The mock factories and dictionary conversion methods are well-structured and follow consistent patterns for Kotlin Multiplatform interop.
53-67: LGTM!The implementation is consistent with the other decision type extensions.
15-18: Verify TCFUserDecisions property optionality in mock
In sample/ios/sampleTests/Mock/TCFUserDecisions+Mock.swift (lines 15–18),purposes,specialFeatures, andvendorsuse optional chaining butadTechProvidersdoes not. Confirm these properties’ optionality in the iOS SDK and align the mock accordingly.sample/ios/sampleTests/Mock/UserDecisions+Mock.swift (1)
4-12: LGTM!The mock factory method correctly creates a UserDecision and converts it to an NSDictionary format that matches the TypeScript model structure. This provides consistent test data for dictionary serialization tests.
sample/ios/sampleTests/UserDecisionDictTests.swift (1)
17-22: LGTM!The negative test case correctly verifies that initialization returns
nilwhen the dictionary has incorrect keys. This ensures the parser properly validates input.sample/ios/sampleTests/Mock/UsercentricsIsReady+Mock.swift (3)
20-31: LGTM!The
UsercentricsServiceConsent.mock()factory method provides comprehensive test data with all required fields properly initialized. The nested mock call for history entries is a good pattern.
33-37: LGTM!The
UsercentricsConsentHistoryEntry.mock()factory method correctly initializes all required fields with appropriate test values.
11-18: Cannot locate UsercentricsLocation definition
Please verify the initializer signature includesisInEU,isInUS, andisInCaliforniaparameters.sample/ios/sampleTests/Mock/TCFData+Mock.swift (2)
4-15: LGTM!The
TCFData.mock()factory method properly constructs test data with nested mock calls for all collection properties. The structure matches the expected TCF data model.
74-103: LGTM!The
TCFVendor.mock()factory method is comprehensive and correctly handles:
- All required and optional fields
- Nested structures (restrictions, URLs, data categories)
- Proper use of
KotlinBooleanwrappers- Nullable fields set to
nilwhere appropriateThis provides robust test data for TCF vendor testing.
sample/ios/sampleTests/Mock/UsercentricsOptions+Mock.swift (1)
4-16: LGTM!The
mock()factory method provides sensible defaults for allUsercentricsOptionsparameters, making it easy to create test data with minimal setup. The parameter defaults are appropriate for testing.sample/ios/sampleTests/Mock/CMPData+Mock.swift (1)
1-543: Well-structured mock factory.The comprehensive set of mock factory methods provides good test coverage for Usercentrics domain types. The consistent
.mock()pattern and nested mock construction enable realistic test scenarios.sample/ios/sampleTests/Fake/FakeDispatchQueue.swift (1)
1-9: LGTM!The synchronous fake dispatch queue is well-suited for deterministic test execution without real asynchrony.
sample/ios/sampleTests/Fake/FakePresentationViewController.swift (1)
1-4: LGTM!The minimal fake view controller serves its purpose for testing presentation behavior.
sample/ios/sampleTests/FirstLayerOptionsDictTests.swift (1)
125-179: Thorough font fallback testing.The comprehensive test coverage for TitleSettings font fallback behavior (lines 125-179) validates multiple scenarios including fallback fonts, system fonts, and font size handling.
sample/ios/sampleTests/UsercentricsOptionsDictTests.swift (1)
8-87: Well-structured dictionary mapping tests.The tests comprehensively validate UsercentricsOptions initialization from dictionaries, including enum serialization for loggerLevel and networkMode.
sample/ios/sampleTests/ReadyStatusDictTests.swift (1)
8-75: Comprehensive serialization tests.The tests thoroughly validate dictionary serialization for ReadyStatus, UsercentricsServiceConsent, and ConsentHistoryEntry, including handling of explicit vs implicit consent types.
Note: Force casts in test assertions (flagged by SwiftLint) are acceptable here since test failures will reveal type mismatches.
sample/Gemfile (1)
4-17: Verify Ruby 3.2.0 compatibilityGems cocoppods 1.16.2 and xcodeproj 1.27.0 declare no explicit
required_ruby_version; please run full tests under Ruby 3.2.0 and review their changelogs for any compatibility notes.sample/ios/sampleTests/RNUsercentricsModuleTests.swift (1)
200-206: Clarify expectation for changeLanguage success valueJS API returns Promise; native resolve is typically nil/NSNull. Asserting non-nil may be misleading.
If the resolve is void/NSNull, prefer:
- module.changeLanguage("pt") { result in - XCTAssert(result != nil) + module.changeLanguage("pt") { result in + XCTAssertNotNil(result) // or XCTAssertNil(result) depending on bridge contractPlease confirm the intended return (void vs value) for changeLanguage and adjust the assertion accordingly.
.github/workflows/ci.yml (1)
89-99: Avoid hardcoding Simulator OS/device; use OS=latest to prevent runner failuresSandbox lacks Xcode tools—please run on the macOS CI runner:
xcodebuild -showsdks xcrun simctl list runtimes xcrun simctl list devices availableThen update the workflow:
- -destination 'platform=iOS Simulator,OS=17.5,name=iPhone 15' \ + -destination 'platform=iOS Simulator,OS=latest,name=iPhone 15' \
| classes = { | ||
| }; | ||
| objectVersion = 54; | ||
| objectVersion= 77; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify objectVersion compatibility.
The objectVersion is set to 77, which is specific to Xcode 16+. Ensure that all team members and CI environments use Xcode 16 or later, as older versions may not be able to open this project.
Run the following script to check if the workflow and documentation reference the correct Xcode version:
🏁 Script executed:
#!/bin/bash
# Check for Xcode version requirements in CI configuration and documentation
rg -n --type yaml "xcode.*version|Xcode.*[0-9]+" .github/workflows/
rg -n "Xcode.*[0-9]+" README.md docs/ 2>/dev/null || echo "No docs/ directory found"Length of output: 128
Update Xcode version requirement in documentation
- CI uses Xcode 16.1 while README still says “Xcode 15.0+”; objectVersion 77 needs Xcode 16+, so bump the README’s minimum Xcode version to 16+.
🤖 Prompt for AI Agents
In sample/ios/sample.xcodeproj/project.pbxproj around line 6 the objectVersion
is 77 which requires Xcode 16+, but README still states "Xcode 15.0+"; update
the README (and any other docs or CI hints) to require "Xcode 16.0+" (or "Xcode
16+") and ensure any platform/tooling notes reflect this minimum version so
documentation matches the project file requirement.
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEBUG_INFORMATION_FORMAT = dwarf; | ||
| DEVELOPMENT_TEAM = 4GB94SV7D4; |
There was a problem hiding this comment.
Remove hardcoded development team.
The DEVELOPMENT_TEAM is hardcoded to 4GB94SV7D4. This will cause build failures for developers who are not part of this team. Consider removing this setting or moving it to a user-specific .xcconfig file that is not committed to version control.
Apply this approach:
- Remove the
DEVELOPMENT_TEAMsetting from the project file - Create a
Config.xcconfigfile (add to.gitignore) - Document in README how developers should configure their own team ID
Or, if code signing is not required for tests, set:
CODE_SIGN_STYLE = Manual;
🤖 Prompt for AI Agents
In sample/ios/sample.xcodeproj/project.pbxproj around line 622, the file
contains a hardcoded DEVELOPMENT_TEAM = 4GB94SV7D4; which causes build failures
for developers not in that team; remove this DEVELOPMENT_TEAM entry from the
project file, create a Config.xcconfig (add it to .gitignore) that developers
can populate with their own DEVELOPMENT_TEAM (or leave unset), update the
project to import Config.xcconfig, and add README instructions showing how to
set a personal team ID in Config.xcconfig; alternatively, if code signing isn’t
required for tests, set CODE_SIGN_STYLE = Manual in the project or xcconfig
instead of hardcoding the team.
| COPY_PHASE_STRIP = NO; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; | ||
| DEVELOPMENT_TEAM = 4GB94SV7D4; |
There was a problem hiding this comment.
Remove hardcoded development team in Release configuration.
Same issue as Debug configuration - the DEVELOPMENT_TEAM = 4GB94SV7D4 should be removed or moved to a user-specific configuration file.
🤖 Prompt for AI Agents
In sample/ios/sample.xcodeproj/project.pbxproj around line 661, the Release
configuration contains a hardcoded DEVELOPMENT_TEAM = 4GB94SV7D4 which must be
removed or moved out of the committed project file; delete the DEVELOPMENT_TEAM
entry from the Release block (or replace it with a reference to a user-specific
xcconfig variable), add the per-developer xcconfig file to .gitignore, and
document/ensure developers set DEVELOPMENT_TEAM in their local xcconfig or Xcode
workspace settings instead of committing the team ID.
| func testFromDictWithCorrectValues() { | ||
| let dict: NSDictionary = [ | ||
| "generalStyleSettings": [ | ||
| "font": ["fontName": "Avenir-Heavy", "fontSize": 16.0], | ||
| "logo": ["logoName": "logo"] | ||
| ] | ||
| ] | ||
| let settings = BannerSettings(from: dict) | ||
|
|
||
| XCTAssertNotNil(settings) | ||
| // XCTAssertNotNil(settings?.generalStyleSettings?.logo) | ||
| // XCTAssertEqual("Avenir-Heavy", settings?.font?.fontName) | ||
| // XCTAssertEqual(16.0, settings?.font?.pointSize) | ||
| } |
There was a problem hiding this comment.
Incomplete test assertions.
Lines 18-20 have commented-out assertions that should verify the font and logo properties. This reduces test coverage and may miss regressions.
Uncomment and fix the assertions, or clarify why they're disabled. Would you like me to help investigate why these assertions were commented out?
| extension LegalBasisLocalization { | ||
|
|
||
| static func mock() -> LegalBasisLocalization { | ||
| let value = (NSClassFromString("UsercentricsLegalBasisLocalization")?.alloc() as! LegalBasisLocalization) | ||
| return value.doCopy(labels: 123, labelsAria: .mock(), data: ["123": "123"]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Force cast risk in test mock.
The force cast on line 110 could crash if NSClassFromString("UsercentricsLegalBasisLocalization") returns nil or an incompatible type. While this is test code and failures are isolated, consider using optional binding for more defensive code.
Apply this diff to use safer optional binding:
static func mock() -> LegalBasisLocalization {
- let value = (NSClassFromString("UsercentricsLegalBasisLocalization")?.alloc() as! LegalBasisLocalization)
- return value.doCopy(labels: 123, labelsAria: .mock(), data: ["123": "123"])
+ guard let cls = NSClassFromString("UsercentricsLegalBasisLocalization"),
+ let value = cls.alloc() as? LegalBasisLocalization else {
+ fatalError("Failed to create LegalBasisLocalization mock")
+ }
+ return value.doCopy(labels: 123, labelsAria: .mock(), data: ["123": "123"])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extension LegalBasisLocalization { | |
| static func mock() -> LegalBasisLocalization { | |
| let value = (NSClassFromString("UsercentricsLegalBasisLocalization")?.alloc() as! LegalBasisLocalization) | |
| return value.doCopy(labels: 123, labelsAria: .mock(), data: ["123": "123"]) | |
| } | |
| } | |
| extension LegalBasisLocalization { | |
| static func mock() -> LegalBasisLocalization { | |
| guard let cls = NSClassFromString("UsercentricsLegalBasisLocalization"), | |
| let value = cls.alloc() as? LegalBasisLocalization else { | |
| fatalError("Failed to create LegalBasisLocalization mock") | |
| } | |
| return value.doCopy(labels: 123, labelsAria: .mock(), data: ["123": "123"]) | |
| } | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 110-110: Force casts should be avoided
(force_cast)
🤖 Prompt for AI Agents
In sample/ios/sampleTests/Mock/CMPData+Mock.swift around lines 107 to 113,
replace the forced casting of NSClassFromString(...).alloc() as!
LegalBasisLocalization with safe optional binding: attempt to resolve the class
and cast it to LegalBasisLocalization (or LegalBasisLocalization.Type),
guard/if-let the result, instantiate via alloc/init if available, and if the
class is missing or incompatible return a safe fallback (e.g., create and return
a plain LegalBasisLocalization mock instance) or fail the test with a clear
error; this removes the force-cast crash risk while preserving the mock
behavior.
39bfbd4 to
622d2d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
sample/ios/sample.xcodeproj/project.pbxproj (2)
622-622: Remove hardcoded development team (duplicate concern).The
DEVELOPMENT_TEAM = 4GB94SV7D4will cause build failures for developers not on this team. This was previously flagged and remains unresolved.
661-661: Remove hardcoded development team in Release (duplicate concern).Same issue as Debug configuration—the hardcoded
DEVELOPMENT_TEAM = 4GB94SV7D4must be removed or moved to a user-specific configuration file.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
77-82: Aggressive pod cleanup may slow CI unnecessarily.Running
pod deintegrateandpod cache clean --allon every CI run can significantly increase build times. Consider whether this aggressive cleanup is necessary for every build, or if it should only run when pod-related issues are suspected.If full cleanup isn't required every time, consider this conditional approach:
- - name: Install pods (via bundler) - working-directory: ./sample/ios - run: | - pod deintegrate - pod cache clean --all - bundle exec pod install --repo-update + - name: Install pods (via bundler) + working-directory: ./sample/ios + run: | + bundle exec pod install --repo-updateAdd a separate manual workflow_dispatch job for full cleanup when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(3 hunks)sample/ios/sample.xcodeproj/project.pbxproj(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-ios
- GitHub Check: test-rn
🔇 Additional comments (7)
.github/workflows/ci.yml (4)
40-45: LGTM! Runner and Xcode versions align with project requirements.The upgrade to macos-14 and Xcode 16.1 is consistent with the
objectVersion = 77requirement in the project file, which mandates Xcode 16+.
55-59: LGTM! Ruby setup with bundler caching improves CI efficiency.The Ruby 3.2 setup with bundler caching will speed up subsequent CI runs by caching gem installations.
99-114: LGTM! Crash log display aids debugging test failures.The conditional crash log display step will help diagnose test failures by surfacing crash reports when tests fail.
88-98: Hardcoded simulator configuration may cause brittleness. The test step pinsOS=17.5,name=iPhone 15, which may be unavailable on the macos-14 runner; consider omitting the OS version to select the latest available simulator or documenting required simulator versions. To verify availability in CI, run:xcrun simctl list devices available | grep -E 'iPhone 15'and ensure iOS 17.5 is installed.
sample/ios/sample.xcodeproj/project.pbxproj (3)
6-6: LGTM! objectVersion 77 aligns with CI Xcode 16.1.The objectVersion is consistent with the CI workflow's Xcode 16.1 requirement. Team members must use Xcode 16+ to open this project.
161-182: LGTM! sampleTests target structure is well-formed.The test target configuration follows standard Xcode conventions with proper dependency on the main sample target, appropriate build phases, and file system synchronization.
607-681: Test target build settings are appropriately configured.The Debug and Release configurations for sampleTests include proper test-specific settings (
BUNDLE_LOADER,TEST_HOST,GENERATE_INFOPLIST_FILE) and modern Swift/Clang options. The configurations correctly reference Pods-sampleTests xcconfig files.
islameldesoky95
force-pushed
the
chore/MSDK-2613-fix-ios-sample-tests
branch
from
622d2d9 to
0022f4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(3 hunks)sample/ios/sample.xcodeproj/project.pbxproj(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-android
- GitHub Check: test-ios
🔇 Additional comments (4)
sample/ios/sample.xcodeproj/project.pbxproj (1)
161-182: LGTM! Test target properly configured.The new
sampleTeststarget is correctly wired with:
- Proper target dependencies via
PBXContainerItemProxyandPBXTargetDependency- CocoaPods integration via
[CP] Check Pods Manifest.lockbuild phase- Build phases (Sources, Frameworks, Resources) correctly configured
TEST_HOSTandBUNDLE_LOADERpointing to the main appAlso applies to: 231-237, 354-360
.github/workflows/ci.yml (3)
40-45: LGTM! Runner and Xcode versions align with project requirements.The upgrade to
macos-14andxcode-version: '16.1'is consistent with the project'sobjectVersion = 77, which requires Xcode 16+.
55-82: LGTM! Robust dependency management.The Ruby/Bundler setup with CocoaPods workflow improvements are excellent:
- Bundler caching speeds up CI runs
pod deintegrateandpod cache clean --allensure a clean statebundle exec pod install --repo-updateuses the locked gem versionsThis reduces dependency-related flakiness.
84-86: LGTM! Clean build environment.Clearing
DerivedDataprevents stale build artifacts from causing flaky test failures.
islameldesoky95
force-pushed
the
chore/MSDK-2613-fix-ios-sample-tests
branch
from
0022f4a to
5772374
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
sample/ios/sample.xcodeproj/project.pbxproj (2)
607-644: Test target Debug configuration looks good, but DEVELOPMENT_TEAM issue applies.The Debug configuration for
sampleTestsis properly set up with:
- Correct
BUNDLE_LOADERandTEST_HOSTpointing to the sample app- Appropriate Swift and compilation settings
- Modern Swift features enabled
However, the hardcoded
DEVELOPMENT_TEAM = 4GB94SV7D4on line 622 is the same issue flagged in past comments for lines 622 and 661 - this should be removed or moved to a user-specific configuration.
645-681: Test target Release configuration looks good, but DEVELOPMENT_TEAM issue applies.The Release configuration mirrors the Debug setup appropriately with optimization settings. However, line 661 contains the same hardcoded
DEVELOPMENT_TEAM = 4GB94SV7D4issue already flagged in past comments.
🧹 Nitpick comments (1)
sample/ios/sample.xcodeproj/project.pbxproj (1)
515-515: Remove trailing spaces in OTHER_LDFLAGS.Both Debug and Release configurations have trailing spaces in
OTHER_LDFLAGS = "$(inherited) ";. While harmless, removing them improves consistency.Apply this diff to remove the trailing spaces:
- OTHER_LDFLAGS = "$(inherited) "; + OTHER_LDFLAGS = "$(inherited)";Also applies to: 599-599
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(3 hunks)sample/ios/sample.xcodeproj/project.pbxproj(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-ios
- GitHub Check: test-android
🔇 Additional comments (6)
sample/ios/sample.xcodeproj/project.pbxproj (6)
11-14: LGTM: Framework references are correctly configured.The build file entries properly reference both
Pods_sample.frameworkandPods_sampleTests.framework, ensuring each target has access to its respective CocoaPods dependencies.
18-26: LGTM: Test target dependency is correctly configured.The
PBXContainerItemProxyproperly establishes the dependency relationship between the test target and the main sample target, ensuring tests can access the app under test.
161-182: LGTM: Test target is properly configured.The
sampleTestsnative target includes all necessary build phases:
- CocoaPods manifest check
- Sources compilation
- Framework linking
- Resources handling
The target dependency ensures the main app is built before tests run.
189-199: LGTM: Project attributes are correctly set.The project attributes properly configure the test target with appropriate Xcode version tracking (
LastSwiftUpdateCheck = 2600) and test target metadata (TestTargetID).
703-711: LGTM: Test target configuration list is correctly set up.The
XCConfigurationListforsampleTestsproperly references both Debug and Release configurations with Release as the default.
45-51: Approve PBXFileSystemSynchronizedRootGroup usage
Verifiedsample/ios/sampleTestsexists with test files; configuration is correct.
| pod deintegrate | ||
| pod cache clean --all | ||
| bundle exec pod install --repo-update |
There was a problem hiding this comment.
Run CocoaPods commands through Bundler.
Lines [80-82] call pod deintegrate and pod cache clean without bundle exec, so they hit the runner’s global cocoapods instead of the version (and plugins like cocoapods-deintegrate) you just installed via Bundler. On runners where those commands aren’t preinstalled, the step will fail (pod: command not found or Unknown command 'deintegrate'). Please execute every pod command through Bundler for consistency and to guarantee the expected plugin set.
Apply this diff:
- pod deintegrate
- pod cache clean --all
- bundle exec pod install --repo-update
+ bundle exec pod deintegrate
+ bundle exec pod cache clean --all
+ bundle exec pod install --repo-update📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pod deintegrate | |
| pod cache clean --all | |
| bundle exec pod install --repo-update | |
| bundle exec pod deintegrate | |
| bundle exec pod cache clean --all | |
| bundle exec pod install --repo-update |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 80 to 82: the workflow runs `pod
deintegrate` and `pod cache clean --all` directly which can invoke the runner’s
global CocoaPods (or fail if not installed) instead of the Bundler-installed
version and plugins; update the workflow so every `pod` invocation in this step
is prefixed with `bundle exec` (e.g., `bundle exec pod deintegrate`, `bundle
exec pod cache clean --all`, `bundle exec pod install --repo-update`) to ensure
the correct CocoaPods binary and plugins from the Gemfile are used.
c89a4d7 to
1b7d47d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
77-83: Run all CocoaPods commands through Bundler; add strict shell options
Withoutbundle exec,pod deintegrateandpod cache cleanmay hit the global binary or miss plugins, causing flakiness. Also addset -euo pipefail.Apply this diff:
- - name: Install pods (via bundler) + - name: Install pods (via bundler) working-directory: ./sample/ios run: | - pod deintegrate - pod cache clean --all - bundle exec pod install --repo-update + set -euo pipefail + bundle exec pod deintegrate + bundle exec pod cache clean --all + bundle exec pod install --repo-update
🧹 Nitpick comments (2)
sample/ios/sample.xcodeproj/project.pbxproj (1)
515-515: Consider removing trailing spaces.
OTHER_LDFLAGShas trailing spaces on both Debug (line 515) and Release (line 599) configurations. While harmless, removing them would improve consistency.Apply this pattern to clean up both occurrences:
- OTHER_LDFLAGS = "$(inherited) "; + OTHER_LDFLAGS = "$(inherited)";Also applies to: 599-599
.github/workflows/ci.yml (1)
84-86: DerivedData cleanup – OK; tiny simplification
Command is fine. Optionally drop the wildcard.- rm -rf ~/Library/Developer/Xcode/DerivedData/* + rm -rf ~/Library/Developer/Xcode/DerivedData
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(3 hunks)sample/ios/sample.xcodeproj/project.pbxproj(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-ios
- GitHub Check: test-android
🔇 Additional comments (8)
sample/ios/sample.xcodeproj/project.pbxproj (4)
161-182: LGTM! Test target structure is correct.The
sampleTeststarget is properly configured with all necessary build phases, dependencies, and the modernfileSystemSynchronizedGroupsfeature for managing test files in Xcode 16.
257-278: LGTM! CocoaPods integration for test target.The
[CP] Check Pods Manifest.lockphase is properly configured for the test target with the correct output path.
607-644: LGTM! Test target build settings are appropriate.The Debug and Release configurations for
sampleTestsare properly set up with:
- Correct
BUNDLE_LOADERandTEST_HOSTlinking to the main app- Appropriate Swift 5.0 and C++20 language standards
- Proper inheritance from CocoaPods xcconfig files
- Standard test bundle settings
Also applies to: 645-681
623-623: Manual Verification Required: CocoaPods Script SandboxingRunning
xcodebuildisn’t supported in this CI environment. Please manually build thesampleTeststarget on macOS withENABLE_USER_SCRIPT_SANDBOXING = YESand confirm that the “[CP] Check Pods Manifest.lock” (and other CocoaPods script) phases complete without sandbox errors..github/workflows/ci.yml (4)
40-40: macOS 14 runner for Xcode 16.1 – LGTM
Matches the intended Xcode toolchain.
45-45: Pinning Xcode 16.1 – LGTM
Deterministic toolchain selection.
55-60: Ruby 3.2 + bundler-cache – LGTM
Solid setup for pod/xcpretty via Gemfile.
71-76: Bundle install under ./sample – LGTM
Local vendor/bundle keeps gems scoped to sample.
islameldesoky95
force-pushed
the
chore/MSDK-2613-fix-ios-sample-tests
branch
2 times, most recently
from
f0924cc to
f3d8021
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
87-89: Run every CocoaPods command via Bundler.
pod deintegrate/pod cache clean --allare still executed withoutbundle exec, so they bypass the Gemfile-managed CocoaPods/plugin versions and can fail if the runner lacks those binaries. Preface them withbundle execjust likepod install.- pod deintegrate - pod cache clean --all + bundle exec pod deintegrate + bundle exec pod cache clean --all bundle exec pod install --repo-update
103-103: Don’t pin the simulator OS to 18.2.macOS-14 runners with Xcode 16.1 don’t guarantee the iOS 18.2 runtime. If it’s missing,
xcodebuildaborts before tests run. Drop the OS qualifier (let Xcode pick an installed runtime) or pin to a runtime you’ve confirmed exists on the runner.- -destination 'platform=iOS Simulator,OS=18.2,name=iPhone 16 Pro' \ + -destination 'platform=iOS Simulator,name=iPhone 16 Pro' \
🧹 Nitpick comments (1)
sample/ios/sample.xcodeproj/project.pbxproj (1)
515-515: Consider removing trailing spaces in OTHER_LDFLAGS.The
OTHER_LDFLAGSsetting has trailing spaces ("$(inherited) "). While harmless, removing them improves consistency.Apply this diff to clean up the trailing spaces:
- OTHER_LDFLAGS = "$(inherited) "; + OTHER_LDFLAGS = "$(inherited)";Apply the same change at both line 515 (Debug) and line 599 (Release).
Also applies to: 599-599
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(3 hunks)sample/ios/sample.xcodeproj/project.pbxproj(14 hunks)
🔇 Additional comments (7)
sample/ios/sample.xcodeproj/project.pbxproj (7)
9-43: LGTM! Test target file references properly configured.The file references, build files, and container proxy for the test target are correctly structured. The CocoaPods integration references (xcconfig files and frameworks) follow standard conventions.
45-70: LGTM! Modern file management and framework linking.The use of
PBXFileSystemSynchronizedRootGroup(Xcode 16 feature) for the test directory enables automatic file reference management. Framework build phases correctly link the respective Pods frameworks for both targets.
72-183: LGTM! Test target properly integrated into project structure.The test target is comprehensively configured with:
- Correct placement in project groups (Frameworks, Products, main group)
- All necessary build phases (Check Pods Manifest, Sources, Frameworks, Resources)
- Proper dependency linking to the main
sampletarget via PBXTargetDependency- Product type correctly set to
com.apple.product-type.bundle.unit-test
185-343: LGTM! Build phases and target attributes correctly configured.The project attributes and build phases are properly set up:
- Target attributes correctly reference Xcode 26.0.1 (16.0.1) and establish test target relationship
- Resource and source build phases appropriately configured (empty files array is correct with fileSystemSynchronizedGroups)
- CocoaPods shell script phases follow standard patterns for both targets
363-369: LGTM! Test target dependency correctly wired.The
PBXTargetDependencyproperly links the test target to the main app target through the container proxy, ensuring the app builds before tests run.
607-681: LGTM! Test target build settings comprehensive and appropriate.The test target build configurations include:
- Correct
TEST_HOSTpointing to the sample appBUNDLE_LOADERproperly set for test executionENABLE_USER_SCRIPT_SANDBOXING = YESfor security (compatible with modern CocoaPods)- Appropriate Swift and deployment target settings matching the main app
- Standard test bundle identifier and product name
Note: The
DEVELOPMENT_TEAMsetting on lines 622 and 661 was already flagged in previous review comments.
703-711: LGTM! Configuration list properly structured.The
XCConfigurationListfor the test target correctly references both Debug and Release configurations with appropriate defaults.
islameldesoky95
force-pushed
the
chore/MSDK-2613-fix-ios-sample-tests
branch
from
f3d8021 to
0aa9901
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
sample/ios/sample.xcodeproj/project.pbxproj (3)
6-6: objectVersion 77 compatibility already flagged.This issue was raised in previous reviews. Ensure the team addresses the Xcode 16+ requirement and updates documentation accordingly.
622-622: Hardcoded development team still present.This issue was flagged in previous reviews but remains unresolved. The hardcoded
DEVELOPMENT_TEAM = 4GB94SV7D4will cause build failures for developers not on this team.
661-661: Hardcoded development team in Release configuration.Same as Debug configuration—the hardcoded
DEVELOPMENT_TEAMshould be removed or moved to a user-specific configuration..github/workflows/ci.yml (2)
89-91: Run CocoaPods commands through BundlerThese
podcalls bypass the Bundler-managed CocoaPods/plugins, so they can fail (missingpodordeintegrateplugin) or run an unexpected version on the runner.Apply this diff to route every invocation through Bundler:
- pod deintegrate - pod cache clean --all + bundle exec pod deintegrate + bundle exec pod cache clean --all bundle exec pod install --repo-update
105-105: Don’t pin the simulator OS to 18.2
OS=18.2isn’t guaranteed on macOS‑14/Xcode 16.1 runners; if that runtime is missing the tests fail before starting. Let Xcode pick the installed runtime (or verify availability).Use:
- -destination 'platform=iOS Simulator,OS=18.2,name=iPhone 16 Pro' \ + -destination 'platform=iOS Simulator,name=iPhone 16 Pro' \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(3 hunks)sample/ios/sample.xcodeproj/project.pbxproj(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-android
- GitHub Check: test-ios
🔇 Additional comments (3)
sample/ios/sample.xcodeproj/project.pbxproj (3)
163-184: LGTM: Test target properly configured.The
sampleTeststarget is correctly set up with:
- Proper dependency on the main
sampletarget viaPBXContainerItemProxy- CocoaPods integration with manifest lock check
- File system synchronized groups (Xcode 16 feature)
- Standard build phases for unit tests
298-319: LGTM: CocoaPods integration for test target.The
sampleTeststarget correctly includes a manifest lock check phase. This ensures the test target's dependencies stay in sync with the Podfile.lock.
357-363: LGTM: Target dependency correctly configured.The
PBXTargetDependencyensures the mainsampleapp is built before running tests, which is the correct dependency order.
| set -o pipefail && \ | ||
| xcodebuild \ | ||
| -workspace sample.xcworkspace \ | ||
| -scheme "sample" \ | ||
| -sdk iphonesimulator \ | ||
| -destination 'platform=iOS Simulator,OS=18.2,name=iPhone 16 Pro' \ | ||
| -only-testing:sampleTests \ | ||
| test | bundle exec xcpretty && exit ${PIPESTATUS[0]} |
There was a problem hiding this comment.
Fix the broken pipefail/xcodebuild command
The space after the trailing \ causes xcodebuild to run without the test action, and then test runs as its own command—this step will fail every time. Make the backslash the last character and split the pipefail handling so the pipeline executes as intended.
Apply this diff:
- set -o pipefail && \
- xcodebuild \
+ set -o pipefail
+ xcodebuild \
-workspace sample.xcworkspace \
-scheme "sample" \
-sdk iphonesimulator \
-destination 'platform=iOS Simulator,OS=18.2,name=iPhone 16 Pro' \
- -only-testing:sampleTests \
- test | bundle exec xcpretty && exit ${PIPESTATUS[0]}
+ -only-testing:sampleTests \
+ test | bundle exec xcpretty
+ exit ${PIPESTATUS[0]}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set -o pipefail && \ | |
| xcodebuild \ | |
| -workspace sample.xcworkspace \ | |
| -scheme "sample" \ | |
| -sdk iphonesimulator \ | |
| -destination 'platform=iOS Simulator,OS=18.2,name=iPhone 16 Pro' \ | |
| -only-testing:sampleTests \ | |
| test | bundle exec xcpretty && exit ${PIPESTATUS[0]} | |
| set -o pipefail | |
| xcodebuild \ | |
| -workspace sample.xcworkspace \ | |
| -scheme "sample" \ | |
| -sdk iphonesimulator \ | |
| -destination 'platform=iOS Simulator,OS=18.2,name=iPhone 16 Pro' \ | |
| -only-testing:sampleTests \ | |
| test | bundle exec xcpretty | |
| exit ${PIPESTATUS[0]} |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 100 to 107: the heredoc shows the trailing
backslash on the xcodebuild line has a space after it which breaks the line
continuation so `test` is executed as a separate command; remove the trailing
space so each backslash is the last character on its line (or put the entire
xcodebuild invocation on one line), and move/set `set -o pipefail` so it applies
to the whole shell (e.g., run `set -o pipefail` on its own line before the
xcodebuild command) so the pipeline `xcodebuild ... test | bundle exec xcpretty
&& exit ${PIPESTATUS[0]}` executes as intended.
0aa9901 to
032e7f5
Compare
islameldesoky95
force-pushed
the
chore/MSDK-2613-fix-ios-sample-tests
branch
from
032e7f5 to
8981fb7
Compare
Summary by CodeRabbit
New Features
Tests
Chores
Refactor