Do not continue with fetch if we do not get a valid IID. #4745
Conversation
| _checkCompletionTimeout = 1.0; | ||
|
|
||
| // Always remove the database at the start of testing. | ||
| _DBPath = [RCNTestUtilities remoteConfigPathForTestDatabase]; |
There was a problem hiding this comment.
nit: I would prefer having separate helper methods for the setup operations as it looks more readable to me. Maybe even move some into RCNTestUtilities ?
There was a problem hiding this comment.
will raincheck on this one. Some of the setup is particular to this file and will need to spend some time for a good generic option to pull this into RCNTestUtilities.
| // Always remove the database at the start of testing. | ||
| _DBPath = [RCNTestUtilities remoteConfigPathForTestDatabase]; | ||
| id classMock = OCMClassMock([RCNConfigDBManager class]); | ||
| OCMStub([classMock remoteConfigPathForDatabase]).andReturn(_DBPath); |
There was a problem hiding this comment.
Do we need to stop mocking the class at the end of the test to avoid interference with other tests? If so classMock should be stored to an ivar and taken care in tearDown method.
The same for similar mocks below.
| .andReturn(_userDefaults); | ||
|
|
||
| RCNConfigContent *configContent = [[RCNConfigContent alloc] initWithDBManager:_DBManager]; | ||
| _configInstances = [[NSMutableArray alloc] initWithCapacity:3]; |
There was a problem hiding this comment.
nit: use RCNTestRCNumTotalInstances instead of 3?
| // Test for each RC FIRApp, namespace instance. | ||
| for (int i = 0; i < RCNTestRCNumTotalInstances; i++) { | ||
| // Set the token as nil. | ||
| [self mockInstanceIDMethodForTokenAndIdentity:nil |
There was a problem hiding this comment.
looks like it should be out of the loop.
| XCTAssertNil(error); | ||
| }]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe add a test for a success case?
There was a problem hiding this comment.
This should be addressed in the other tests.
| @@ -1396,7 +1396,9 @@ - (void)testSetFetchTimeoutConfigSetting { | |||
| - (void)testConfigureConfigWithValidInput { | |||
| // Configure the default app with our options and ensure the Remote Config instance is set up | |||
| // properly. | |||
| XCTAssertNoThrow([FIRApp configureWithOptions:[self firstAppOptions]]); | |||
| if (![FIRApp isDefaultAppConfigured]) { | |||
There was a problem hiding this comment.
Why the default app may be configured here? It looks cleaner to me to make sure previous tests clean up the state in tearDown and assert this condition in the tests or setUp, so the check below is performed unconditionally.
There was a problem hiding this comment.
The IID SDK's init needs a default app configured. How do i reset FIRApp state in teardown?
There was a problem hiding this comment.
I think [FIRApp deleteApp:] or probably better [FIRApp resetApps] should do the trick.
|
Since code freeze is tomorrow and RC is a dependent pod, please try to resolve the open issues and merge today. |
|
Still seeing an Xcode 11 build warning: |
|
Hi! im coming here from #4622 |
Fixes #4622