[Crashlyics] Address some Crashlytics flakes and disable others in nightly tests

.github/workflows/common.yml

@@ -52,6 +52,12 @@ on:
         required: false
         default: ""
 
+      # An environment variable to be set for the test run.
+      env_var:
+        type: string
+        required: false
+        default: ""
+
     outputs:
       cache_key:
         description: "The cache key for the Swift package resolution."
@@ -105,6 +111,9 @@ jobs:
       with:
         path: .build
         key: ${{needs.spm-package-resolved.outputs.cache_key}}
+    - name: Set nightly env var
+      if: inputs.env_var != ''
+      run: echo "${{ inputs.env_var }}=1" >> $GITHUB_ENV
     - name: Xcode
       run: sudo xcode-select -s /Applications/${{ matrix.xcode }}.app/Contents/Developer
     - name: Run setup command, if needed.

.github/workflows/crashlytics.yml

@@ -30,6 +30,7 @@ jobs:
     uses: ./.github/workflows/common.yml
     with:
       target: FirebaseCrashlyticsUnit
+      env_var: ${{ github.event_name == 'schedule' && 'CRASHLYTICS_NIGHTLY' || '' }}
 
   catalyst:
     uses: ./.github/workflows/common_catalyst.yml
@@ -41,7 +42,8 @@ jobs:
     uses: ./.github/workflows/common_cocoapods.yml
     with:
       product: FirebaseCrashlytics
-      buildonly_platforms: tvOS, macOS, watchOS
+      # Run CI tests only on SPM.
+      buildonly_platforms: iOS, tvOS, macOS, watchOS
 
   quickstart:
     uses: ./.github/workflows/common_quickstart.yml

Crashlytics/UnitTests/FIRCLSContextManagerTests.m

@@ -42,6 +42,10 @@ @implementation FIRCLSContextManagerTests
 
 - (void)setUp {
   self.fileManager = [[FIRCLSMockFileManager alloc] init];
+  [[NSFileManager defaultManager] createDirectoryAtPath:self.fileManager.rootPath
+                            withIntermediateDirectories:YES
+                                             attributes:nil
+                                                  error:nil];
   [self.fileManager createReportDirectories];
   [self.fileManager setupNewPathForExecutionIdentifier:TestContextReportID];
 

Crashlytics/UnitTests/FIRCLSExistingReportManagerTests.m

@@ -54,10 +54,11 @@ - (void)setUp {
 
   self.fileManager = [[FIRCLSTempMockFileManager alloc] init];
 
-  // Cleanup potential artifacts from other test files.
+  // Clean up the directory and then re-create it to ensure a fresh state
   if ([[NSFileManager defaultManager] fileExistsAtPath:[self.fileManager rootPath]]) {
     assert([self.fileManager removeItemAtPath:[self.fileManager rootPath]]);
   }
+  [self.fileManager createReportDirectories];
 
   // Allow nil values only in tests
 #pragma clang diagnostic push

Crashlytics/UnitTests/FIRCLSReportUploaderTests.m

@@ -284,6 +284,9 @@ - (void)runUploadPackagedReportWithUrgency:(BOOL)urgent {
                                    asUrgent:urgent];
 
   XCTAssertNotNil(self.mockDataTransport.sendDataEvent_event);
+
+  // Wait a little bit for the file to be removed.
+  [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.1]];
   XCTAssertEqualObjects(self.fileManager.removedItemAtPath_path, [self packagePath]);
 }
 

Crashlytics/UnitTests/FIRCLSUserDefaultsTests.m

