Add test-related functionality, make GDLRegistrar thread-safe and tested (#2285)

* Add some functionality to the test prioritizer

* Change the registrar's API and make it thread safe.

* Add an error message enum for a failed upload

* Add more functionality to the log storage fake

* Make a property readonly.

Author: mikehaney24

GoogleDataLogger/GoogleDataLogger/Classes/GDLRegistrar.m

@@ -18,7 +18,13 @@
 
 #import "GDLRegistrar_Private.h"
 
-@implementation GDLRegistrar
+@implementation GDLRegistrar {
+  /** Backing ivar for logTargetToUploader property. */
+  NSMutableDictionary<NSNumber *, id<GDLLogUploader>> *_logTargetToUploader;
+
+  /** Backing ivar for logTargetToPrioritizer property. */
+  NSMutableDictionary<NSNumber *, id<GDLLogPrioritizer>> *_logTargetToPrioritizer;
+}
 
 + (instancetype)sharedInstance {
   static GDLRegistrar *sharedInstance;
@@ -32,19 +38,55 @@ + (instancetype)sharedInstance {
 - (instancetype)init {
   self = [super init];
   if (self) {
+    _registrarQueue = dispatch_queue_create("com.google.GDLRegistrar", DISPATCH_QUEUE_CONCURRENT);
     _logTargetToPrioritizer = [[NSMutableDictionary alloc] init];
     _logTargetToUploader = [[NSMutableDictionary alloc] init];
   }
   return self;
 }
 
-- (void)registerBackend:(id<GDLLogUploader>)backend forLogTarget:(GDLLogTarget)logTarget {
-  self.logTargetToUploader[@(logTarget)] = backend;
+- (void)registerUploader:(id<GDLLogUploader>)backend logTarget:(GDLLogTarget)logTarget {
+  __weak GDLRegistrar *weakSelf = self;
+  dispatch_barrier_async(_registrarQueue, ^{
+    GDLRegistrar *strongSelf = weakSelf;
+    if (strongSelf) {
+      strongSelf->_logTargetToUploader[@(logTarget)] = backend;
+    }
+  });
+}
+
+- (void)registerPrioritizer:(id<GDLLogPrioritizer>)prioritizer logTarget:(GDLLogTarget)logTarget {
+  __weak GDLRegistrar *weakSelf = self;
+  dispatch_barrier_async(_registrarQueue, ^{
+    GDLRegistrar *strongSelf = weakSelf;
+    if (strongSelf) {
+      strongSelf->_logTargetToPrioritizer[@(logTarget)] = prioritizer;
+    }
+  });
 }
 
-- (void)registerLogPrioritizer:(id<GDLLogPrioritizer>)prioritizer
-                  forLogTarget:(GDLLogTarget)logTarget {
-  self.logTargetToPrioritizer[@(logTarget)] = prioritizer;
+- (NSMutableDictionary<NSNumber *, id<GDLLogUploader>> *)logTargetToUploader {
+  __block NSMutableDictionary<NSNumber *, id<GDLLogUploader>> *logTargetToUploader;
+  __weak GDLRegistrar *weakSelf = self;
+  dispatch_sync(_registrarQueue, ^{
+    GDLRegistrar *strongSelf = weakSelf;
+    if (strongSelf) {
+      logTargetToUploader = strongSelf->_logTargetToUploader;
+    }
+  });
+  return logTargetToUploader;
+}
+
+- (NSMutableDictionary<NSNumber *, id<GDLLogPrioritizer>> *)logTargetToPrioritizer {
+  __block NSMutableDictionary<NSNumber *, id<GDLLogPrioritizer>> *logTargetToPrioritizer;
+  __weak GDLRegistrar *weakSelf = self;
+  dispatch_sync(_registrarQueue, ^{
+    GDLRegistrar *strongSelf = weakSelf;
+    if (strongSelf) {
+      logTargetToPrioritizer = strongSelf->_logTargetToPrioritizer;
+    }
+  });
+  return logTargetToPrioritizer;
 }
 
 @end

GoogleDataLogger/GoogleDataLogger/Classes/Private/GDLRegistrar_Private.h

@@ -18,10 +18,15 @@
 
 @interface GDLRegistrar ()
 
+/** The concurrent queue on which all registration occurs. */
+@property(nonatomic, readonly) dispatch_queue_t registrarQueue;
+
 /** A map of logTargets to backend implementations. */
-@property(nonatomic) NSMutableDictionary<NSNumber *, id<GDLLogUploader>> *logTargetToUploader;
+@property(atomic, readonly)
+    NSMutableDictionary<NSNumber *, id<GDLLogUploader>> *logTargetToUploader;
 
 /** A map of logTargets to prioritizer implementations. */
-@property(nonatomic) NSMutableDictionary<NSNumber *, id<GDLLogPrioritizer>> *logTargetToPrioritizer;
+@property(atomic, readonly)
+    NSMutableDictionary<NSNumber *, id<GDLLogPrioritizer>> *logTargetToPrioritizer;
 
 @end

GoogleDataLogger/GoogleDataLogger/Classes/Public/GDLRegistrar.h

@@ -36,15 +36,14 @@ NS_ASSUME_NONNULL_BEGIN
  * @param backend The backend object to register.
  * @param logTarget The logTarget this backend object will be responsible for.
  */
-- (void)registerBackend:(id<GDLLogUploader>)backend forLogTarget:(GDLLogTarget)logTarget;
+- (void)registerUploader:(id<GDLLogUploader>)backend logTarget:(GDLLogTarget)logTarget;
 
 /** Registers a log prioritizer implementation with the GoogleDataLogger infrastructure.
  *
  * @param prioritizer The prioritizer object to register.
  * @param logTarget The logTarget this prioritizer object will be responsible for.
  */
-- (void)registerLogPrioritizer:(id<GDLLogPrioritizer>)prioritizer
-                  forLogTarget:(GDLLogTarget)logTarget;
+- (void)registerPrioritizer:(id<GDLLogPrioritizer>)prioritizer logTarget:(GDLLogTarget)logTarget;
 
 @end
 

GoogleDataLogger/GoogleDataLogger/DependencyWrappers/GDLConsoleLogger.h

@@ -33,6 +33,9 @@ typedef NS_ENUM(NSInteger, GDLMessageCode) {
   /** For warning messages concerning protoBytes: not being implemented by a log extension. */
   GDLMCWExtensionMissingBytesImpl = 1,
 
+  /** For warning message concerning a failed log upload. */
+  GDLMCWUploadFailed = 2,
+
   /** For error messages concerning transform: not being implemented by a log transformer. */
   GDLMCETransformerDoesntImplementTransform = 1000,
 

GoogleDataLogger/Tests/Unit/Categories/GDLRegistrar+Testing.h

@@ -15,6 +15,7 @@
  */
 
 #import "GDLRegistrar.h"
+#import "GDLRegistrar_Private.h"
 
 NS_ASSUME_NONNULL_BEGIN
 

GoogleDataLogger/Tests/Unit/Categories/GDLRegistrar+Testing.m

@@ -21,8 +21,10 @@
 @implementation GDLRegistrar (Testing)
 
 - (void)reset {
-  [self.logTargetToPrioritizer removeAllObjects];
-  [self.logTargetToUploader removeAllObjects];
+  dispatch_sync(self.registrarQueue, ^{
+    [self.logTargetToPrioritizer removeAllObjects];
+    [self.logTargetToUploader removeAllObjects];
+  });
 }
 
 @end

GoogleDataLogger/Tests/Unit/Fakes/GDLLogStorageFake.h

@@ -21,6 +21,9 @@ NS_ASSUME_NONNULL_BEGIN
 /** A functionless fake that can be injected into classes that need it. */
 @interface GDLLogStorageFake : GDLLogStorage
 
+/** The logs to return from -logHashesToFiles. */
+@property(nonatomic) NSSet<NSURL *> *logsToReturnFromLogHashesToFiles;
+
 @end
 
 NS_ASSUME_NONNULL_END

GoogleDataLogger/Tests/Unit/Fakes/GDLLogStorageFake.m

@@ -21,4 +21,18 @@ @implementation GDLLogStorageFake
 - (void)storeLog:(GDLLogEvent *)log {
 }
 
+- (NSSet<NSURL *> *)logHashesToFiles:(NSSet<NSNumber *> *)logHashes {
+  if (_logsToReturnFromLogHashesToFiles) {
+    return _logsToReturnFromLogHashesToFiles;
+  } else {
+    return [[NSSet alloc] init];
+  }
+}
+
+- (void)removeLog:(NSNumber *)logHash logTarget:(NSNumber *)logTarget {
+}
+
+- (void)removeLogs:(NSSet<NSNumber *> *)logHashes logTarget:(NSNumber *)logTarget {
+}
+
 @end

GoogleDataLogger/Tests/Unit/GDLLogStorageTest.m

@@ -53,8 +53,8 @@ - (void)setUp {
   [super setUp];
   self.testBackend = [[GDLTestUploader alloc] init];
   self.testPrioritizer = [[GDLTestPrioritizer alloc] init];
-  [[GDLRegistrar sharedInstance] registerBackend:_testBackend forLogTarget:logTarget];
-  [[GDLRegistrar sharedInstance] registerLogPrioritizer:_testPrioritizer forLogTarget:logTarget];
+  [[GDLRegistrar sharedInstance] registerUploader:_testBackend logTarget:logTarget];
+  [[GDLRegistrar sharedInstance] registerPrioritizer:_testPrioritizer logTarget:logTarget];
   self.uploaderFake = [[GDLUploadCoordinatorFake alloc] init];
   [GDLLogStorage sharedInstance].uploader = self.uploaderFake;
 }

GoogleDataLogger/Tests/Unit/GDLRegistrarTest.m

@@ -18,15 +18,41 @@
 
 #import <GoogleDataLogger/GDLRegistrar.h>
 
+#import "GDLRegistrar_Private.h"
+#import "GDLTestPrioritizer.h"
+#import "GDLTestUploader.h"
+
 @interface GDLRegistrarTest : GDLTestCase
 
+@property(nonatomic) GDLLogTarget logTarget;
+
 @end
 
 @implementation GDLRegistrarTest
 
+- (void)setUp {
+  _logTarget = 23;
+}
+
 /** Tests the default initializer. */
 - (void)testInit {
   XCTAssertNotNil([[GDLRegistrarTest alloc] init]);
 }
 
+/** Test registering an uploader. */
+- (void)testRegisterUpload {
+  GDLRegistrar *registrar = [GDLRegistrar sharedInstance];
+  GDLTestUploader *uploader = [[GDLTestUploader alloc] init];
+  XCTAssertNoThrow([registrar registerUploader:uploader logTarget:self.logTarget]);
+  XCTAssertEqual(uploader, registrar.logTargetToUploader[@(_logTarget)]);
+}
+
+/** Test registering a prioritizer. */
+- (void)testRegisterPrioritizer {
+  GDLRegistrar *registrar = [GDLRegistrar sharedInstance];
+  GDLTestPrioritizer *prioritizer = [[GDLTestPrioritizer alloc] init];
+  XCTAssertNoThrow([registrar registerPrioritizer:prioritizer logTarget:self.logTarget]);
+  XCTAssertEqual(prioritizer, registrar.logTargetToPrioritizer[@(_logTarget)]);
+}
+
 @end

GoogleDataLogger/Tests/Unit/Helpers/GDLTestPrioritizer.h

@@ -31,6 +31,9 @@ NS_ASSUME_NONNULL_BEGIN
 /** Allows the running of a block of code during -prioritizeLog. */
 @property(nullable, nonatomic) void (^prioritizeLogBlock)(GDLLogEvent *logEvent);
 
+/** A block that can run before -logsForNextUpload completes. */
+@property(nullable, nonatomic) void (^logsForNextUploadBlock)(void);
+
 @end
 
 NS_ASSUME_NONNULL_END

GoogleDataLogger/Tests/Unit/Helpers/GDLTestPrioritizer.m

@@ -27,6 +27,9 @@ - (instancetype)init {
 }
 
 - (NSSet<NSNumber *> *)logsForNextUpload {
+  if (_logsForNextUploadBlock) {
+    _logsForNextUploadBlock();
+  }
   return _logsForNextUploadFake;
 }