Working drop of Segmentation SDK along with test app and unit tests. #4574
Conversation
There was a problem hiding this comment.
Generally looks good to me. A few nits and one issue with managing plist files for integration tests.
Please get an approval from at least one of @maksymmalyhin or @ryanwilson
FirebaseSegmentation/Tests/Sample/SecondApp-GoogleService-Info.plist
Outdated
Show resolved
Hide resolved
|
Ping. Ok to merge? This does not go into master yet. |
I'll be able to take a look tomorrow at this, sorry for the delay. |
|
@dmandar Sorry didn't have chance to do a detailed review. I'm fine with merging it if it is a blocker for you and I can review it later if it is OK for you. |
| @@ -70,7 +71,10 @@ - (instancetype)initWithApp:(FIRApp *)app { | |||
| self = [super init]; | |||
| if (self) { | |||
| _app = app; | |||
| _segmentationInstance = nil; | |||
| if (!_segmentationInstance) { | |||
| _segmentationInstance = [[FIRSegmentation alloc] initWithAppName:app.name | |||
There was a problem hiding this comment.
Can you describe a situation where _segmentationInstance would be non-nil when initWithApp: is called? I'm not sure I can think of a case when that's true.
There was a problem hiding this comment.
Not typical. Unless initWithApp is called twice tomorrow from elsewhere.
| @@ -28,6 +28,17 @@ NS_ASSUME_NONNULL_BEGIN | |||
| NS_SWIFT_NAME(Segmentation) | |||
| @interface FIRSegmentation : NSObject | |||
|
|
|||
| /// Firebase Remote Config service fetch error. | |||
| typedef NS_ENUM(NSInteger, FIRSegmentationErrorCode) { | |||
There was a problem hiding this comment.
This has not gone through API review - please add it to the API doc or create a new one.
There was a problem hiding this comment.
Done. PTAL in the API review (commented)
| @@ -0,0 +1,223 @@ | |||
| // Copyright 2019 Google | |||
There was a problem hiding this comment.
FYI: Haven't reviewed this file yet
| @@ -0,0 +1,100 @@ | |||
| // Copyright 2019 Google | |||
There was a problem hiding this comment.
FYI: Haven't reviewed this file yet
| @@ -0,0 +1,419 @@ | |||
| // Copyright 2019 Google | |||
There was a problem hiding this comment.
FYI: Haven't reviewed this file yet
There was a problem hiding this comment.
I added a couple suggestions for tests but didn't do a full review deferring it to @ryanwilson
| } | ||
| - (void)tearDown { | ||
| // Put teardown code here. This method is called after the invocation of each test method in the | ||
| // class. |
There was a problem hiding this comment.
Due to XCTests implementation details it is highly recommended to tear down all mocks and assign to nil all instance variable in this method to avoid memory leaks an interference between the test methods which can lead to flakes.
| associateCustomInstallationIdentiferNamed:@"my-custom-id" | ||
| firebaseApp:@"my-firebase-app-id" | ||
| completion:^(BOOL success, NSDictionary *result) { | ||
| XCTAssertTrue(success, |
There was a problem hiding this comment.
I assume the tests will be extended to test result content, ect. in the following PRs
|
|
||
| - (void)tearDown { | ||
| // Put teardown code here. This method is called after the invocation of each test method in the | ||
| // class. |
There was a problem hiding this comment.
The same as for SEGContentManagerTests - tear down mocks
No description provided.