GDTTransformer: fix background tasks handling (#6457)

* GDTCORTransformer: background task tests

* GDTCORTransformer: background tasks handling fix

* Copyright

* Changelog

* Comments

* rand() -> arc4random()

* GDTCORTransformer tests for default initializer

* Fix default initializer

* ./scripts/style.sh

* Pass weak application reference to the blocks

Author: maksymmalyhin

GoogleDataTransport/CHANGELOG.md

@@ -1,5 +1,6 @@
 # v7.4.0
 - Limit disk space consumed by GoogleDataTransport to store events. (#6365)
+- Fix `GDTTransformer` background task handling.  (#6258)
 
 # v7.1.1
 - Use `NSTimeZone` instead of `CFTimeZone` to get time zone offset respecting daylight. (#6246)

GoogleDataTransport/GDTCORLibrary/GDTCORTransformer.m

@@ -39,10 +39,15 @@ + (instancetype)sharedInstance {
 }
 
 - (instancetype)init {
+  return [self initWithApplication:[GDTCORApplication sharedApplication]];
+}
+
+- (instancetype)initWithApplication:(id<GDTCORApplicationProtocol>)application {
   self = [super init];
   if (self) {
     _eventWritingQueue =
         dispatch_queue_create("com.google.GDTCORTransformer", DISPATCH_QUEUE_SERIAL);
+    _application = application;
   }
   return self;
 }
@@ -51,45 +56,47 @@ - (void)transformEvent:(GDTCOREvent *)event
       withTransformers:(NSArray<id<GDTCOREventTransformer>> *)transformers
             onComplete:(void (^_Nullable)(BOOL wasWritten, NSError *_Nullable error))completion {
   GDTCORAssert(event, @"You can't write a nil event");
-  BOOL hadOriginalCompletion = completion != nil;
-  if (!completion) {
-    completion = ^(BOOL wasWritten, NSError *_Nullable error) {
-    };
-  }
 
   __block GDTCORBackgroundIdentifier bgID = GDTCORBackgroundIdentifierInvalid;
-  bgID = [[GDTCORApplication sharedApplication]
-      beginBackgroundTaskWithName:@"GDTTransformer"
-                expirationHandler:^{
-                  [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
-                  bgID = GDTCORBackgroundIdentifierInvalid;
-                }];
+  __auto_type __weak weakApplication = self.application;
+  bgID = [self.application beginBackgroundTaskWithName:@"GDTTransformer"
+                                     expirationHandler:^{
+                                       [weakApplication endBackgroundTask:bgID];
+                                       bgID = GDTCORBackgroundIdentifierInvalid;
+                                     }];
+
+  __auto_type completionWrapper = ^(BOOL wasWritten, NSError *_Nullable error) {
+    if (completion) {
+      completion(wasWritten, error);
+    }
+
+    // The work is done, cancel the background task if it's valid.
+    [weakApplication endBackgroundTask:bgID];
+    bgID = GDTCORBackgroundIdentifierInvalid;
+  };
+
   dispatch_async(_eventWritingQueue, ^{
     GDTCOREvent *transformedEvent = event;
     for (id<GDTCOREventTransformer> transformer in transformers) {
       if ([transformer respondsToSelector:@selector(transform:)]) {
         GDTCORLogDebug(@"Applying a transformer to event %@", event);
         transformedEvent = [transformer transform:transformedEvent];
         if (!transformedEvent) {
-          completion(NO, nil);
+          completionWrapper(NO, nil);
           return;
         }
       } else {
         GDTCORLogError(GDTCORMCETransformerDoesntImplementTransform,
                        @"Transformer doesn't implement transform: %@", transformer);
-        completion(NO, nil);
+        completionWrapper(NO, nil);
         return;
       }
     }
 
     id<GDTCORStorageProtocol> storage =
         [GDTCORRegistrar sharedInstance].targetToStorage[@(event.target)];
 
-    [storage storeEvent:transformedEvent onComplete:hadOriginalCompletion ? completion : nil];
-
-    // The work is done, cancel the background task if it's valid.
-    [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
-    bgID = GDTCORBackgroundIdentifierInvalid;
+    [storage storeEvent:transformedEvent onComplete:completionWrapper];
   });
 }
 

GoogleDataTransport/GDTCORLibrary/Private/GDTCORTransformer_Private.h

@@ -16,13 +16,22 @@
 
 #import "GoogleDataTransport/GDTCORLibrary/Private/GDTCORTransformer.h"
 
+@protocol GDTCORApplicationProtocol;
+
 NS_ASSUME_NONNULL_BEGIN
 
 @interface GDTCORTransformer ()
 
 /** The queue on which all work will occur. */
 @property(nonatomic) dispatch_queue_t eventWritingQueue;
 
+/** The application instance that is used to begin/end background tasks.  */
+@property(nonatomic, readonly) id<GDTCORApplicationProtocol> application;
+
+/** The internal initializer. Should be used in tests only to create an instance with a
+ * particular(fake) application instance. */
+- (instancetype)initWithApplication:(id<GDTCORApplicationProtocol>)application;
+
 @end
 
 NS_ASSUME_NONNULL_END

GoogleDataTransport/GDTCORLibrary/Public/GoogleDataTransport/GDTCORPlatform.h

@@ -186,18 +186,13 @@ FOUNDATION_EXPORT const GDTCORBackgroundIdentifier GDTCORBackgroundIdentifierInv
 
 @end
 
-/** A cross-platform application class. */
-@interface GDTCORApplication : NSObject <GDTCORApplicationDelegate>
+@protocol GDTCORApplicationProtocol <NSObject>
+
+@required
 
 /** Flag to determine if the application is running in the background. */
 @property(atomic, readonly) BOOL isRunningInBackground;
 
-/** Creates and/or returns the shared application instance.
- *
- * @return The shared application instance.
- */
-+ (nullable GDTCORApplication *)sharedApplication;
-
 /** Creates a background task with the returned identifier if on a suitable platform.
  *
  * @name name The name of the task, useful for debugging which background tasks are running.
@@ -216,4 +211,15 @@ FOUNDATION_EXPORT const GDTCORBackgroundIdentifier GDTCORBackgroundIdentifierInv
 
 @end
 
+/** A cross-platform application class. */
+@interface GDTCORApplication : NSObject <GDTCORApplicationProtocol, GDTCORApplicationDelegate>
+
+/** Creates and/or returns the shared application instance.
+ *
+ * @return The shared application instance.
+ */
++ (nullable GDTCORApplication *)sharedApplication;
+
+@end
+
 NS_ASSUME_NONNULL_END

GoogleDataTransport/GDTCORTests/Common/Fakes/GDTCORApplicationFake.h

@@ -0,0 +1,34 @@
+/*
+ * Copyright 2020 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#import <Foundation/Foundation.h>
+
+#import "GoogleDataTransport/GDTCORLibrary/Public/GoogleDataTransport/GDTCORPlatform.h"
+
+NS_ASSUME_NONNULL_BEGIN
+
+typedef GDTCORBackgroundIdentifier (^GDTCORFakeBeginBackgroundTaskHandler)(
+    NSString *name, dispatch_block_t handler);
+typedef void (^GDTCORFakeEndBackgroundTaskHandler)(GDTCORBackgroundIdentifier);
+
+@interface GDTCORApplicationFake : NSObject <GDTCORApplicationProtocol>
+
+@property(nonatomic, copy, nullable) GDTCORFakeBeginBackgroundTaskHandler beginTaskHandler;
+@property(nonatomic, copy, nullable) GDTCORFakeEndBackgroundTaskHandler endTaskHandler;
+
+@end
+
+NS_ASSUME_NONNULL_END

GoogleDataTransport/GDTCORTests/Common/Fakes/GDTCORApplicationFake.m

@@ -0,0 +1,32 @@
+/*
+ * Copyright 2020 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#import "GoogleDataTransport/GDTCORTests/Common/Fakes/GDTCORApplicationFake.h"
+
+@implementation GDTCORApplicationFake
+
+@synthesize isRunningInBackground;
+
+- (GDTCORBackgroundIdentifier)beginBackgroundTaskWithName:(NSString *)name
+                                        expirationHandler:(void (^__nullable)(void))handler {
+  return self.beginTaskHandler(name, handler);
+}
+
+- (void)endBackgroundTask:(GDTCORBackgroundIdentifier)bgID {
+  self.endTaskHandler(bgID);
+}
+
+@end

GoogleDataTransport/GDTCORTests/Unit/GDTCORTransformerTest.m

@@ -29,10 +29,10 @@
 
 #import "GoogleDataTransport/GDTCORTests/Common/Categories/GDTCORRegistrar+Testing.h"
 
+#import "GoogleDataTransport/GDTCORTests/Common/Fakes/GDTCORApplicationFake.h"
 #import "GoogleDataTransport/GDTCORTests/Common/Fakes/GDTCORStorageFake.h"
 
 @interface GDTCORTransformerTestNilingTransformer : NSObject <GDTCOREventTransformer>
-
 @end
 
 @implementation GDTCORTransformerTestNilingTransformer
@@ -44,7 +44,6 @@ - (GDTCOREvent *)transform:(GDTCOREvent *)eventEvent {
 @end
 
 @interface GDTCORTransformerTestNewEventTransformer : NSObject <GDTCOREventTransformer>
-
 @end
 
 @implementation GDTCORTransformerTestNewEventTransformer
@@ -57,50 +56,73 @@ - (GDTCOREvent *)transform:(GDTCOREvent *)eventEvent {
 
 @interface GDTCORTransformerTest : GDTCORTestCase
 
+@property(nonatomic) GDTCORApplicationFake *fakeApplication;
+@property(nonatomic) GDTCORTransformer *transformer;
+
 @end
 
 @implementation GDTCORTransformerTest
 
 - (void)setUp {
   [super setUp];
-  dispatch_sync([GDTCORTransformer sharedInstance].eventWritingQueue, ^{
+
+  self.fakeApplication = [[GDTCORApplicationFake alloc] init];
+
+  self.transformer = [[GDTCORTransformer alloc] initWithApplication:self.fakeApplication];
+  dispatch_sync(self.transformer.eventWritingQueue, ^{
     [[GDTCORRegistrar sharedInstance] registerStorage:[[GDTCORStorageFake alloc] init]
                                                target:kGDTCORTargetTest];
   });
 }
 
 - (void)tearDown {
   [super tearDown];
-  dispatch_sync([GDTCORTransformer sharedInstance].eventWritingQueue, ^{
+  dispatch_sync(self.transformer.eventWritingQueue, ^{
     [[GDTCORRegistrar sharedInstance] reset];
   });
+  self.transformer = nil;
+
+  self.fakeApplication.beginTaskHandler = nil;
+  self.fakeApplication = nil;
 }
 
 /** Tests the default initializer. */
 - (void)testInit {
-  XCTAssertNotNil([[GDTCORTransformer alloc] init]);
+  GDTCORTransformer *transformer = [[GDTCORTransformer alloc] init];
+  XCTAssertNotNil(transformer);
+  XCTAssertEqualObjects(transformer.application, [GDTCORApplication sharedApplication]);
 }
 
 /** Tests the pointer equality of result of the -sharedInstance method. */
 - (void)testSharedInstance {
   XCTAssertEqual([GDTCORTransformer sharedInstance], [GDTCORTransformer sharedInstance]);
+  XCTAssertEqualObjects([GDTCORTransformer sharedInstance].application,
+                        [GDTCORApplication sharedApplication]);
 }
 
 /** Tests writing a event without a transformer. */
 - (void)testWriteEventWithoutTransformers {
-  GDTCORTransformer *transformer = [GDTCORTransformer sharedInstance];
+  __auto_type bgTaskExpectations =
+      [self expectationsBackgroundTaskBeginAndEndWithName:@"GDTTransformer"];
+
+  GDTCORTransformer *transformer = self.transformer;
   GDTCOREvent *event = [[GDTCOREvent alloc] initWithMappingID:@"1" target:kGDTCORTargetTest];
   event.dataObject = [[GDTCORDataObjectTesterSimple alloc] init];
   XCTAssertNoThrow([transformer transformEvent:event
                               withTransformers:nil
                                     onComplete:^(BOOL wasWritten, NSError *_Nullable error) {
                                       XCTAssertTrue(wasWritten);
                                     }]);
+
+  [self waitForExpectations:bgTaskExpectations timeout:0.5];
 }
 
 /** Tests writing a event with a transformer that nils out the event. */
 - (void)testWriteEventWithTransformersThatNilTheEvent {
-  GDTCORTransformer *transformer = [GDTCORTransformer sharedInstance];
+  __auto_type bgTaskExpectations =
+      [self expectationsBackgroundTaskBeginAndEndWithName:@"GDTTransformer"];
+
+  GDTCORTransformer *transformer = self.transformer;
   GDTCOREvent *event = [[GDTCOREvent alloc] initWithMappingID:@"2" target:kGDTCORTargetTest];
   event.dataObject = [[GDTCORDataObjectTesterSimple alloc] init];
   NSArray<id<GDTCOREventTransformer>> *transformers =
@@ -110,11 +132,16 @@ - (void)testWriteEventWithTransformersThatNilTheEvent {
                                     onComplete:^(BOOL wasWritten, NSError *_Nullable error) {
                                       XCTAssertFalse(wasWritten);
                                     }]);
+
+  [self waitForExpectations:bgTaskExpectations timeout:0.5];
 }
 
 /** Tests writing a event with a transformer that creates a new event. */
 - (void)testWriteEventWithTransformersThatCreateANewEvent {
-  GDTCORTransformer *transformer = [GDTCORTransformer sharedInstance];
+  __auto_type bgTaskExpectations =
+      [self expectationsBackgroundTaskBeginAndEndWithName:@"GDTTransformer"];
+
+  GDTCORTransformer *transformer = self.transformer;
   GDTCOREvent *event = [[GDTCOREvent alloc] initWithMappingID:@"2" target:kGDTCORTargetTest];
   event.dataObject = [[GDTCORDataObjectTesterSimple alloc] init];
   NSArray<id<GDTCOREventTransformer>> *transformers =
@@ -125,6 +152,41 @@ - (void)testWriteEventWithTransformersThatCreateANewEvent {
                                       XCTAssertTrue(wasWritten);
                                       XCTAssertNil(error);
                                     }]);
+
+  [self waitForExpectations:bgTaskExpectations timeout:0.5];
+}
+
+#pragma mark - Helpers
+
+/** Sets  GDTCORApplicationFake handlers to expect the begin and the end of a background task with
+ * the specified name.
+ *  @return An array with the task begin and end XCTestExpectation.
+ */
+- (NSArray<XCTestExpectation *> *)expectationsBackgroundTaskBeginAndEndWithName:
+    (NSString *)expectedName {
+  XCTestExpectation *beginExpectation = [self expectationWithDescription:@"Background task begin"];
+  XCTestExpectation *endExpectation = [self expectationWithDescription:@"Background task end"];
+
+  GDTCORBackgroundIdentifier taskID = arc4random();
+
+  __auto_type __weak weakSelf = self;
+
+  self.fakeApplication.beginTaskHandler =
+      ^GDTCORBackgroundIdentifier(NSString *_Nonnull name, dispatch_block_t _Nonnull handler) {
+        __auto_type self = weakSelf;
+        XCTAssertEqualObjects(expectedName, name);
+
+        [beginExpectation fulfill];
+        return taskID;
+      };
+
+  self.fakeApplication.endTaskHandler = ^(GDTCORBackgroundIdentifier endTaskID) {
+    __auto_type self = weakSelf;
+    XCTAssert(endTaskID == taskID);
+    [endExpectation fulfill];
+  };
+
+  return @[ beginExpectation, endExpectation ];
 }
 
 @end