Fix and enable flakey Crashlytics tests (#5145)

Author: jasonhu-g

Crashlytics/Crashlytics/Models/FIRCLSFileManager.h

@@ -82,4 +82,6 @@
 
 - (BOOL)moveItemAtPath:(NSString *)srcPath toPath:(NSString *)dstPath error:(NSError **)error;
 
+- (NSData *)dataWithContentsOfFile:(NSString *)path;
+
 @end

Crashlytics/Crashlytics/Models/FIRCLSFileManager.m

@@ -332,4 +332,9 @@ - (BOOL)moveItemAtPath:(NSString *)srcPath toPath:(NSString *)dstPath error:(NSE
   return [self.underlyingFileManager moveItemAtPath:srcPath toPath:dstPath error:error];
 }
 
+// Wrapper over NSData so the method can be mocked for unit tests
+- (NSData *)dataWithContentsOfFile:(NSString *)path {
+  return [NSData dataWithContentsOfFile:path];
+}
+
 @end

Crashlytics/Crashlytics/Models/FIRCLSSettings.m

@@ -66,7 +66,8 @@ - (void)reloadFromCacheWithGoogleAppID:(NSString *)googleAppID
                       currentTimestamp:(NSTimeInterval)currentTimestamp {
   NSString *settingsFilePath = self.fileManager.settingsFilePath;
 
-  NSData *data = [NSData dataWithContentsOfFile:settingsFilePath];
+  NSData *data = [self.fileManager dataWithContentsOfFile:settingsFilePath];
+
   if (!data) {
     FIRCLSDebugLog(@"[Crashlytics:Settings] No settings were cached");
 
@@ -173,7 +174,8 @@ - (void)cacheSettingsWithGoogleAppID:(NSString *)googleAppID
 #pragma mark - Convenience Methods
 
 - (NSDictionary *)loadCacheKey {
-  NSData *cacheKeyData = [NSData dataWithContentsOfFile:self.fileManager.settingsCacheKeyPath];
+  NSData *cacheKeyData =
+      [self.fileManager dataWithContentsOfFile:self.fileManager.settingsCacheKeyPath];
 
   if (!cacheKeyData) {
     return nil;

Crashlytics/UnitTests/FIRCLSFileManagerTests.m

@@ -15,13 +15,13 @@
 #import <Foundation/Foundation.h>
 #import <XCTest/XCTest.h>
 
-#import "FIRCLSMockFileManager.h"
+#import "FIRCLSFileManager.h"
 
 @interface FIRCLSFileManagerTests : XCTestCase {
-  FIRCLSMockFileManager* _manager;
+  FIRCLSFileManager* _manager;
 }
 
-@property(nonatomic, retain, readonly) FIRCLSMockFileManager* manager;
+@property(nonatomic, retain, readonly) FIRCLSFileManager* manager;
 
 @end
 
@@ -30,8 +30,7 @@ @implementation FIRCLSFileManagerTests
 - (void)setUp {
   [super setUp];
 
-  _manager = [[FIRCLSMockFileManager alloc] init];
-  [_manager setPathNamespace:@"com.crashlytics.unittests"];
+  _manager = [[FIRCLSFileManager alloc] init];
 
   [self removeRootDirectory];
 }

Crashlytics/UnitTests/FIRCLSPackageReportsOperationTests.m

@@ -17,10 +17,10 @@
 
 #import "FABMockApplicationIdentifierModel.h"
 #import "FIRCLSInternalReport.h"
-#import "FIRCLSMockFileManager.h"
 #import "FIRCLSMockSettings.h"
 #import "FIRCLSPackageReportOperation.h"
 #import "FIRCLSSettings.h"
+#import "FIRCLSTempMockFileManager.h"
 
 NSString *const TestOrgID = @"TestOrgID";
 
@@ -43,8 +43,7 @@ - (void)setUp {
   self.settings = [[FIRCLSMockSettings alloc] initWithFileManager:self.fileManager
                                                        appIDModel:appIDModel];
 
-  FIRCLSMockFileManager *manager = [[FIRCLSMockFileManager alloc] init];
-  [manager setPathNamespace:@"com.crashlytics.unittests"];
+  FIRCLSTempMockFileManager *manager = [[FIRCLSTempMockFileManager alloc] init];
 
   self.fileManager = manager;
 

Crashlytics/UnitTests/FIRCLSReportManagerTests.m

@@ -33,10 +33,10 @@
 #import "FABMockApplicationIdentifierModel.h"
 #import "FIRAppFake.h"
 #import "FIRCLSApplicationIdentifierModel.h"
-#import "FIRCLSMockFileManager.h"
 #import "FIRCLSMockReportManager.h"
 #import "FIRCLSMockReportUploader.h"
 #import "FIRCLSMockSettings.h"
+#import "FIRCLSTempMockFileManager.h"
 #import "FIRMockGDTCoreTransport.h"
 #import "FIRMockInstallations.h"
 
@@ -50,7 +50,7 @@
 
 @interface FIRCLSReportManagerTests : XCTestCase
 
-@property(nonatomic, strong) FIRCLSMockFileManager *fileManager;
+@property(nonatomic, strong) FIRCLSTempMockFileManager *fileManager;
 @property(nonatomic, strong) FIRCLSMockReportManager *reportManager;
 @property(nonatomic, strong) FIRCLSDataCollectionArbiter *dataArbiter;
 @property(nonatomic, strong) FIRCLSApplicationIdentifierModel *appIDModel;
@@ -70,8 +70,7 @@ - (void)setUp {
   id fakeApp = [[FIRAppFake alloc] init];
   self.dataArbiter = [[FIRCLSDataCollectionArbiter alloc] initWithApp:fakeApp withAppInfo:@{}];
 
-  self.fileManager = [[FIRCLSMockFileManager alloc] init];
-  [self.fileManager setPathNamespace:TEST_BUNDLE_ID];
+  self.fileManager = [[FIRCLSTempMockFileManager alloc] init];
 
   // Delete cached settings
   [self.fileManager removeItemAtPath:_fileManager.settingsFilePath];

Crashlytics/UnitTests/FIRCLSReportUploaderTests.m

@@ -23,10 +23,10 @@
 #include "FIRCLSDefines.h"
 #include "FIRCLSFileManager.h"
 #include "FIRCLSInternalReport.h"
-#include "FIRCLSMockFileManager.h"
 #include "FIRCLSMockNetworkClient.h"
 #include "FIRCLSMockSettings.h"
 #include "FIRCLSSettings.h"
+#include "FIRCLSTempMockFileManager.h"
 #include "FIRMockGDTCoreTransport.h"
 
 NSString *const TestEndpoint = @"https://reports.crashlytics.com";
@@ -35,7 +35,7 @@ @interface FIRCLSReportUploaderTests
     : XCTestCase <FIRCLSReportUploaderDelegate, FIRCLSReportUploaderDataSource>
 
 @property(nonatomic, strong) FIRCLSReportUploader *uploader;
-@property(nonatomic, strong) FIRCLSMockFileManager *fileManager;
+@property(nonatomic, strong) FIRCLSTempMockFileManager *fileManager;
 @property(nonatomic, strong) NSOperationQueue *queue;
 @property(nonatomic, strong) FIRCLSMockNetworkClient *networkClient;
 
@@ -58,7 +58,7 @@ - (void)setUp {
   self.networkClient = [[FIRCLSMockNetworkClient alloc] initWithQueue:self.queue
                                                           fileManager:self.fileManager
                                                              delegate:nil];
-  self.fileManager = [[FIRCLSMockFileManager alloc] init];
+  self.fileManager = [[FIRCLSTempMockFileManager alloc] init];
   self.uploader = [[FIRCLSReportUploader alloc] initWithQueue:self.queue
                                                      delegate:self
                                                    dataSource:self

Crashlytics/UnitTests/FIRCLSSettingsTests.m

@@ -12,11 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// TODO: There is some unreliability with this test.
-//       self.settings.settingsDictionary returns nil.
-//       Abstract FileManager so actual disk operations are not happening.
-
-/*
 #import "FIRCLSSettings.h"
 
 #import <Foundation/Foundation.h>
@@ -82,10 +77,6 @@ - (void)setUp {
 
   _fileManager = [[FIRCLSMockFileManager alloc] init];
 
-  // Delete the cache
-  [_fileManager removeItemAtPath:_fileManager.settingsFilePath];
-  [_fileManager removeItemAtPath:_fileManager.settingsCacheKeyPath];
-
   _appIDModel = [[FABMockApplicationIdentifierModel alloc] init];
   _appIDModel.buildInstanceID = FIRCLSDefaultMockBuildInstanceID;
   _appIDModel.displayVersion = FIRCLSDefaultMockAppDisplayVersion;
@@ -94,10 +85,6 @@ - (void)setUp {
   _settings = [[FIRCLSSettings alloc] initWithFileManager:_fileManager appIDModel:_appIDModel];
 }
 
-- (void)tearDown {
-  [_fileManager removeContentsOfDirectoryAtPath:_fileManager.settingsDirectoryPath];
-}
-
 - (void)testDefaultSettings {
   XCTAssertEqual(self.settings.isCacheExpired, YES);
 
@@ -135,18 +122,9 @@ - (BOOL)writeSettings:(const NSString *)settings
     path = _fileManager.settingsCacheKeyPath;
   }
 
-  // Create the directory.
-  [[NSFileManager defaultManager] createDirectoryAtPath:path.stringByDeletingLastPathComponent
-                            withIntermediateDirectories:YES
-                                             attributes:nil
-                                                  error:error];
-  if (*error != nil) {
-    return NO;
-  }
-
-  // Create the file.
-  [settings writeToFile:path atomically:NO encoding:NSUTF8StringEncoding error:error];
-  return YES;
+  return [self.fileManager createFileAtPath:path
+                                   contents:[settings dataUsingEncoding:NSUTF8StringEncoding]
+                                 attributes:nil];
 }
 
 - (void)cacheSettingsWithGoogleAppID:(NSString *)googleAppID
@@ -378,8 +356,8 @@ - (void)testGoogleAppIDChanged {
 }
 
 // This is a weird case where we got settings, but never created a cache key for it. We are
-/ treating / this as if the cache was invalid and re - fetching in this case.-
-    (void)testActivatedSettingsMissingCacheKey {
+// treating this as if the cache was invalid and re-fetching in this case.
+- (void)testActivatedSettingsMissingCacheKey {
   NSError *error = nil;
   [self writeSettings:FIRCLSTestSettingsActivated error:&error];
   XCTAssertNil(error, "%@", error);
@@ -550,4 +528,3 @@ - (void)testShouldUseNewReportEndpointWithEmptyDictionary {
 }
 
 @end
-*/

Crashlytics/UnitTests/FIRRecordExceptionModelTests.m

@@ -39,7 +39,6 @@ @implementation FIRRecordExceptionModelTests
 
 - (void)setUp {
   self.fileManager = [[FIRCLSMockFileManager alloc] init];
-  [self.fileManager setPathNamespace:TEST_BUNDLE_ID];
 
   FABMockApplicationIdentifierModel *appIDModel = [[FABMockApplicationIdentifierModel alloc] init];
   self.mockSettings = [[FIRCLSMockSettings alloc] initWithFileManager:self.fileManager

Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.h

@@ -18,33 +18,16 @@
 
 @interface FIRCLSMockFileManager : FIRCLSFileManager
 
-@property(nonatomic, copy) NSString *pathNamespace;
-
 // Number of calls to removeItemAtPath are expected for the unit test
 @property(nonatomic) NSInteger expectedRemoveCount;
 
 // Incremented when a remove happens with removeItemAtPath
 @property(nonatomic) NSInteger removeCount;
 
-// Overrides fileSizeAtPath if set
-@property(nonatomic, copy) NSNumber *fileSizeAtPathResult;
-
 // Will be fulfilled when the expected number of removes have happened
 // using removeItemAtPath
 //
 // Users should initialize this in their test.
 @property(nonatomic, strong) XCTestExpectation *removeExpectation;
 
-@property(nonatomic, copy) NSString *removedItemAtPath_path;
-
-// Overriding the method for testing Settings
-- (BOOL)removeItemAtPath:(NSString *)path;
-
-// Overrides moveItemAtPath if set
-@property(nonatomic) NSNumber *moveItemAtPathResult;
-@property(nonatomic, copy) NSString *moveItemAtPath_path;
-@property(nonatomic, copy) NSString *moveItemAtPath_destDir;
-
-- (BOOL)moveItemAtPath:(NSString *)path toDirectory:(NSString *)destDir;
-
 @end

Crashlytics/UnitTests/Mocks/FIRCLSMockFileManager.m

@@ -1,4 +1,4 @@
-// Copyright 2019 Google
+// Copyright 2020 Google
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -14,26 +14,27 @@
 
 #import "FIRCLSMockFileManager.h"
 
-@implementation FIRCLSMockFileManager
+@interface FIRCLSMockFileManager ()
+
+@property(nonatomic) NSMutableDictionary<NSString *, NSData *> *fileSystemDict;
+
+@end
 
-@synthesize pathNamespace;
+@implementation FIRCLSMockFileManager
 
 - (instancetype)init {
   self = [super init];
   if (!self) {
     return nil;
   }
 
-  // Should be set by the tests when needed
-  _removeExpectation = nil;
+  _fileSystemDict = [[NSMutableDictionary<NSString *, NSData *> alloc] init];
 
   return self;
 }
 
 - (BOOL)removeItemAtPath:(NSString *)path {
-  self.removedItemAtPath_path = path;
-
-  [super removeItemAtPath:path];
+  [self.fileSystemDict removeObjectForKey:path];
 
   self.removeCount += 1;
 
@@ -46,23 +47,46 @@ - (BOOL)removeItemAtPath:(NSString *)path {
   return YES;
 }
 
-- (NSNumber *)fileSizeAtPath:(NSString *)path {
-  if (self.fileSizeAtPathResult != nil) {
-    return self.fileSizeAtPathResult;
-  }
+- (BOOL)fileExistsAtPath:(NSString *)path {
+  return self.fileSystemDict[path] != nil;
+}
 
-  return [super fileSizeAtPath:path];
+- (BOOL)createFileAtPath:(NSString *)path
+                contents:(NSData *)data
+              attributes:(NSDictionary<NSFileAttributeKey, id> *)attr {
+  self.fileSystemDict[path] = data;
+  return YES;
 }
 
-- (BOOL)moveItemAtPath:(NSString *)path toDirectory:(NSString *)destDir {
-  self.moveItemAtPath_path = path;
-  self.moveItemAtPath_destDir = destDir;
+- (NSData *)dataWithContentsOfFile:(NSString *)path {
+  return self.fileSystemDict[path];
+}
 
-  if (self.moveItemAtPathResult != nil) {
-    return self.moveItemAtPathResult.intValue > 0;
+- (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);
+    }
   }
-
-  return [super moveItemAtPath:path toDirectory:destDir];
 }
 
 @end