Fixing minor issues and doing some minor refactoring/clean-up (#444)

* Avoid NPE when appBundlePath is nil in BPConfiguration * Minor logging changes * Better error handling
MobileNativeFoundation · Jun 12, 2020 · 91ecdd6 · 91ecdd6
1 parent 4e11c74
commit 91ecdd6
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 21 deletions.
diff --git a/bluepill/src/BPApp.m b/bluepill/src/BPApp.m
@@ -106,6 +106,8 @@ + (instancetype)appWithConfig:(BPConfiguration *)config
         for (NSString *testName in config.tests) {
             BPTestPlan *testPlan = [config.tests objectForKey:testName];
             BPXCTestFile *xcTestFile = [BPXCTestFile BPXCTestFileFromBPTestPlan:testPlan withName:testName andError:errPtr];
+            if (*errPtr)
+                return nil;
             [loadedTests addObject:xcTestFile];
         }
 

diff --git a/bp/src/BPConfiguration.m b/bp/src/BPConfiguration.m
@@ -710,23 +710,25 @@ - (BOOL)validateConfigWithError:(NSError *__autoreleasing *)errPtr {
         return NO;
     }
 
-    if (self.appBundlePath && (![[NSFileManager defaultManager] fileExistsAtPath: self.appBundlePath isDirectory:&isdir] || !isdir)) {
-        BP_SET_ERROR(errPtr, @"%@ not found.", self.appBundlePath);
-        return NO;
-    }
-    NSString *path = [self.appBundlePath stringByAppendingPathComponent:@"Info.plist"];
-    NSDictionary *dic = [NSDictionary dictionaryWithContentsOfFile:path];
+    if (self.appBundlePath) {
+        if (![[NSFileManager defaultManager] fileExistsAtPath: self.appBundlePath isDirectory:&isdir] || !isdir) {
+            BP_SET_ERROR(errPtr, @"%@ not found.", self.appBundlePath);
+            return NO;
+        }
+        NSString *path = [self.appBundlePath stringByAppendingPathComponent:@"Info.plist"];
+        NSDictionary *dic = [NSDictionary dictionaryWithContentsOfFile:path];
 
-    if (!dic) {
-        BP_SET_ERROR(errPtr, @"Could not read %@, perhaps you forgot to run xcodebuild build-for-testing?", path);
-    }
+        if (!dic) {
+            BP_SET_ERROR(errPtr, @"Could not read %@, perhaps you forgot to run xcodebuild build-for-testing?", path);
+        }
 
-    NSString *platform = [dic objectForKey:@"DTPlatformName"];
-    if (platform && ![platform isEqualToString:@"iphonesimulator"]) {
-        BP_SET_ERROR(errPtr, @"Wrong platform in %@. Expected 'iphonesimulator', found '%@'", self.appBundlePath, platform);
-        return NO;
+        NSString *platform = [dic objectForKey:@"DTPlatformName"];
+        if (platform && ![platform isEqualToString:@"iphonesimulator"]) {
+            BP_SET_ERROR(errPtr, @"Wrong platform in %@. Expected 'iphonesimulator', found '%@'", self.appBundlePath, platform);
+            return NO;
+        }
     }
-
+    
     if (self.outputDirectory) {
         if ([[NSFileManager defaultManager] fileExistsAtPath:self.outputDirectory isDirectory:&isdir]) {
             if (!isdir) {

diff --git a/bp/src/Bluepill.m b/bp/src/Bluepill.m
@@ -121,6 +121,7 @@ - (void)retry {
     if (![self canRetryOnError] || self.failureTolerance <= 0) {
         // If there is no more retries, set the final exitCode to current context's exitCode
         self.finalExitStatus |= self.context.finalExitStatus;
+        [BPUtils printInfo:ERROR withString:@"No retries left. Giving up."];
         [BPUtils printInfo:INFO withString:@"%s:%d finalExitStatus = %@", __FILE__, __LINE__, [BPExitStatusHelper stringFromExitStatus:self.finalExitStatus]];
         self.exitLoop = YES;
         return;
@@ -158,9 +159,9 @@ - (void)recover {
     // If error retry reach to the max, then return
     if (![self canRetryOnError]) {
         self.finalExitStatus |= self.context.finalExitStatus;
+        [BPUtils printInfo:ERROR withString:@"No retries left. Giving up."];
         [BPUtils printInfo:INFO withString:@"%s:%d finalExitStatus = %@", __FILE__, __LINE__, [BPExitStatusHelper stringFromExitStatus:self.finalExitStatus]];
         self.exitLoop = YES;
-        [BPUtils printInfo:ERROR withString:@"Too many retries have occurred. Giving up."];
         return;
     }
 
@@ -186,9 +187,9 @@ - (void)recover {
 - (void)proceed {
     if (![self canRetryOnError]) {
         self.finalExitStatus |= self.context.finalExitStatus;
+        [BPUtils printInfo:ERROR withString:@"No retries left. Giving up."];
         [BPUtils printInfo:INFO withString:@"%s:%d finalExitStatus = %@", __FILE__, __LINE__, [BPExitStatusHelper stringFromExitStatus:self.finalExitStatus]];
         self.exitLoop = YES;
-        [BPUtils printInfo:ERROR withString:@"Too many retries have occurred. Giving up."];
         return;
     }
     self.retries += 1;
@@ -591,7 +592,9 @@ - (void)deleteSimulatorOnlyTaskWithContext:(BPExecutionContext *)context {
  */
 - (void)finishWithContext:(BPExecutionContext *)context {
     context.finalExitStatus |= context.exitStatus;
-    [BPUtils printInfo:INFO withString:@"Attempt's Exit Status: %@", [BPExitStatusHelper stringFromExitStatus:context.exitStatus]];
+    [BPUtils printInfo:INFO withString:@"Attempt's Exit Status: %@, Bundle exit status: %@",
+     [BPExitStatusHelper stringFromExitStatus:context.exitStatus],
+     [BPExitStatusHelper stringFromExitStatus:context.finalExitStatus]];
 
     switch (context.exitStatus) {
         // BP exit handler
@@ -628,19 +631,24 @@ - (void)finishWithContext:(BPExecutionContext *)context {
             return;
 
         case BPExitStatusAppCrashed:
-            // Remember the app crash and report whether a retry passes or not
+            // Crashed test is considered fatal and shall not be retried
             self.finalExitStatus |= context.exitStatus;
             NEXT([self proceed]);
             return;
 
         case BPExitStatusSimulatorDeleted:
         case BPExitStatusSimulatorReuseFailed:
             self.finalExitStatus |= context.finalExitStatus;
-            [BPUtils printInfo:INFO withString:@"%s:%d finalExitStatus = %@", __FILE__, __LINE__, [BPExitStatusHelper stringFromExitStatus:self.finalExitStatus]];
+            [BPUtils printInfo:INFO withString:@"%s:%d finalExitStatus = %@",
+             __FILE__, __LINE__,
+             [BPExitStatusHelper stringFromExitStatus:self.finalExitStatus]];
             self.exitLoop = YES;
             return;
     }
-    [BPUtils printInfo:ERROR withString:@"%s:%d YOU SHOULDN'T BE HERE. exitStatus = %@, finalExitStatus = %@", __FILE__, __LINE__, [BPExitStatusHelper stringFromExitStatus:context.exitStatus], [BPExitStatusHelper stringFromExitStatus:context.finalExitStatus]];
+    [BPUtils printInfo:ERROR withString:@"%s:%d YOU SHOULDN'T BE HERE. exitStatus = %@, finalExitStatus = %@",
+     __FILE__, __LINE__,
+     [BPExitStatusHelper stringFromExitStatus:context.exitStatus],
+     [BPExitStatusHelper stringFromExitStatus:context.finalExitStatus]];
 }
 
 // MARK: Helpers
@@ -666,7 +674,9 @@ - (BOOL)canRetryOnError {
     if (self.retries > maxErrorRetryCount) {
         // If retries strictly exceeds the max error retry, then we must have incremented it beyond the limit somehow.
         // It is safe to halt retries here, but log to alert unexpected behavior.
-        [BPUtils printInfo:ERROR withString:@"Current retry count (%d) exceeded maximum retry count (%d)!", (int) self.retries, (int) maxErrorRetryCount];
+        [BPUtils printInfo:ERROR withString:@"Current retry count (%d) exceeded maximum retry count (%d)!",
+         (int) self.retries,
+         (int) maxErrorRetryCount];
     }
     return false;
 }

diff --git a/bp/tests/Resource Files/testConfig-busted.json b/bp/tests/Resource Files/testConfig-busted.json
@@ -4,6 +4,8 @@
                  "VoyagerTests"
                  ],
     "error-retries": 0,
+    "num-sims": 2,
+    "unsafe-skip-xcode-version-check": true,
     "test": "/tmp",
     "output-dir": "/tmp/simulator",
     "test-bundle-path": "/usr/bin",