Fix Catalyst Flaky Test (#9797)

elenadoty · web-flow · commit b35b0deb7164 · 2022-05-18T13:48:36.000-04:00
diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSExistingReportManager.m b/Crashlytics/Crashlytics/Controllers/FIRCLSExistingReportManager.m
@@ -207,7 +207,7 @@ - (void)sendUnsentReportsWithToken:(FIRCLSDataCollectionToken *)dataCollectionTo
   // the file still exists.
   [self.operationQueue addOperationWithBlock:^{
     for (NSString *path in self.preparedReportPaths) {
-      if (![[self.fileManager underlyingFileManager] fileExistsAtPath:path]) {
+      if (![self.fileManager fileExistsAtPath:path]) {
         continue;
       }
       [self.reportUploader uploadPackagedReportAtPath:path
diff --git a/Crashlytics/Crashlytics/FIRCrashlytics.m b/Crashlytics/Crashlytics/FIRCrashlytics.m
@@ -131,7 +131,7 @@ - (instancetype)initWithApp:(FIRApp *)app
                                                                 appIDModel:appModel];
 
     FIRCLSOnDemandModel *onDemandModel =
-        [[FIRCLSOnDemandModel alloc] initWithFIRCLSSettings:settings];
+        [[FIRCLSOnDemandModel alloc] initWithFIRCLSSettings:settings fileManager:_fileManager];
     _managerData = [[FIRCLSManagerData alloc] initWithGoogleAppID:_googleAppID
                                                   googleTransport:googleTransport
                                                     installations:installations
diff --git a/Crashlytics/Crashlytics/Models/FIRCLSOnDemandModel.m b/Crashlytics/Crashlytics/Models/FIRCLSOnDemandModel.m
@@ -20,6 +20,7 @@
 #import "Crashlytics/Crashlytics/Handlers/FIRCLSException.h"
 #import "Crashlytics/Crashlytics/Helpers/FIRCLSLogger.h"
 #include "Crashlytics/Crashlytics/Helpers/FIRCLSUtility.h"
+#import "Crashlytics/Crashlytics/Models/FIRCLSFileManager.h"
 #import "Crashlytics/Crashlytics/Models/FIRCLSOnDemandModel.h"
 #import "Crashlytics/Crashlytics/Private/FIRCLSOnDemandModel_Private.h"
 
@@ -38,7 +39,7 @@ @interface FIRCLSOnDemandModel ()
 @property(nonatomic) double lastUpdated;
 @property(nonatomic) double currentStep;
 
-@property(nonatomic, strong) NSFileManager *fileManager;
+@property(nonatomic, strong) FIRCLSFileManager *fileManager;
 @property(nonatomic, strong) NSMutableArray *storedActiveReportPaths;
 
 @end
@@ -52,13 +53,15 @@ @implementation FIRCLSOnDemandModel
 static const double MAX_DELAY_SEC = 3600;
 static const double SEC_PER_MINUTE = 60;
 
-- (instancetype)initWithFIRCLSSettings:(FIRCLSSettings *)settings {
+- (instancetype)initWithFIRCLSSettings:(FIRCLSSettings *)settings
+                           fileManager:(FIRCLSFileManager *)fileManager {
   self = [super init];
   if (!self) {
     return nil;
   }
 
   _settings = settings;
+  _fileManager = fileManager;
 
   NSString *sdkBundleID = FIRCLSApplicationGetSDKBundleID();
   _operationQueue = [NSOperationQueue new];
@@ -74,7 +77,6 @@ - (instancetype)initWithFIRCLSSettings:(FIRCLSSettings *)settings {
 
   _lastUpdated = [NSDate timeIntervalSinceReferenceDate];
   _currentStep = -1;
-  _fileManager = [[NSFileManager alloc] init];
 
   self.storedActiveReportPaths = [NSMutableArray array];
 
@@ -99,8 +101,7 @@ - (BOOL)recordOnDemandExceptionIfQuota:(FIRExceptionModel *)exceptionModel
     }
 
     FIRCLSDataCollectionToken *dataCollectionToken = [FIRCLSDataCollectionToken validToken];
-    NSString *activeReportPath = FIRCLSExceptionRecordOnDemandModel(
-        exceptionModel, self.recordedOnDemandExceptionCount, self.droppedOnDemandExceptionCount);
+    NSString *activeReportPath = [self recordOnDemandExceptionWithModel:exceptionModel];
 
     if (!activeReportPath) {
       FIRCLSErrorLog(@"Error recording on-demand exception");
@@ -125,7 +126,7 @@ - (BOOL)recordOnDemandExceptionIfQuota:(FIRExceptionModel *)exceptionModel
       } else {
         [self.storedActiveReportPaths insertObject:activeReportPath atIndex:0];
         if ([self.storedActiveReportPaths count] > FIRCLSMaxUnsentReports) {
-          [self.fileManager removeItemAtPath:[self.storedActiveReportPaths lastObject] error:nil];
+          [self.fileManager removeItemAtPath:[self.storedActiveReportPaths lastObject]];
           [self.storedActiveReportPaths removeLastObject];
           [self decrementRecordedExceptionCount];
           [self incrementDroppedExceptionCount];
@@ -176,6 +177,11 @@ - (void)implementOnDemandUploadDelay:(int)delay {
   sleep(delay);
 }
 
+- (NSString *)recordOnDemandExceptionWithModel:(FIRExceptionModel *)exceptionModel {
+  return FIRCLSExceptionRecordOnDemandModel(exceptionModel, self.recordedOnDemandExceptionCount,
+                                            self.droppedOnDemandExceptionCount);
+}
+
 - (int)droppedOnDemandExceptionCount {
   @synchronized(self) {
     return _droppedOnDemandExceptionCount;
diff --git a/Crashlytics/Crashlytics/Private/FIRCLSOnDemandModel_Private.h b/Crashlytics/Crashlytics/Private/FIRCLSOnDemandModel_Private.h
@@ -24,7 +24,8 @@
 
 @interface FIRCLSOnDemandModel (Private)
 
-- (instancetype)initWithFIRCLSSettings:(FIRCLSSettings *)settings;
+- (instancetype)initWithFIRCLSSettings:(FIRCLSSettings *)settings
+                           fileManager:(FIRCLSFileManager *)fileManager;
 
 - (BOOL)recordOnDemandExceptionIfQuota:(FIRExceptionModel *)exceptionModel
              withDataCollectionEnabled:(BOOL)dataCollectionEnabled
diff --git a/Crashlytics/UnitTests/FIRCLSOnDemandModelTests.m b/Crashlytics/UnitTests/FIRCLSOnDemandModelTests.m
@@ -21,10 +21,10 @@
 #import "Crashlytics/Crashlytics/Private/FIRCLSOnDemandModel_Private.h"
 #import "Crashlytics/UnitTests/Mocks/FIRAppFake.h"
 #import "Crashlytics/UnitTests/Mocks/FIRCLSMockExistingReportManager.h"
+#import "Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.h"
 #import "Crashlytics/UnitTests/Mocks/FIRCLSMockOnDemandModel.h"
 #import "Crashlytics/UnitTests/Mocks/FIRCLSMockReportUploader.h"
 #import "Crashlytics/UnitTests/Mocks/FIRCLSMockSettings.h"
-#import "Crashlytics/UnitTests/Mocks/FIRCLSTempMockFileManager.h"
 #import "Crashlytics/UnitTests/Mocks/FIRMockGDTCoreTransport.h"
 #import "Crashlytics/UnitTests/Mocks/FIRMockInstallations.h"
 
@@ -40,7 +40,7 @@ @interface FIRCLSOnDemandModelTests : XCTestCase
 @property(nonatomic, strong) FIRCLSExistingReportManager *existingReportManager;
 @property(nonatomic, strong) FIRCLSManagerData *managerData;
 @property(nonatomic, strong) FIRCLSDataCollectionArbiter *dataArbiter;
-@property(nonatomic, strong) FIRCLSTempMockFileManager *fileManager;
+@property(nonatomic, strong) FIRCLSMockFileManager *fileManager;
 @property(nonatomic, strong) FIRCLSMockReportUploader *mockReportUploader;
 @property(nonatomic, strong) FIRCLSMockSettings *mockSettings;
 
@@ -57,12 +57,13 @@ - (void)setUp {
   id fakeApp = [[FIRAppFake alloc] init];
   self.dataArbiter = [[FIRCLSDataCollectionArbiter alloc] initWithApp:fakeApp withAppInfo:@{}];
 
-  self.fileManager = [[FIRCLSTempMockFileManager alloc] init];
+  self.fileManager = [[FIRCLSMockFileManager alloc] init];
 
   FIRCLSApplicationIdentifierModel *appIDModel = [[FIRCLSApplicationIdentifierModel alloc] init];
   _mockSettings = [[FIRCLSMockSettings alloc] initWithFileManager:self.fileManager
                                                        appIDModel:appIDModel];
   _onDemandModel = [[FIRCLSMockOnDemandModel alloc] initWithFIRCLSSettings:_mockSettings
+                                                               fileManager:_fileManager
                                                                 sleepBlock:^(int delay){
                                                                 }];
 
@@ -94,6 +95,7 @@ - (void)setUp {
   FIRCLSInternalReport *report =
       [[FIRCLSInternalReport alloc] initWithPath:reportPath
                              executionIdentifier:@"TEST_EXECUTION_IDENTIFIER"];
+
   FIRCLSContextInitialize(report, self.mockSettings, self.fileManager);
 }
 
@@ -133,14 +135,14 @@ - (void)testIncrementsQueueWhenEventRecorded {
 
 - (void)testCompliesWithDataCollectionOff {
   FIRExceptionModel *exceptionModel = [self getTestExceptionModel];
-  XCTestExpectation *testComplete =
+  XCTestExpectation *sleepComplete =
       [[XCTestExpectation alloc] initWithDescription:@"complete test"];
 
   // Put an expectation in the sleep block so we can test the state of the queue.
   __weak FIRCLSOnDemandModelTests *weakSelf = self;
   [self setSleepBlock:^(int delay) {
     XCTAssertEqual(delay, 60 / self.mockSettings.onDemandUploadRate);
-    [weakSelf waitForExpectations:@[ testComplete ] timeout:1.0];
+    [weakSelf waitForExpectations:@[ sleepComplete ] timeout:1.0];
   }];
 
   BOOL success = [self.onDemandModel recordOnDemandExceptionIfQuota:exceptionModel
@@ -149,14 +151,18 @@ - (void)testCompliesWithDataCollectionOff {
 
   // Should record but not submit a report.
   XCTAssertTrue(success);
+
   // We still count this as a recorded event if it was recorded but not submitted.
   XCTAssertEqual([self.onDemandModel recordedOnDemandExceptionCount], 1);
-  XCTAssertEqual([self contentsOfActivePath].count, 2);
   XCTAssertEqual(self.onDemandModel.getQueuedOperationsCount, 1);
-  XCTAssertEqual([self.onDemandModel.storedActiveReportPaths count], 1);
 
   // Fulfill the expectation so the sleep block completes.
-  [testComplete fulfill];
+  [sleepComplete fulfill];
+  [self.managerData.onDemandModel.operationQueue waitUntilAllOperationsAreFinished];
+
+  XCTAssertEqual(self.onDemandModel.getQueuedOperationsCount, 0);
+  XCTAssertEqual([self contentsOfActivePath].count, 1);
+  XCTAssertEqual([self.onDemandModel.storedActiveReportPaths count], 1);
 }
 
 - (void)testQuotaWithDataCollectionOff {
@@ -179,7 +185,7 @@ - (void)testQuotaWithDataCollectionOff {
 
   XCTAssertEqual([self.managerData.onDemandModel recordedOnDemandExceptionCount],
                  FIRCLSMaxUnsentReports);
-  XCTAssertEqual([self contentsOfActivePath].count, FIRCLSMaxUnsentReports + 1);
+  XCTAssertEqual([self contentsOfActivePath].count, FIRCLSMaxUnsentReports);
   XCTAssertEqual([self.managerData.onDemandModel.storedActiveReportPaths count],
                  FIRCLSMaxUnsentReports);
 
@@ -188,7 +194,8 @@ - (void)testQuotaWithDataCollectionOff {
                                                 asUrgent:YES];
   XCTAssertEqual([self.managerData.onDemandModel recordedOnDemandExceptionCount],
                  FIRCLSMaxUnsentReports);
-  XCTAssertEqual([self contentsOfActivePath].count, 1);
+  [self.existingReportManager.operationQueue waitUntilAllOperationsAreFinished];
+  XCTAssertEqual([self contentsOfActivePath].count, 0);
   XCTAssertEqual([self.managerData.onDemandModel.storedActiveReportPaths count], 0);
 }
 
@@ -234,8 +241,7 @@ - (void)testDroppedEventCountResets {
 
 #pragma mark - Helpers
 - (NSArray *)contentsOfActivePath {
-  return [[NSFileManager defaultManager] contentsOfDirectoryAtPath:self.fileManager.activePath
-                                                             error:nil];
+  return [self.fileManager activePathContents];
 }
 
 - (FIRExceptionModel *)getTestExceptionModel {
diff --git a/Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.m b/Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.m
@@ -58,6 +58,17 @@ - (BOOL)createFileAtPath:(NSString *)path
   return YES;
 }
 
+- (NSArray *)activePathContents {
+  NSMutableArray *pathsWithActive = [[NSMutableArray alloc] init];
+  for (NSString *path in [_fileSystemDict allKeys]) {
+    if ([path containsString:@"v5/reports/active"]) {
+      [pathsWithActive addObject:path];
+    }
+  }
+
+  return pathsWithActive;
+}
+
 - (NSData *)dataWithContentsOfFile:(NSString *)path {
   return self.fileSystemDict[path];
 }
diff --git a/Crashlytics/UnitTests/Mocks/FIRCLSMockOnDemandModel.h b/Crashlytics/UnitTests/Mocks/FIRCLSMockOnDemandModel.h
@@ -18,6 +18,7 @@
 @interface FIRCLSMockOnDemandModel : FIRCLSOnDemandModel
 
 - (instancetype)initWithFIRCLSSettings:(FIRCLSSettings *)settings
+                           fileManager:(FIRCLSFileManager *)fileManager
                             sleepBlock:(void (^)(int))sleepBlock;
 
 // Public for testing purposes
diff --git a/Crashlytics/UnitTests/Mocks/FIRCLSMockOnDemandModel.m b/Crashlytics/UnitTests/Mocks/FIRCLSMockOnDemandModel.m
@@ -13,22 +13,26 @@
 // limitations under the License.
 
 #import "Crashlytics/UnitTests/Mocks/FIRCLSMockOnDemandModel.h"
+#import "Crashlytics/Crashlytics/Models/FIRCLSFileManager.h"
 
 @interface FIRCLSMockOnDemandModel ()
 
 @property(nonatomic, readonly) FIRCLSSettings *settings;
+@property(nonatomic, readonly) FIRCLSFileManager *fileManager;
 
 @end
 
 @implementation FIRCLSMockOnDemandModel
 
 - (instancetype)initWithFIRCLSSettings:(FIRCLSSettings *)settings
+                           fileManager:(FIRCLSFileManager *)fileManager
                             sleepBlock:(void (^)(int))sleepBlock {
-  self = [super initWithFIRCLSSettings:settings];
+  self = [super initWithFIRCLSSettings:settings fileManager:fileManager];
   if (!self) {
     return nil;
   }
   _settings = settings;
+  _fileManager = fileManager;
   _sleepBlock = sleepBlock;
   return self;
 }
@@ -48,4 +52,15 @@ - (void)implementOnDemandUploadDelay:(int)delay {
   _sleepBlock(delay);
 }
 
+- (NSString *)recordOnDemandExceptionWithModel:(FIRExceptionModel *)exceptionModel {
+  int randomSuffix = arc4random_uniform(10000);
+  NSString *activePath = [_fileManager activePath];
+  NSString *exceptionName =
+      [activePath stringByAppendingPathComponent:[NSString stringWithFormat:@"test_exception_%d",
+                                                                            randomSuffix]];
+  NSData *data = [exceptionName dataUsingEncoding:NSUTF8StringEncoding];
+  [self.fileManager createFileAtPath:exceptionName contents:data attributes:nil];
+  return exceptionName;
+}
+
 @end

Original file line number	Diff line number	Diff line change
`@@ -207,7 +207,7 @@ - (void)sendUnsentReportsWithToken:(FIRCLSDataCollectionToken *)dataCollectionTo`
`207`	`207`	`// the file still exists.`
`208`	`208`	`[self.operationQueue addOperationWithBlock:^{`
`209`	`209`	`for (NSString *path in self.preparedReportPaths) {`
`210`		`- if (![[self.fileManager underlyingFileManager] fileExistsAtPath:path]) {`
	`210`	`+ if (![self.fileManager fileExistsAtPath:path]) {`
`211`	`211`	`continue;`
`212`	`212`	`}`
`213`	`213`	`[self.reportUploader uploadPackagedReportAtPath:path`