From 8a71ffde4bdb3c7dde8020614b091ecf6afd47e1 Mon Sep 17 00:00:00 2001 From: Michael Haney Date: Tue, 3 Sep 2019 16:12:28 -0700 Subject: [PATCH 1/4] Git rid of all NSAsserts in GDT and GDTCCT - Moves GDTAssert to be a public API - Always logs to console - Differentiates between fatal and non-fatal assertions - Messaging of assertions encourages creating an issue - Tests assertion use with NS_BLOCK_ASSERTIONS defined and undefined --- GoogleDataTransport.podspec | 6 ++ GoogleDataTransport/GDTLibrary/GDTAssert.m | 4 +- GoogleDataTransport/GDTLibrary/GDTEvent.m | 5 +- GoogleDataTransport/GDTLibrary/GDTPlatform.m | 6 +- GoogleDataTransport/GDTLibrary/GDTStorage.m | 6 +- .../GDTLibrary/GDTTransformer.m | 2 +- GoogleDataTransport/GDTLibrary/GDTTransport.m | 9 +- .../GDTLibrary/GDTUploadCoordinator.m | 2 +- .../GDTLibrary/Private/GDTAssert.h | 54 ----------- .../GDTLibrary/Public/GDTAssert.h | 91 +++++++++++++++++++ .../GDTLibrary/Public/GDTConsoleLogger.h | 7 +- .../AssertionsBlocked/GDTAssertBlockedTest.m | 37 ++++++++ .../Helpers/GDTIntegrationTestUploader.m | 36 ++++---- .../GDTTests/Unit/GDTAssertTest.m | 41 +++++++++ .../GDTTests/Unit/GDTEventTest.m | 2 +- .../GDTTests/Unit/GDTTransportTest.m | 2 +- .../GDTTests/Unit/Helpers/GDTAssertHelper.h | 6 +- .../Unit/Helpers/GDTCCTEventGenerator.m | 3 +- .../Unit/TestServer/GDTCCTTestServer.m | 8 +- 19 files changed, 235 insertions(+), 92 deletions(-) delete mode 100644 GoogleDataTransport/GDTLibrary/Private/GDTAssert.h create mode 100644 GoogleDataTransport/GDTLibrary/Public/GDTAssert.h create mode 100644 GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m create mode 100644 GoogleDataTransport/GDTTests/Unit/GDTAssertTest.m diff --git a/GoogleDataTransport.podspec b/GoogleDataTransport.podspec index f92c5e0fcf3..953b2f23b04 100644 --- a/GoogleDataTransport.podspec +++ b/GoogleDataTransport.podspec @@ -48,6 +48,12 @@ Shared library for iOS SDK data transport needs. test_spec.pod_target_xcconfig = header_search_paths end + s.test_spec 'Tests-Assertions-Blocked' do |test_spec| + test_spec.requires_app_host = false + test_spec.source_files = ['GoogleDataTransport/GDTTests/AssertionsBlocked/**/*.{h,m}'] + test_spec.pod_target_xcconfig = header_search_paths.merge({'GCC_PREPROCESSOR_DEFINITIONS' => 'NS_BLOCK_ASSERTIONS=1'}) + end + s.test_spec 'Tests-Lifecycle' do |test_spec| test_spec.requires_app_host = false test_spec.source_files = ['GoogleDataTransport/GDTTests/Lifecycle/**/*.{h,m}'] + common_test_sources diff --git a/GoogleDataTransport/GDTLibrary/GDTAssert.m b/GoogleDataTransport/GDTLibrary/GDTAssert.m index a6bedd2410b..106fce3a9b1 100644 --- a/GoogleDataTransport/GDTLibrary/GDTAssert.m +++ b/GoogleDataTransport/GDTLibrary/GDTAssert.m @@ -14,9 +14,9 @@ * limitations under the License. */ -#import "GDTLibrary/Private/GDTAssert.h" +#import "GDTLibrary/Public/GDTAssert.h" -GDTAssertionBlock GDTAssertionBlockToRunInsteadOfNSAssert(void) { +GDTAssertionBlock GDTAssertionBlockToRunInstead(void) { // This class is only compiled in by unit tests, and this should fail quickly in optimized builds. Class GDTAssertClass = NSClassFromString(@"GDTAssertHelper"); if (__builtin_expect(!!GDTAssertClass, 0)) { diff --git a/GoogleDataTransport/GDTLibrary/GDTEvent.m b/GoogleDataTransport/GDTLibrary/GDTEvent.m index 3ea9425d1cc..3d31ea5aec7 100644 --- a/GoogleDataTransport/GDTLibrary/GDTEvent.m +++ b/GoogleDataTransport/GDTLibrary/GDTEvent.m @@ -16,9 +16,9 @@ #import +#import #import -#import "GDTLibrary/Private/GDTAssert.h" #import "GDTLibrary/Private/GDTEvent_Private.h" @implementation GDTEvent @@ -26,6 +26,9 @@ @implementation GDTEvent - (instancetype)initWithMappingID:(NSString *)mappingID target:(NSInteger)target { GDTAssert(mappingID.length > 0, @"Please give a valid mapping ID"); GDTAssert(target > 0, @"A target cannot be negative or 0"); + if (mappingID == nil || mappingID.length == 0 || target <= 0) { + return nil; + } self = [super init]; if (self) { _mappingID = mappingID; diff --git a/GoogleDataTransport/GDTLibrary/GDTPlatform.m b/GoogleDataTransport/GDTLibrary/GDTPlatform.m index cd6cb4bd25e..122517db9e6 100644 --- a/GoogleDataTransport/GDTLibrary/GDTPlatform.m +++ b/GoogleDataTransport/GDTLibrary/GDTPlatform.m @@ -16,6 +16,8 @@ #import +#import + const GDTBackgroundIdentifier GDTBackgroundIdentifierInvalid = 0; NSString *const kGDTApplicationDidEnterBackgroundNotification = @@ -40,8 +42,8 @@ @implementation GDTApplication + (void)load { #if TARGET_OS_IOS || TARGET_OS_TV // If this asserts, please file a bug at https://github.com/firebase/firebase-ios-sdk/issues. - NSAssert(GDTBackgroundIdentifierInvalid == UIBackgroundTaskInvalid, - @"GDTBackgroundIdentifierInvalid and UIBackgroundTaskInvalid should be the same."); + GDTFatalAssert(GDTBackgroundIdentifierInvalid == UIBackgroundTaskInvalid, + @"GDTBackgroundIdentifierInvalid and UIBackgroundTaskInvalid should be the same."); #endif [self sharedApplication]; } diff --git a/GoogleDataTransport/GDTLibrary/GDTStorage.m b/GoogleDataTransport/GDTLibrary/GDTStorage.m index f5c77649d39..df3b9e68147 100644 --- a/GoogleDataTransport/GDTLibrary/GDTStorage.m +++ b/GoogleDataTransport/GDTLibrary/GDTStorage.m @@ -17,12 +17,12 @@ #import "GDTLibrary/Private/GDTStorage.h" #import "GDTLibrary/Private/GDTStorage_Private.h" +#import #import #import #import #import -#import "GDTLibrary/Private/GDTAssert.h" #import "GDTLibrary/Private/GDTEvent_Private.h" #import "GDTLibrary/Private/GDTRegistrar_Private.h" #import "GDTLibrary/Private/GDTUploadCoordinator.h" @@ -86,6 +86,10 @@ - (void)storeEvent:(GDTEvent *)event { }]; } + if (event == nil) { + return; + } + dispatch_async(_storageQueue, ^{ // Check that a backend implementation is available for this target. NSInteger target = event.target; diff --git a/GoogleDataTransport/GDTLibrary/GDTTransformer.m b/GoogleDataTransport/GDTLibrary/GDTTransformer.m index e1619f478a6..ffed57ecfba 100644 --- a/GoogleDataTransport/GDTLibrary/GDTTransformer.m +++ b/GoogleDataTransport/GDTLibrary/GDTTransformer.m @@ -17,11 +17,11 @@ #import "GDTLibrary/Private/GDTTransformer.h" #import "GDTLibrary/Private/GDTTransformer_Private.h" +#import #import #import #import -#import "GDTLibrary/Private/GDTAssert.h" #import "GDTLibrary/Private/GDTStorage.h" @implementation GDTTransformer diff --git a/GoogleDataTransport/GDTLibrary/GDTTransport.m b/GoogleDataTransport/GDTLibrary/GDTTransport.m index a1696c7562d..a34f15a3cc8 100644 --- a/GoogleDataTransport/GDTLibrary/GDTTransport.m +++ b/GoogleDataTransport/GDTLibrary/GDTTransport.m @@ -17,10 +17,10 @@ #import #import "GDTLibrary/Private/GDTTransport_Private.h" +#import #import #import -#import "GDTLibrary/Private/GDTAssert.h" #import "GDTLibrary/Private/GDTTransformer.h" @implementation GDTTransport @@ -28,10 +28,13 @@ @implementation GDTTransport - (instancetype)initWithMappingID:(NSString *)mappingID transformers:(nullable NSArray> *)transformers target:(NSInteger)target { + GDTAssert(mappingID.length > 0, @"A mapping ID cannot be nil or empty"); + GDTAssert(target > 0, @"A target cannot be negative or 0"); + if (mappingID == nil || mappingID.length == 0 || target <= 0) { + return nil; + } self = [super init]; if (self) { - GDTAssert(mappingID.length > 0, @"A mapping ID cannot be nil or empty"); - GDTAssert(target > 0, @"A target cannot be negative or 0"); _mappingID = mappingID; _transformers = transformers; _target = target; diff --git a/GoogleDataTransport/GDTLibrary/GDTUploadCoordinator.m b/GoogleDataTransport/GDTLibrary/GDTUploadCoordinator.m index b5e98a69576..e384c5dec0c 100644 --- a/GoogleDataTransport/GDTLibrary/GDTUploadCoordinator.m +++ b/GoogleDataTransport/GDTLibrary/GDTUploadCoordinator.m @@ -16,10 +16,10 @@ #import "GDTLibrary/Private/GDTUploadCoordinator.h" +#import #import #import -#import "GDTLibrary/Private/GDTAssert.h" #import "GDTLibrary/Private/GDTReachability.h" #import "GDTLibrary/Private/GDTRegistrar_Private.h" #import "GDTLibrary/Private/GDTStorage.h" diff --git a/GoogleDataTransport/GDTLibrary/Private/GDTAssert.h b/GoogleDataTransport/GDTLibrary/Private/GDTAssert.h deleted file mode 100644 index 4f157ed8625..00000000000 --- a/GoogleDataTransport/GDTLibrary/Private/GDTAssert.h +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright 2019 Google - * - * 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 - -/** A block type that could be run instead of NSAssert. No return type, no params. */ -typedef void (^GDTAssertionBlock)(void); - -/** Returns the result of executing a soft-linked method present in unit tests that allows a block - * to be run in lieu of a call to NSAssert. This helps ameliorate issues with catching exceptions - * that occur on a dispatch_queue. - * - * @return A block that can be run instead of calling NSAssert, or nil. - */ -FOUNDATION_EXPORT GDTAssertionBlock _Nullable GDTAssertionBlockToRunInsteadOfNSAssert(void); - -#if !defined(NS_BLOCK_ASSERTIONS) - -/** Asserts using NSAssert, unless a block was specified to be run instead. - * - * @param condition The condition you'd expect to be YES. - */ -#define GDTAssert(condition, ...) \ - do { \ - if (__builtin_expect(!(condition), 0)) { \ - GDTAssertionBlock assertionBlock = GDTAssertionBlockToRunInsteadOfNSAssert(); \ - if (assertionBlock) { \ - assertionBlock(); \ - } else { \ - NSAssert(condition, __VA_ARGS__); \ - } \ - } \ - } while (0); - -#else - -#define GDTAssert(condition, ...) \ - do { \ - } while (0); - -#endif // !defined(NS_BLOCK_ASSERTIONS) diff --git a/GoogleDataTransport/GDTLibrary/Public/GDTAssert.h b/GoogleDataTransport/GDTLibrary/Public/GDTAssert.h new file mode 100644 index 00000000000..a00428d453b --- /dev/null +++ b/GoogleDataTransport/GDTLibrary/Public/GDTAssert.h @@ -0,0 +1,91 @@ +/* + * Copyright 2019 Google + * + * 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 + +#import + +/** A block type that could be run instead of normal assertion logging. No return type, no params. + */ +typedef void (^GDTAssertionBlock)(void); + +/** Returns the result of executing a soft-linked method present in unit tests that allows a block + * to be run instead of normal assertion logging. This helps ameliorate issues with catching + * exceptions that occur on a dispatch_queue. + * + * @return A block that can be run instead of normal assert printing. + */ +FOUNDATION_EXPORT GDTAssertionBlock _Nullable GDTAssertionBlockToRunInstead(void); + +#if defined(NS_BLOCK_ASSERTIONS) + +#define GDTAssert(condition, ...) \ + do { \ + } while (0); + +#define GDTFatalAssert(condition, ...) \ + do { \ + } while (0); + +#else // defined(NS_BLOCK_ASSERTIONS) + +/** Asserts using a console log, unless a block was specified to be run instead. + * + * @param condition The condition you'd expect to be YES. + */ +#define GDTAssert(condition, ...) \ + do { \ + if (__builtin_expect(!(condition), 0)) { \ + GDTAssertionBlock assertionBlock = GDTAssertionBlockToRunInstead(); \ + if (assertionBlock) { \ + assertionBlock(); \ + } else { \ + __PRAGMA_PUSH_NO_EXTRA_ARG_WARNINGS \ + NSString *__assert_file__ = [NSString stringWithUTF8String:__FILE__]; \ + __assert_file__ = __assert_file__ ? __assert_file__ : @""; \ + GDTLogError(GDTMCEGeneralError, @"Assertion failed (%@:%d): %s,", __assert_file__, \ + __LINE__, ##__VA_ARGS__); \ + __PRAGMA_POP_NO_EXTRA_ARG_WARNINGS \ + } \ + } \ + } while (0); + +/** Asserts by logging to the console and throwing an exception if NS_BLOCK_ASSERTIONS is not + * defined. + * + * @param condition The condition you'd expect to be YES. + */ +#define GDTFatalAssert(condition, ...) \ + do { \ + __PRAGMA_PUSH_NO_EXTRA_ARG_WARNINGS \ + if (__builtin_expect(!(condition), 0)) { \ + NSString *__assert_file__ = [NSString stringWithUTF8String:__FILE__]; \ + __assert_file__ = __assert_file__ ? __assert_file__ : @""; \ + GDTLogError(GDTMCEFatalAssertion, \ + @"Fatal assertion encountered, please open an issue at " \ + "https://github.com/firebase/firebase-ios-sdk/issues " \ + "(%@:%d): %s,", \ + __assert_file__, __LINE__, ##__VA_ARGS__); \ + [[NSAssertionHandler currentHandler] handleFailureInMethod:_cmd \ + object:self \ + file:__assert_file__ \ + lineNumber:__LINE__ \ + description:@"%@", ##__VA_ARGS__]; \ + } \ + __PRAGMA_POP_NO_EXTRA_ARG_WARNINGS \ + } while (0); + +#endif // defined(NS_BLOCK_ASSERTIONS) diff --git a/GoogleDataTransport/GDTLibrary/Public/GDTConsoleLogger.h b/GoogleDataTransport/GDTLibrary/Public/GDTConsoleLogger.h index 6035f3da76e..0988d345ef9 100644 --- a/GoogleDataTransport/GDTLibrary/Public/GDTConsoleLogger.h +++ b/GoogleDataTransport/GDTLibrary/Public/GDTConsoleLogger.h @@ -56,7 +56,12 @@ typedef NS_ENUM(NSInteger, GDTMessageCode) { GDTMCETransportBytesError = 1005, /** For general purpose error messages in a dependency. */ - GDTMCEGeneralError = 1006 + GDTMCEGeneralError = 1006, + + /** For fatal errors. Please go to https://github.com/firebase/firebase-ios-sdk/issues and open + * an issue if you encounter an error with this code. + */ + GDTMCEFatalAssertion = 1007 }; /** */ diff --git a/GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m b/GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m new file mode 100644 index 00000000000..2a82fce5c5d --- /dev/null +++ b/GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m @@ -0,0 +1,37 @@ +/* + * Copyright 2019 Google + * + * 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 + +#import + +@interface GDTAssertBlockedTest : XCTestCase + +@end + +@implementation GDTAssertBlockedTest + +/** Tests that asserting is innocuous and doesn't throw. NS_BLOCK_ASSERTIONS doesn't matter here. */ +- (void)testNonFatallyAssertingDoesntThrow { + GDTAssert(NO, @"test assertion"); +} + +/** Tests that fatally asserting doesn't throw with NS_BLOCK_ASSERTIONS defined. */ +- (void)testFatallyAssertingDoesntThrow { + GDTFatalAssert(NO, @"test assertion"); +} + +@end diff --git a/GoogleDataTransport/GDTTests/Integration/Helpers/GDTIntegrationTestUploader.m b/GoogleDataTransport/GDTTests/Integration/Helpers/GDTIntegrationTestUploader.m index 279ac2aadde..d60645f4a86 100644 --- a/GoogleDataTransport/GDTTests/Integration/Helpers/GDTIntegrationTestUploader.m +++ b/GoogleDataTransport/GDTTests/Integration/Helpers/GDTIntegrationTestUploader.m @@ -16,6 +16,7 @@ #import "GDTTests/Integration/Helpers/GDTIntegrationTestUploader.h" +#import #import #import @@ -41,7 +42,8 @@ - (instancetype)initWithServerURL:(NSURL *)serverURL { } - (void)uploadPackage:(GDTUploadPackage *)package { - NSAssert(!_currentUploadTask, @"An upload shouldn't be initiated with another in progress."); + GDTFatalAssert(!_currentUploadTask, + @"An upload shouldn't be initiated with another in progress."); NSURL *serverURL = arc4random_uniform(2) ? [_serverURL URLByAppendingPathComponent:@"log"] : [_serverURL URLByAppendingPathComponent:@"logBatch"]; NSURLSession *session = [NSURLSession sharedSession]; @@ -54,24 +56,24 @@ - (void)uploadPackage:(GDTUploadPackage *)package { // In real usage, you'd create an instance of whatever request proto your server needs. for (GDTStoredEvent *event in package.events) { NSData *fileData = [NSData dataWithContentsOfURL:event.dataFuture.fileURL]; - NSAssert(fileData, @"An event file shouldn't be empty"); + GDTFatalAssert(fileData, @"An event file shouldn't be empty"); [uploadData appendData:fileData]; } - _currentUploadTask = - [session uploadTaskWithRequest:request - fromData:uploadData - completionHandler:^(NSData *_Nullable data, NSURLResponse *_Nullable response, - NSError *_Nullable error) { - NSLog(@"Batch upload complete."); - // Remove from the prioritizer if there were no errors. - NSAssert(!error, @"There should be no errors uploading events: %@", error); - if (error) { - [package retryDeliveryInTheFuture]; - } else { - [package completeDelivery]; - } - self->_currentUploadTask = nil; - }]; + _currentUploadTask = [session + uploadTaskWithRequest:request + fromData:uploadData + completionHandler:^(NSData *_Nullable data, NSURLResponse *_Nullable response, + NSError *_Nullable error) { + NSLog(@"Batch upload complete."); + // Remove from the prioritizer if there were no errors. + GDTFatalAssert(!error, @"There should be no errors uploading events: %@", error); + if (error) { + [package retryDeliveryInTheFuture]; + } else { + [package completeDelivery]; + } + self->_currentUploadTask = nil; + }]; [_currentUploadTask resume]; } diff --git a/GoogleDataTransport/GDTTests/Unit/GDTAssertTest.m b/GoogleDataTransport/GDTTests/Unit/GDTAssertTest.m new file mode 100644 index 00000000000..6c48dde9a3f --- /dev/null +++ b/GoogleDataTransport/GDTTests/Unit/GDTAssertTest.m @@ -0,0 +1,41 @@ +/* + * Copyright 2019 Google + * + * 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. + */ + +#undef NS_BLOCK_ASSERTIONS + +#import + +#import + +@interface GDTAssertNotBlockedTest : XCTestCase + +@end + +@implementation GDTAssertNotBlockedTest + +/** Tests that asserting is innocuous and doesn't throw. */ +- (void)testNonFatallyAssertingDoesntThrow { + GDTAssert(NO, @"test assertion"); +} + +/** Tests that fatally asserting throws. */ +- (void)testFatallyAssertingThrows { + void (^assertionBlock)(void) = ^{ + GDTFatalAssert(NO, @"test assertion") + }; + XCTAssertThrows(assertionBlock()); +} +@end diff --git a/GoogleDataTransport/GDTTests/Unit/GDTEventTest.m b/GoogleDataTransport/GDTTests/Unit/GDTEventTest.m index 3609e3aa50d..43ecbe8a6c9 100644 --- a/GoogleDataTransport/GDTTests/Unit/GDTEventTest.m +++ b/GoogleDataTransport/GDTTests/Unit/GDTEventTest.m @@ -29,7 +29,7 @@ @implementation GDTEventTest /** Tests the designated initializer. */ - (void)testInit { XCTAssertNotNil([[GDTEvent alloc] initWithMappingID:@"1" target:1]); - XCTAssertThrows([[GDTEvent alloc] initWithMappingID:@"" target:1]); + XCTAssertNil([[GDTEvent alloc] initWithMappingID:@"" target:1]); } /** Tests NSKeyedArchiver encoding and decoding. */ diff --git a/GoogleDataTransport/GDTTests/Unit/GDTTransportTest.m b/GoogleDataTransport/GDTTests/Unit/GDTTransportTest.m index 96aa99586f7..68a438c98c3 100644 --- a/GoogleDataTransport/GDTTests/Unit/GDTTransportTest.m +++ b/GoogleDataTransport/GDTTests/Unit/GDTTransportTest.m @@ -33,7 +33,7 @@ @implementation GDTTransportTest /** Tests the default initializer. */ - (void)testInit { XCTAssertNotNil([[GDTTransport alloc] initWithMappingID:@"1" transformers:nil target:1]); - XCTAssertThrows([[GDTTransport alloc] initWithMappingID:@"" transformers:nil target:1]); + XCTAssertNil([[GDTTransport alloc] initWithMappingID:@"" transformers:nil target:1]); } /** Tests sending a telemetry event. */ diff --git a/GoogleDataTransport/GDTTests/Unit/Helpers/GDTAssertHelper.h b/GoogleDataTransport/GDTTests/Unit/Helpers/GDTAssertHelper.h index 372d4c146ad..d37c6fa9330 100644 --- a/GoogleDataTransport/GDTTests/Unit/Helpers/GDTAssertHelper.h +++ b/GoogleDataTransport/GDTTests/Unit/Helpers/GDTAssertHelper.h @@ -16,14 +16,14 @@ #import -#import "GDTLibrary/Private/GDTAssert.h" +#import "GDTLibrary/Public/GDTAssert.h" NS_ASSUME_NONNULL_BEGIN -/** Allows the setting a block to be used in the GDTAssert macro instead of a call to NSAssert. */ +/** Allows the setting a block to be used in the GDTAssert macro instead of assertion log. */ @interface GDTAssertHelper : NSObject -/** A class property that can be run instead of NSAssert. */ +/** A class property that can be run instead of normal assertion logging. */ @property(class, nullable, nonatomic) GDTAssertionBlock assertionBlock; @end diff --git a/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/Helpers/GDTCCTEventGenerator.m b/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/Helpers/GDTCCTEventGenerator.m index 7433eb42d2a..6f013465f4a 100644 --- a/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/Helpers/GDTCCTEventGenerator.m +++ b/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/Helpers/GDTCCTEventGenerator.m @@ -16,6 +16,7 @@ #import "GDTCCTTests/Unit/Helpers/GDTCCTEventGenerator.h" +#import #import @implementation GDTCCTEventGenerator @@ -27,7 +28,7 @@ - (void)deleteGeneratedFilesFromDisk { for (GDTStoredEvent *storedEvent in self.allGeneratedEvents) { NSError *error; [[NSFileManager defaultManager] removeItemAtURL:storedEvent.dataFuture.fileURL error:&error]; - NSAssert(error == nil, @"There was an error deleting a temporary event file."); + GDTAssert(error == nil, @"There was an error deleting a temporary event file."); } } diff --git a/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m b/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m index abb81818793..1991013f53d 100644 --- a/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m +++ b/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m @@ -16,6 +16,8 @@ #import "GDTCCTTests/Unit/TestServer/GDTCCTTestServer.h" +#import + #import #import @@ -50,16 +52,16 @@ - (void)dealloc { } - (void)start { - NSAssert(self.server.isRunning == NO, @"The server should not be already running."); + GDTAssert(self.server.isRunning == NO, @"The server should not be already running."); NSError *error; [self.server startWithOptions:@{GCDWebServerOption_Port : @0, GCDWebServerOption_BindToLocalhost : @YES} error:&error]; - NSAssert(error == nil, @"Error when starting server: %@", error); + GDTAssert(error == nil, @"Error when starting server: %@", error); } - (void)stop { - NSAssert(self.server.isRunning, @"The server should be running before stopping."); + GDTAssert(self.server.isRunning, @"The server should be running before stopping."); [self.server stop]; } From 5df9a45611b4ea63d0d15e962f02c28636e74941 Mon Sep 17 00:00:00 2001 From: Michael Haney Date: Tue, 3 Sep 2019 16:20:09 -0700 Subject: [PATCH 2/4] Update CHANGELOGs and remove one last NSCAssert --- GoogleDataTransport/CHANGELOG.md | 3 +++ GoogleDataTransportCCTSupport/CHANGELOG.md | 6 ++++++ .../GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/GoogleDataTransport/CHANGELOG.md b/GoogleDataTransport/CHANGELOG.md index b4f6e564a12..d82338adc57 100644 --- a/GoogleDataTransport/CHANGELOG.md +++ b/GoogleDataTransport/CHANGELOG.md @@ -1,3 +1,6 @@ +# v1.2.0 +- Removes all NSAsserts in favor of custom asserts. (#3747) + # v1.1.3 - Wrap decoding in GDTUploadCoordinator in a try catch. (#3676) diff --git a/GoogleDataTransportCCTSupport/CHANGELOG.md b/GoogleDataTransportCCTSupport/CHANGELOG.md index eb68ade0709..ddb6df57ab5 100644 --- a/GoogleDataTransportCCTSupport/CHANGELOG.md +++ b/GoogleDataTransportCCTSupport/CHANGELOG.md @@ -1,3 +1,9 @@ +# v1.0.3 +- Remove all NSAsserts in favor of GDTAssert. + +# v1.0.2 +- More safely handle backgrounding. + # v1.0.1 - Removed unused fields from firebasecore.proto. diff --git a/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m b/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m index 1991013f53d..24446926e08 100644 --- a/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m +++ b/GoogleDataTransportCCTSupport/GDTCCTTests/Unit/TestServer/GDTCCTTestServer.m @@ -87,7 +87,7 @@ - (NSData *)responseData { pb_ostream_t sizestream = PB_OSTREAM_SIZING; // Encode 1 time to determine the size. if (!pb_encode(&sizestream, gdt_cct_LogResponse_fields, &logResponse)) { - NSCAssert(NO, @"Error in nanopb encoding for size: %s", PB_GET_ERROR(&sizestream)); + GDTAssert(NO, @"Error in nanopb encoding for size: %s", PB_GET_ERROR(&sizestream)); } // Encode a 2nd time to actually get the bytes from it. @@ -95,7 +95,7 @@ - (NSData *)responseData { CFMutableDataRef dataRef = CFDataCreateMutable(CFAllocatorGetDefault(), bufferSize); pb_ostream_t ostream = pb_ostream_from_buffer((void *)CFDataGetBytePtr(dataRef), bufferSize); if (!pb_encode(&sizestream, gdt_cct_LogResponse_fields, &logResponse)) { - NSCAssert(NO, @"Error in nanopb encoding for bytes: %s", PB_GET_ERROR(&ostream)); + GDTAssert(NO, @"Error in nanopb encoding for bytes: %s", PB_GET_ERROR(&ostream)); } CFDataSetLength(dataRef, ostream.bytes_written); pb_release(gdt_cct_LogResponse_fields, &logResponse); From fde68ed819d6bf1e07aeb386472af5374153c65b Mon Sep 17 00:00:00 2001 From: Michael Haney Date: Tue, 3 Sep 2019 19:01:52 -0700 Subject: [PATCH 3/4] Remove a test because xcconfigs aren't respected --- GoogleDataTransport.podspec | 6 --- .../AssertionsBlocked/GDTAssertBlockedTest.m | 37 ------------------- 2 files changed, 43 deletions(-) delete mode 100644 GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m diff --git a/GoogleDataTransport.podspec b/GoogleDataTransport.podspec index 953b2f23b04..f92c5e0fcf3 100644 --- a/GoogleDataTransport.podspec +++ b/GoogleDataTransport.podspec @@ -48,12 +48,6 @@ Shared library for iOS SDK data transport needs. test_spec.pod_target_xcconfig = header_search_paths end - s.test_spec 'Tests-Assertions-Blocked' do |test_spec| - test_spec.requires_app_host = false - test_spec.source_files = ['GoogleDataTransport/GDTTests/AssertionsBlocked/**/*.{h,m}'] - test_spec.pod_target_xcconfig = header_search_paths.merge({'GCC_PREPROCESSOR_DEFINITIONS' => 'NS_BLOCK_ASSERTIONS=1'}) - end - s.test_spec 'Tests-Lifecycle' do |test_spec| test_spec.requires_app_host = false test_spec.source_files = ['GoogleDataTransport/GDTTests/Lifecycle/**/*.{h,m}'] + common_test_sources diff --git a/GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m b/GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m deleted file mode 100644 index 2a82fce5c5d..00000000000 --- a/GoogleDataTransport/GDTTests/AssertionsBlocked/GDTAssertBlockedTest.m +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright 2019 Google - * - * 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 - -#import - -@interface GDTAssertBlockedTest : XCTestCase - -@end - -@implementation GDTAssertBlockedTest - -/** Tests that asserting is innocuous and doesn't throw. NS_BLOCK_ASSERTIONS doesn't matter here. */ -- (void)testNonFatallyAssertingDoesntThrow { - GDTAssert(NO, @"test assertion"); -} - -/** Tests that fatally asserting doesn't throw with NS_BLOCK_ASSERTIONS defined. */ -- (void)testFatallyAssertingDoesntThrow { - GDTFatalAssert(NO, @"test assertion"); -} - -@end From 6eec8a994361704fb972fd26c7ce8ae23c927399 Mon Sep 17 00:00:00 2001 From: Michael Haney Date: Tue, 3 Sep 2019 20:20:57 -0700 Subject: [PATCH 4/4] Address nit --- GoogleDataTransport/GDTLibrary/GDTStorage.m | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/GoogleDataTransport/GDTLibrary/GDTStorage.m b/GoogleDataTransport/GDTLibrary/GDTStorage.m index df3b9e68147..853bbb615b8 100644 --- a/GoogleDataTransport/GDTLibrary/GDTStorage.m +++ b/GoogleDataTransport/GDTLibrary/GDTStorage.m @@ -74,6 +74,10 @@ - (instancetype)init { } - (void)storeEvent:(GDTEvent *)event { + if (event == nil) { + return; + } + [self createEventDirectoryIfNotExists]; __block GDTBackgroundIdentifier bgID = GDTBackgroundIdentifierInvalid; @@ -86,10 +90,6 @@ - (void)storeEvent:(GDTEvent *)event { }]; } - if (event == nil) { - return; - } - dispatch_async(_storageQueue, ^{ // Check that a backend implementation is available for this target. NSInteger target = event.target;