[Infra] Address flakiness in Crashlytics unit tests

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;

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();
 

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;
 }
 

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,7 +75,11 @@
 @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 @@
 
   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)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 @@
 #pragma mark - File/Directory Handling
 - (void)testCreatesNewReportOnStart {
   FBLPromise<NSNumber *> *promise = [self->_reportManager startWithProfiling];
-  FBLWaitForPromisesWithTimeout(1.0);
+  FBLWaitForPromisesWithTimeout(1.0); // Reverted timeout
 
   XCTAssertTrue([promise.value boolValue]);
   XCTAssertEqual([[self contentsOfActivePath] count], 1);
 }
 
 - (void)waitForPromise:(FBLPromise<NSNumber *> *)promise {
-  [self waitForPromise:promise withTimeout:1.0];
+  [self waitForPromise:promise withTimeout:1.0]; // Reverted default timeout
 }
 
 - (void)waitForPromise:(FBLPromise<NSNumber *> *)promise withTimeout:(double)timeout {
@@ -258,7 +294,7 @@
     [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 @@
                           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 {

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 {