@@ -53,6 +53,9 @@ - (void)setUp {
 
 - (void)tearDown {
   [_userDefaults removeAllObjects];
+  [[NSUserDefaults standardUserDefaults] removeObjectForKey:_testKey1];
+  [[NSUserDefaults standardUserDefaults] removeObjectForKey:_testKey2];
+  [[NSUserDefaults standardUserDefaults] removeObjectForKey:_testKey3];
   [super tearDown];
 }
 

Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.h

@@ -16,14 +16,17 @@
 
 #import <XCTest/XCTest.h>
 
-@interface FIRCLSMockFileManager : FIRCLSFileManager
+// Notification posted when an item is removed via `removeItemAtPath`.
+extern NSNotificationName const FIRCLSMockFileManagerDidRemoveItemNotification;
 
-// Number of calls to removeItemAtPath are expected for the unit test
-@property(nonatomic) NSInteger expectedRemoveCount;
+@interface FIRCLSMockFileManager : FIRCLSFileManager
 
 // Incremented when a remove happens with removeItemAtPath
 @property(nonatomic) NSInteger removeCount;
 
+// Number of calls to removeItemAtPath are expected for the unit test
+@property(nonatomic) NSInteger expectedRemoveCount;
+
 // Will be fulfilled when the expected number of removes have happened
 // using removeItemAtPath
 //

Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.m

@@ -14,6 +14,9 @@
 
 #import "Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.h"
 
+NSNotificationName const FIRCLSMockFileManagerDidRemoveItemNotification =
+    @"FIRCLSMockFileManagerDidRemoveItemNotification";
+
 @interface FIRCLSMockFileManager ()
 
 @property(nonatomic) NSMutableDictionary<NSString *, NSData *> *fileSystemDict;
@@ -34,68 +37,82 @@ - (instancetype)init {
 }
 
 - (BOOL)removeItemAtPath:(NSString *)path {
-  [self.fileSystemDict removeObjectForKey:path];
-
-  self.removeCount += 1;
-
-  // If we set up the expectation, and we went over the expected count or removes, fulfill the
-  // expectation
-  if (self.removeExpectation && self.removeCount >= self.expectedRemoveCount) {
-    [self.removeExpectation fulfill];
+  @synchronized(self) {
+    [self.fileSystemDict removeObjectForKey:path];
+    self.removeCount += 1;
+
+    // If we set up the expectation, and we went over the expected count or removes, fulfill the
+    // expectation
+    if (self.removeExpectation && self.removeCount >= self.expectedRemoveCount) {
+      [self.removeExpectation fulfill];
+    }
   }
 
+  [[NSNotificationCenter defaultCenter]
+      postNotificationName:FIRCLSMockFileManagerDidRemoveItemNotification
+                    object:self];
+
   return YES;
 }
 
 - (BOOL)fileExistsAtPath:(NSString *)path {
-  return self.fileSystemDict[path] != nil;
+  @synchronized(self) {
+    return self.fileSystemDict[path] != nil;
+  }
 }
 
 - (BOOL)createFileAtPath:(NSString *)path
                 contents:(NSData *)data
               attributes:(NSDictionary<NSFileAttributeKey, id> *)attr {
-  self.fileSystemDict[path] = data;
+  @synchronized(self) {
+    self.fileSystemDict[path] = data;
+  }
   return YES;
 }
 
 - (NSArray *)activePathContents {
-  NSMutableArray *pathsWithActive = [[NSMutableArray alloc] init];
-  for (NSString *path in [_fileSystemDict allKeys]) {
-    if ([path containsString:@"v5/reports/active"]) {
-      [pathsWithActive addObject:path];
+  @synchronized(self) {
+    NSMutableArray *pathsWithActive = [[NSMutableArray alloc] init];
+    for (NSString *path in [_fileSystemDict allKeys]) {
+      if ([path containsString:@"v5/reports/active"]) {
+        [pathsWithActive addObject:path];
+      }
     }
+    return pathsWithActive;
   }
-
-  return pathsWithActive;
 }
 
 - (NSData *)dataWithContentsOfFile:(NSString *)path {
-  return self.fileSystemDict[path];
+  @synchronized(self) {
+    return self.fileSystemDict[path];
+  }
 }
 
 - (void)enumerateFilesInDirectory:(NSString *)directory
                        usingBlock:(void (^)(NSString *filePath, NSString *extension))block {
-  NSArray<NSString *> *filteredPaths = [self.fileSystemDict.allKeys
-      filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(NSString *path,
-                                                                        NSDictionary *bindings) {
-        return [path hasPrefix:directory];
-      }]];
-
-  for (NSString *path in filteredPaths) {
-    NSString *extension;
-    NSString *fullPath;
-
-    // Skip files that start with a dot.  This is important, because if you try to move a .DS_Store
-    // file, it will fail if the target directory also has a .DS_Store file in it.  Plus, its
-    // wasteful, because we don't care about dot files.
-    if ([path hasPrefix:@"."]) {
-      continue;
-    }
-
-    extension = [path pathExtension];
-    fullPath = [directory stringByAppendingPathComponent:path];
-    if (block) {
-      block(fullPath, extension);
+  @synchronized(self) {
+    NSArray<NSString *> *filteredPaths = [self.fileSystemDict.allKeys
+        filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(NSString *path,
+                                                                          NSDictionary *bindings) {
+          return [path hasPrefix:directory];
+        }]];
+
+    for (NSString *path in filteredPaths) {
+      NSString *extension;
+      NSString *fullPath;
+
+      // Skip files that start with a dot.  This is important, because if you try to move a
+      // .DS_Store file, it will fail if the target directory also has a .DS_Store file in
+      //  it.  Plus, it's wasteful, because we don't care about dot files.
+      if ([path hasPrefix:@"."]) {
+        continue;
+      }
+
+      extension = [path pathExtension];
+      fullPath = [directory stringByAppendingPathComponent:path];
+      if (block) {
+        block(fullPath, extension);
+      }
     }
   }
 }

Package.swift

@@ -626,27 +626,7 @@ let package = Package(
         .swiftLanguageMode(SwiftLanguageMode.v5),
       ]
     ),
-    .testTarget(
-      name: "FirebaseCrashlyticsUnit",
-      dependencies: ["FirebaseCrashlytics", .product(name: "OCMock", package: "ocmock")],
-      path: "Crashlytics/UnitTests",
-      resources: [
-        .copy("FIRCLSMachO/machO_data"),
-        .copy("Data"),
-      ],
-      cSettings: [
-        .headerSearchPath("../.."),
-        .define("DISPLAY_VERSION", to: firebaseVersion),
-        .define("CLS_SDK_NAME", to: "Crashlytics iOS SDK", .when(platforms: [.iOS])),
-        .define(
-          "CLS_SDK_NAME",
-          to: "Crashlytics macOS SDK",
-          .when(platforms: [.macOS, .macCatalyst])
-        ),
-        .define("CLS_SDK_NAME", to: "Crashlytics tvOS SDK", .when(platforms: [.tvOS])),
-        .define("CLS_SDK_NAME", to: "Crashlytics watchOS SDK", .when(platforms: [.watchOS])),
-      ]
-    ),
+    crashlyticsUnitTestChooser(),
     .target(
       name: "FirebaseDatabaseInternal",
       dependencies: [
@@ -1467,6 +1447,56 @@ func firestoreWrapperTarget() -> Target {
   )
 }
 
+func crashlyticsUnitTestChooser() -> Target {
+  // Don't run flaky tests in nightly runs.
+  if Context.environment["CRASHLYTICS_NIGHTLY"] != nil {
+    return .testTarget(
+      name: "FirebaseCrashlyticsUnit",
+      dependencies: ["FirebaseCrashlytics", .product(name: "OCMock", package: "ocmock")],
+      path: "Crashlytics/UnitTests",
+      exclude: ["FIRCrashlyticsReportTests.m"], // Flaky
+      resources: [
+        .copy("FIRCLSMachO/machO_data"),
+        .copy("Data"),
+      ],
+      cSettings: [
+        .headerSearchPath("../.."),
+        .define("DISPLAY_VERSION", to: firebaseVersion),
+        .define("CLS_SDK_NAME", to: "Crashlytics iOS SDK", .when(platforms: [.iOS])),
+        .define(
+          "CLS_SDK_NAME",
+          to: "Crashlytics macOS SDK",
+          .when(platforms: [.macOS, .macCatalyst])
+        ),
+        .define("CLS_SDK_NAME", to: "Crashlytics tvOS SDK", .when(platforms: [.tvOS])),
+        .define("CLS_SDK_NAME", to: "Crashlytics watchOS SDK", .when(platforms: [.watchOS])),
+      ]
+    )
+  } else {
+    return .testTarget(
+      name: "FirebaseCrashlyticsUnit",
+      dependencies: ["FirebaseCrashlytics", .product(name: "OCMock", package: "ocmock")],
+      path: "Crashlytics/UnitTests",
+      resources: [
+        .copy("FIRCLSMachO/machO_data"),
+        .copy("Data"),
+      ],
+      cSettings: [
+        .headerSearchPath("../.."),
+        .define("DISPLAY_VERSION", to: firebaseVersion),
+        .define("CLS_SDK_NAME", to: "Crashlytics iOS SDK", .when(platforms: [.iOS])),
+        .define(
+          "CLS_SDK_NAME",
+          to: "Crashlytics macOS SDK",
+          .when(platforms: [.macOS, .macCatalyst])
+        ),
+        .define("CLS_SDK_NAME", to: "Crashlytics tvOS SDK", .when(platforms: [.tvOS])),
+        .define("CLS_SDK_NAME", to: "Crashlytics watchOS SDK", .when(platforms: [.watchOS])),
+      ]
+    )
+  }
+}
+
 func firestoreTargets() -> [Target] {
   if Context.environment["FIREBASE_SOURCE_FIRESTORE"] != nil {
     return [