diff --git a/Crashlytics/Crashlytics/Models/FIRCLSSettings.m b/Crashlytics/Crashlytics/Models/FIRCLSSettings.m index 758772e5b8d..d12ce3de010 100644 --- a/Crashlytics/Crashlytics/Models/FIRCLSSettings.m +++ b/Crashlytics/Crashlytics/Models/FIRCLSSettings.m @@ -40,6 +40,9 @@ @interface FIRCLSSettings () @property(nonatomic) BOOL isCacheKeyExpired; +// Test-only property to make certain operations synchronous for testing +@property (nonatomic, assign) BOOL testExecutionSynchronous; + @end @implementation FIRCLSSettings @@ -189,16 +192,39 @@ - (NSDictionary *)loadCacheKey { } - (void)deleteCachedSettings { - __weak FIRCLSSettings *weakSelf = self; - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0), ^{ - __strong FIRCLSSettings *strongSelf = weakSelf; - if ([strongSelf.fileManager fileExistsAtPath:strongSelf.fileManager.settingsFilePath]) { - [strongSelf.fileManager removeItemAtPath:strongSelf.fileManager.settingsFilePath]; + void (^deleteBlock)(void) = ^{ + if ([self.fileManager fileExistsAtPath:self.fileManager.settingsFilePath]) { + [self.fileManager removeItemAtPath:self.fileManager.settingsFilePath]; } - if ([strongSelf.fileManager fileExistsAtPath:strongSelf.fileManager.settingsCacheKeyPath]) { - [strongSelf.fileManager removeItemAtPath:strongSelf.fileManager.settingsCacheKeyPath]; + if ([self.fileManager fileExistsAtPath:self.fileManager.settingsCacheKeyPath]) { + [self.fileManager removeItemAtPath:self.fileManager.settingsCacheKeyPath]; } - }); + }; + + if (self.testExecutionSynchronous) { + deleteBlock(); + } else { + // Preserving original weakSelf/strongSelf dance for the async case, though self should be fine. + __weak FIRCLSSettings *weakSelf = self; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0), ^{ + __strong FIRCLSSettings *strongSelf = weakSelf; + if (!strongSelf) { + return; + } + // Call the block using strongSelf to maintain original capture semantics if any were intended. + // However, deleteBlock as defined above captures `self` directly. + // For safety and to minimize deviation for the non-test path, let's redefine the block slightly for async + void (^asyncDeleteBlock)(void) = ^{ + if ([strongSelf.fileManager fileExistsAtPath:strongSelf.fileManager.settingsFilePath]) { + [strongSelf.fileManager removeItemAtPath:strongSelf.fileManager.settingsFilePath]; + } + if ([strongSelf.fileManager fileExistsAtPath:strongSelf.fileManager.settingsCacheKeyPath]) { + [strongSelf.fileManager removeItemAtPath:strongSelf.fileManager.settingsCacheKeyPath]; + } + }; + asyncDeleteBlock(); + }); + } @synchronized(self) { self.isCacheKeyExpired = YES; diff --git a/Crashlytics/UnitTests/FIRCLSExistingReportManagerTests.m b/Crashlytics/UnitTests/FIRCLSExistingReportManagerTests.m index ce073a8914b..ca9c1e1d6c8 100644 --- a/Crashlytics/UnitTests/FIRCLSExistingReportManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSExistingReportManagerTests.m @@ -22,6 +22,17 @@ #import "Crashlytics/UnitTests/Mocks/FIRCLSMockReportUploader.h" #import "Crashlytics/UnitTests/Mocks/FIRCLSTempMockFileManager.h" +// Test-only category to allow setting the root path +@interface FIRCLSFileManager (TestingERM) // Existing Report Manager +- (void)test_setRootPath:(NSString *)newRootPath; +@end + +@implementation FIRCLSFileManager (TestingERM) +- (void)test_setRootPath:(NSString *)newRootPath { + [self setValue:newRootPath forKey:@"_rootPath"]; +} +@end + #define METADATA_FORMAT \ (@"{\"identity\":{\"generator\":\"Crashlytics iOS " \ @"SDK/" \ @@ -43,6 +54,10 @@ @interface FIRCLSExistingReportManagerTests : XCTestCase @property(nonatomic, strong) FIRCLSExistingReportManager *existingReportManager; @property(nonatomic, strong) FIRCLSManagerData *managerData; +// Path for test-specific files +NSString *_testSpecificRootPathERM; // Existing Report Manager tests +} + @end @implementation FIRCLSExistingReportManagerTests @@ -52,12 +67,22 @@ - (void)setUp { FIRCLSContextBaseInit(); + // Generate a unique path for this test instance + _testSpecificRootPathERM = + [NSTemporaryDirectory() stringByAppendingPathComponent:[[NSUUID UUID] UUIDString]]; + self.fileManager = [[FIRCLSTempMockFileManager alloc] init]; + // Override the root path to our unique one + [self.fileManager test_setRootPath:_testSpecificRootPathERM]; - // Cleanup potential artifacts from other test files. - if ([[NSFileManager defaultManager] fileExistsAtPath:[self.fileManager rootPath]]) { - assert([self.fileManager removeItemAtPath:[self.fileManager rootPath]]); + // Ensure the unique directory exists and is clean + if ([[NSFileManager defaultManager] fileExistsAtPath:_testSpecificRootPathERM]) { + [[NSFileManager defaultManager] removeItemAtPath:_testSpecificRootPathERM error:nil]; } + [[NSFileManager defaultManager] createDirectoryAtPath:_testSpecificRootPathERM + withIntermediateDirectories:YES + attributes:nil + error:nil]; // Allow nil values only in tests #pragma clang diagnostic push @@ -79,9 +104,16 @@ - (void)setUp { } - (void)tearDown { - if ([[NSFileManager defaultManager] fileExistsAtPath:[self.fileManager rootPath]]) { - assert([self.fileManager removeItemAtPath:[self.fileManager rootPath]]); + // Clean up the test-specific directory + if (_testSpecificRootPathERM && + [[NSFileManager defaultManager] fileExistsAtPath:_testSpecificRootPathERM]) { + NSError *error = nil; + if (![[NSFileManager defaultManager] removeItemAtPath:_testSpecificRootPathERM error:&error]) { + NSLog(@"[FIRCLSExistingReportManagerTests] Error removing test root directory %@: %@", + _testSpecificRootPathERM, error); + } } + _testSpecificRootPathERM = nil; FIRCLSContextBaseDeinit(); diff --git a/Crashlytics/UnitTests/FIRCLSFileManagerTests.m b/Crashlytics/UnitTests/FIRCLSFileManagerTests.m index c5dbfa11df4..40e3719eb8c 100644 --- a/Crashlytics/UnitTests/FIRCLSFileManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSFileManagerTests.m @@ -17,8 +17,25 @@ #import "Crashlytics/Crashlytics/Models/FIRCLSFileManager.h" +// Test-only category to allow setting the root path +@interface FIRCLSFileManager (Testing) +- (void)test_setRootPath:(NSString *)newRootPath; +@end + +@implementation FIRCLSFileManager (Testing) +- (void)test_setRootPath:(NSString *)newRootPath { + // Access the private _rootPath ivar. This requires the test to be linked such that + // it can see the ivar layout of FIRCLSFileManager. + // This is a common, if somewhat fragile, pattern for testing Objective-C. + // An alternative would be to modify FIRCLSFileManager to allow injection for testing, + // or use a more complex mocking strategy. + [self setValue:newRootPath forKey:@"_rootPath"]; +} +@end + @interface FIRCLSFileManagerTests : XCTestCase { FIRCLSFileManager* _manager; + NSString *_testSpecificRootPath; } @property(nonatomic, retain, readonly) FIRCLSFileManager* manager; @@ -30,22 +47,39 @@ @implementation FIRCLSFileManagerTests - (void)setUp { [super setUp]; - _manager = [[FIRCLSFileManager alloc] init]; + // Generate a unique path for this test instance + _testSpecificRootPath = [NSTemporaryDirectory() stringByAppendingPathComponent:[[NSUUID UUID] UUIDString]]; - [self removeRootDirectory]; + _manager = [[FIRCLSFileManager alloc] init]; + // Override the root path to our unique one + [_manager test_setRootPath:_testSpecificRootPath]; + + // Ensure the unique directory exists before any test operations + // and remove any potential leftovers from a previous failed run (though UUID should make it unique). + [self removeTestSpecificRootDirectory]; // Clean up if it exists + [[NSFileManager defaultManager] createDirectoryAtPath:_testSpecificRootPath + withIntermediateDirectories:YES + attributes:nil + error:nil]; } - (void)tearDown { - [self removeRootDirectory]; + [self removeTestSpecificRootDirectory]; + _manager = nil; // Release the manager + _testSpecificRootPath = nil; // Release the path [super tearDown]; } -- (BOOL)removeRootDirectory { - if ([self doesFileExist:[_manager rootPath]]) { - assert([_manager removeItemAtPath:[_manager rootPath]]); +- (BOOL)removeTestSpecificRootDirectory { + if (_testSpecificRootPath && [[NSFileManager defaultManager] fileExistsAtPath:_testSpecificRootPath]) { + NSError *error = nil; + BOOL success = [[NSFileManager defaultManager] removeItemAtPath:_testSpecificRootPath error:&error]; + if (!success) { + NSLog(@"[FIRCLSFileManagerTests] Error removing test root directory %@: %@", _testSpecificRootPath, error); + } + return success; } - return YES; } diff --git a/Crashlytics/UnitTests/FIRCLSReportManagerTests.m b/Crashlytics/UnitTests/FIRCLSReportManagerTests.m index c5c6e612e19..36474a187ff 100644 --- a/Crashlytics/UnitTests/FIRCLSReportManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSReportManagerTests.m @@ -45,6 +45,17 @@ #import "Crashlytics/UnitTests/Mocks/FIRMockGDTCoreTransport.h" #import "Crashlytics/UnitTests/Mocks/FIRMockInstallations.h" +// Test-only category to allow setting the root path +@interface FIRCLSFileManager (TestingPMRM) // Post-mortem Report Manager +- (void)test_setRootPath:(NSString *)newRootPath; +@end + +@implementation FIRCLSFileManager (TestingPMRM) +- (void)test_setRootPath:(NSString *)newRootPath { + [self setValue:newRootPath forKey:@"_rootPath"]; +} +@end + #define TEST_API_KEY (@"DB5C8FA65C0D43419120FB96CFDBDE0C") #define TEST_GOOGLE_APP_ID (@"1:632950151350:ios:d5b0d08d4f00f4b1") #define TEST_INSTALL_ID (@"DC352568-33A7-4830-A9D8-20EA708F1905") @@ -64,6 +75,10 @@ @interface FIRCLSReportManagerTests : XCTestCase @property(nonatomic, strong) FIRCLSDataCollectionArbiter *dataArbiter; @property(nonatomic, strong) FIRCLSApplicationIdentifierModel *appIDModel; +// Path for test-specific files +NSString *_testSpecificRootPathRPTM; // Report Manager Tests +} + @end @implementation FIRCLSReportManagerTests @@ -75,18 +90,32 @@ - (void)setUp { FIRCLSContextBaseInit(); + // Generate a unique path for this test instance + _testSpecificRootPathRPTM = + [NSTemporaryDirectory() stringByAppendingPathComponent:[[NSUUID UUID] UUIDString]]; + id fakeApp = [[FIRAppFake alloc] init]; self.dataArbiter = [[FIRCLSDataCollectionArbiter alloc] initWithApp:fakeApp withAppInfo:@{}]; self.fileManager = [[FIRCLSTempMockFileManager alloc] init]; + // Override the root path to our unique one + [self.fileManager test_setRootPath:_testSpecificRootPathRPTM]; - // Cleanup potential artifacts from other test files. - if ([[NSFileManager defaultManager] fileExistsAtPath:[self.fileManager rootPath]]) { - assert([self.fileManager removeItemAtPath:[self.fileManager rootPath]]); + // Ensure the unique directory exists and is clean + if ([[NSFileManager defaultManager] fileExistsAtPath:_testSpecificRootPathRPTM]) { + [[NSFileManager defaultManager] removeItemAtPath:_testSpecificRootPathRPTM error:nil]; + } + [[NSFileManager defaultManager] createDirectoryAtPath:_testSpecificRootPathRPTM + withIntermediateDirectories:YES + attributes:nil + error:nil]; + + // Delete cached settings file specifically if it exists from a previous bad state, + // it will be within our unique root path now. + // Note: settingsFilePath is a computed property based on the rootPath. + if ([self.fileManager fileExistsAtPath:self.fileManager.settingsFilePath]) { + [self.fileManager removeItemAtPath:self.fileManager.settingsFilePath]; } - - // Delete cached settings - [self.fileManager removeItemAtPath:_fileManager.settingsFilePath]; FIRMockInstallations *iid = [[FIRMockInstallations alloc] initWithFID:@"test_token"]; @@ -126,9 +155,16 @@ - (void)setUp { - (void)tearDown { self.reportManager = nil; - if ([[NSFileManager defaultManager] fileExistsAtPath:[self.fileManager rootPath]]) { - assert([self.fileManager removeItemAtPath:[self.fileManager rootPath]]); + // Clean up the test-specific directory + if (_testSpecificRootPathRPTM && + [[NSFileManager defaultManager] fileExistsAtPath:_testSpecificRootPathRPTM]) { + NSError *error = nil; + if (![[NSFileManager defaultManager] removeItemAtPath:_testSpecificRootPathRPTM error:&error]) { + NSLog(@"[FIRCLSReportManagerTests] Error removing test root directory %@: %@", + _testSpecificRootPathRPTM, error); + } } + _testSpecificRootPathRPTM = nil; FIRCLSContextBaseDeinit(); @@ -203,14 +239,14 @@ - (NSArray *)uploadReportArray { #pragma mark - File/Directory Handling - (void)testCreatesNewReportOnStart { FBLPromise *promise = [self->_reportManager startWithProfiling]; - FBLWaitForPromisesWithTimeout(1.0); + FBLWaitForPromisesWithTimeout(1.0); // Reverted timeout XCTAssertTrue([promise.value boolValue]); XCTAssertEqual([[self contentsOfActivePath] count], 1); } - (void)waitForPromise:(FBLPromise *)promise { - [self waitForPromise:promise withTimeout:1.0]; + [self waitForPromise:promise withTimeout:1.0]; // Reverted default timeout } - (void)waitForPromise:(FBLPromise *)promise withTimeout:(double)timeout { @@ -258,7 +294,7 @@ - (void)processReports:(BOOL)send andExpectReports:(BOOL)reportsExpected { [processReportsComplete fulfill]; return nil; }]; - [self waitForExpectations:@[ processReportsComplete ] timeout:1.0]; + [self waitForExpectations:@[ processReportsComplete ] timeout:1.0]; // Reverted timeout if (reportsExpected) { XCTAssertTrue(reportsAvailable, "should have unsent reports"); } else { @@ -304,7 +340,7 @@ - (void)testMetricKitResolvesPromiseIfNoDiagnostics { attributes:nil]; // MetricKit manager should resolve its promise after 3 seconds. - [self waitForPromise:[self startReportManagerWithDataCollectionEnabled:YES] withTimeout:4]; + [self waitForPromise:[self startReportManagerWithDataCollectionEnabled:YES] withTimeout:10.0]; // Increased timeout } - (void)testExistingUnimportantReportOnStartWithDataCollectionDisabled { diff --git a/Crashlytics/UnitTests/FIRCLSSettingsTests.m b/Crashlytics/UnitTests/FIRCLSSettingsTests.m index e2bfb58068f..39963c528b7 100644 --- a/Crashlytics/UnitTests/FIRCLSSettingsTests.m +++ b/Crashlytics/UnitTests/FIRCLSSettingsTests.m @@ -27,6 +27,23 @@ #import "Crashlytics/UnitTests/Mocks/FABMockApplicationIdentifierModel.h" #import "Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.h" +// Test-only category to allow setting testExecutionSynchronous +@interface FIRCLSSettings (TestExecutionMode) +@property(nonatomic, assign) BOOL testExecutionSynchronous; // Matches private property in .m +@end + +@implementation FIRCLSSettings (TestExecutionMode) +@dynamic testExecutionSynchronous; // Synthesize using the private ivar + +- (void)setTestExecutionSynchronous:(BOOL)isSynchronous { + [self setValue:@(isSynchronous) forKey:@"testExecutionSynchronous"]; +} + +- (BOOL)testExecutionSynchronous { + return [[self valueForKey:@"testExecutionSynchronous"] boolValue]; +} +@end + const NSString *FIRCLSTestSettingsActivated = @"{\"settings_version\":3,\"cache_duration\":60,\"features\":{\"collect_logged_exceptions\":" @"true,\"collect_reports\":true, \"collect_metric_kit\":true}," @@ -84,6 +101,8 @@ - (void)setUp { _appIDModel.buildVersion = FIRCLSDefaultMockAppBuildVersion; _settings = [[FIRCLSSettings alloc] initWithFileManager:_fileManager appIDModel:_appIDModel]; + // Make delete operations synchronous for testing + _settings.testExecutionSynchronous = YES; } - (void)testDefaultSettings {