Reuse Simulator (from sim-reuse branch) #54

njuliyu · 2017-02-08T06:38:54Z

Option added:
"reuse-simulator" added at the bluepill command level.
"keep-simulator", "use-simulator", "delete-simulator" added at the bp command level for internal use.

Below is how this feature works:

Start bluepill command with "--reuse-simulator" or add "reuse-simulator : true" to config.json
The driver process will start a bunch of bp instance with "keep-simulator: true"
When "keep-simulator" is enabled, the bp instance won't shutdown and delete the simulator when the test bundle is finished normally (pass or normal fail).
If the test cases crash or hang, or simulator crashes, the bp instance will force delete the simulator and won't allow to reuse it.
The bp instance pass the device id of the simulator back to the driver process by writing the id to a temporary file before it terminates.
The bluepill driver process read the temporary file to get the simulator device id and stored it in a pool ready to be reused.
If there are available simulator device id to reuse, the driver process will pass the option "use-simualtor: [device id string]" when starting a new bp instance.
When a bp instance start with a specified device id, it won't create new device first, it will try to look for a running device with specified id first, and also connect to the existing running simulator GUI app if it is in the non-headless mode.
if a device is successfully reused, bp will continue to install the app and launch the test cases to run.
if a device is invalid and cannot be reused, but retry is allowed by the configuration, a new device will be created.
When all test bundles have been finished, and the device id pool to reuse is not empty, the bluepill driver will launch new bp instance with "delete-simulator:[device id string]" for each device to shutdown and delete all the running simulators. But it is fire & forget so the driver won't wait for the bp to return.
Some test cases have been added to BluepillTest.m and a new BPIntegrationTests.m to test this feature.

* find the running simulator app process by device id so we can shutdown the simulator cleanly * bluebill will delete all simulators after all bp instances has finished when keep-simulator is enabled * added test suite BPIntegrationTests

…d when Retry when it has been deleted due to crash or hang in the first run. * added test case testReuseSimulatorRetryAppCrashingTestsSet();

* support recreate simulator when the specified device id is not valid and retry is allowed. * added test case testReuseSimulatorNotExistWithRetry

…lativePathInConfigurationFile to sync with the Linkedin repository.

* fixed some minor style issues.

* minor style fixes.

…eID" to "useSimUDID" and "deleteSimUDID" respectively.

- fixed some minor style issue - changed some function names from "deviceID" to "UDID"

njuliyu · 2017-02-12T06:29:21Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

+    context.simulatorCreated = NO;  //also use this flag to tell writeSimUDIDFile() the simulator not avaiable
+    context.config.useSimUDID = nil; //prevent reuse this device when RETRY
+    self.config.useSimUDID = nil;
+


Previous comments from @vargon:
Does self.config need to be altered here? It should be treated as immutable. That's why we store executionConfigCopy which we do treat as mutable.

My comments:
Sorry I don't know self.config should be treated as immutable. But If we don't reset it here, the original value will be copied from self.config to context.config again when retry, and the invalid UDID may be reused, so only changing context.config is not enough.

If we don't want to modify self.config in any way, one workaround is to add one more property to Bluepill class, e.g. noUseSimUDID, and use that to control if we should use useSimUDID, but that looks redundant and awkward too.

Let's make BPConfiguration fields readonly

oliverhu · 2017-02-13T06:11:20Z

Bluepill-cli/BPInstanceTests/BluepillTests.m

@@ -344,6 +344,137 @@ - (void)testRunWithPassingTestsSet {
    XCTAssert(exitCode == BPExitStatusTestsAllPassed);
 }

+- (void)testReuseSimulator {
+    //[BPUtils quietMode:NO];


remove, same below

do you mean remove "//[BPUtils quietMode:NO];", but I find it very useful to uncomment it during test & debug. If we don't like commented code, I would like to uncomment and disable quite mode.

how about making [BPUtils quietModel:YES] as default in tests? (move to +(void)setUp{})

If we move this to setup as default, then there will be too much output from every test case.
ideally, I think should have different control when we run the tests in different environments, like CI vs local. So the hardcoded way is not the good way. Better way should be controlled by an environment variable or CLI option passed to test (not sure how to do) .
As @ashit said, our quite and debug mode in the current code is a mess. So I think I will just leave these few lines of commented code in the test cases for now, and do a clean up when we have sorted out a better way to control quite and debug mode.

oliverhu · 2017-02-13T06:14:06Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.h

@@ -28,4 +28,6 @@

 - (instancetype)init NS_UNAVAILABLE;

+- (NSString *)getSimulatorUDID;


documentation pls.

Make this a (readonly) propoerty

oliverhu · 2017-02-13T06:15:38Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

+    BOOL success = [context.runner uninstallApplicationAndReturnError:&error];
+
+    [[BPStats sharedStats] endTimer:stepName];
+    [BPUtils printInfo:(success ? INFO : ERROR) withString:[@"Completed: " stringByAppendingString:stepName]];


@"Completed: %@", stepName

oliverhu · 2017-02-13T06:16:34Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

+    context.simulatorCreated = NO;  //also use this flag to tell writeSimUDIDFile() the simulator not avaiable
+    context.config.useSimUDID = nil; //prevent reuse this device when RETRY
+    self.config.useSimUDID = nil;
+


Let's make BPConfiguration fields readonly

oliverhu · 2017-02-13T06:21:36Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

+        context.simulatorCreated = YES; //if we don't set this flag, deleteSimulatorWithContext() won't proceed
+
+        NEXT([self deleteSimulatorWithContext:context andStatus:BPExitStatusSimulatorDeleted]);
+


oliverhu · 2017-02-13T06:22:32Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

+    NSString *idStr = [self getSimulatorUDID];
+    if (!idStr || !self.context.simulatorCreated) return;
+
+    NSString *tempFileName = [NSString stringWithFormat:@"bluepill-deviceid.%d",getpid()];


space before getpid()

oliverhu · 2017-02-13T06:23:21Z

Bluepill-cli/Bluepill-cli/Simulator/SimulatorRunner.m

@@ -61,6 +61,27 @@ - (void)createSimulatorWithDeviceName:(NSString *)deviceName completion:(void (^
    }];
 }

+- (BOOL)useSimulatorWithDeviceUDID:(NSUUID *)deviceUDID {
+    self.device = [SimulatorRunner findDeviceWithConfig:self.config andDeviceID:deviceUDID];
+    if (self.device) {


there are too many ifs

There are. But the code block are small, so the readability are pretty good. Any better way?

if (!self.device) {return no;} if (![self.device.stateString isEqualToString:@"Booted"]) {printError, return NO} Other logic?

oliverhu · 2017-02-13T06:23:42Z

Bluepill-cli/Bluepill-cli/Simulator/SimulatorRunner.m

-    if (!installed) return FALSE;
-    return TRUE;
+ (BOOL)uninstallAppWithBundleID:(NSString *)hostBundleID
+                    bundlePath:(NSString *)hostBundlePath


indentation

oliverhu · 2017-02-13T06:24:49Z

Bluepill-cli/Bluepill-cli/Simulator/SimulatorRunner.m

@@ -190,6 +217,43 @@ + (void)createDeviceWithConfig:(BPConfiguration *)config andName:(NSString *)dev
                       }];
 }

+ (SimDevice *)findDeviceWithConfig:(BPConfiguration *)config andDeviceID:(NSUUID *)deviceID {


Pass in an NSError ** to store the error instead of printing here.

I couldn't see the benefit of passing NSError ** here. look at the code of createDeviceWithConfig, which has completion:(void (^)(NSError *, SimDevice *))completion. But findDeviceWithConfig doesn't have an async call with completion block. any benefit of create our own NSError object and return to the caller?

Nah, by passing a NSError back to the caller, it is up to the caller to decide how to deal with the error (suppress, error handling etc), instead of printing things out just here.

To reduce your concern, I moved the printing of error messages to the caller when device lookup fail.
I think in this simply case of dictionary lookup, return of nil is good enough to indicate an error to the caller, no extra NSError object is needed. The other methods in SimulatorRunner.m which need a NSError pointer actually pass that to the completion block, but it doesn't apply in this sync method, so we should be OK.

oliverhu · 2017-02-13T06:25:48Z

Bluepill-cli/Bluepill-cli/Simulator/SimulatorRunner.m

+ }
+
+ (NSRunningApplication *)findSimAppWithDeviceUDID:(NSString *)deviceUDID {
+    NSString * cmd = [NSString stringWithFormat:@"ps -A | grep 'Simulator\\.app.*-CurrentDeviceUDID %@'", deviceUDID];


i'd prefer xcrun simctl list | grep booted

Here we are looking for the process id of the running simulator app with GUI. simctl command won't help here.

…invalid simulator in retry - add a new property mayUseSim to Bluepill class to indicate if we may reuse the specified simulator - added test case testDeleteSimulatorNotExistWithRetry

- change the Bluepill method getSimulatorUDID to a readonly property simulatorUDID - reorganize the code of useSimulatorWithDeviceUDID() to reduce the level of nested if - fixed the missing step name when printing the information of uninstalling app - fixed other minor style issues

- process the options differently based on master or slave program before calling getopt_long() - allow "--test" option both at master and slave program - allow "--reuse-simulator" option at master program only, and the other 3 reusing sim related options at slave program only - if "--delete-simulator" option is used, no other options are required.

…m-reuse

oliverhu · 2017-05-11T03:59:08Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.h

@@ -14,6 +14,8 @@

 @interface Bluepill : NSObject

+@property (nonatomic, readonly) NSString *simulatorUDID;


move this to BPConfiguration?

two benefits:

avoid polluting the bluepill class

users can also use this field for single bluepill-cli instance (point to specific simulator by UUID)

BPConfiguration already has "@Property (nonatomic, strong) NSString *useSimUDID;"

simulatorUDID is defined as

(NSString *)simulatorUDID {
return self.context.runner.UDID;
}

Which is the runtime UDID and could be different than the one specified from the CLI.

Currently simulatorUDID is mostly used in the test cases

renamed to test_simulatorUDID as we discussed.

oliverhu · 2017-05-11T04:00:05Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

@@ -40,6 +40,7 @@ @interface Bluepill()<BPTestBundleConnectionDelegate>
 @property (nonatomic, assign) BOOL exitLoop;
 @property (nonatomic, assign) NSInteger failureTolerance;
 @property (nonatomic, assign) NSInteger retries;
+@property (nonatomic, assign) BOOL mayReuseSim;


I can't remember if I left a comment for this before.. but I am not a fan of may or possible for variable name.

allowReuseSim looks better?

reuseSimAllowed ?

oliverhu · 2017-05-11T04:01:44Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

+    } else {
+        self.mayReuseSim = NO; //prevent reuse this device when RETRY
+
+


oliverhu · 2017-05-11T04:03:16Z

Bluepill-cli/Bluepill-cli/Bluepill/Bluepill.m

@@ -458,8 +518,12 @@ - (void)runnerCompletedWithContext:(BPExecutionContext *)context {
    if (context.simulatorCrashed) {
        // If we crashed, we need to retry
        [self deleteSimulatorWithContext:context andStatus:BPExitStatusSimulatorCrashed];
+    } else if (self.config.keepSimulator && (context.runner.exitStatus == BPExitStatusTestsAllPassed ||


if (a && e && (b || c))

oliverhu · 2017-05-11T04:04:19Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

@@ -145,6 +169,107 @@ - (NSError *)waitForDeviceReady {
    return nil;
 }

+ (BOOL)installAppWithBundleID:(NSString *)hostBundleID


this is merge conflict , use - (BOOL)installApp

This is a new method added, not merge conflict. but it is OK to change it to a instance method.

oliverhu · 2017-05-11T04:04:28Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

+                                  error:error];
+}
+
+ (void)createDeviceWithConfig:(BPConfiguration *)config andName:(NSString *)deviceName completion:(void (^)(NSError *, SimDevice *))completion {


same here, merge conflict

oliverhu · 2017-05-11T04:04:34Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

+                                error:error];
+}
+
+ (BOOL)uninstallAppWithBundleID:(NSString *)hostBundleID


change to instance method

oliverhu · 2017-05-11T04:06:21Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

+    return device; //could be nil when not found
+ }
+
+ (NSRunningApplication *)findSimAppWithDeviceUDID:(NSString *)deviceUDID {


Confusing name. runningAppInDeviceUDID

Why do you need this method btw? can you just do a [simDevice getRunningApp]

I think you asked the same question before :-)
This is to find the simulator GUI app, not the app running in the simulator.

change to simulatorWithDeviceUDID?

the name of 'simulator" is even more confusing, which sounds like a sim device. I changed the method name to "findSimGUIAppWith DeviceUDID"

oliverhu · 2017-05-11T04:06:59Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

+    [BPUtils printInfo:INFO withString:@" ¯\\_(ツ)_/¯"];
+}
+
+ (SimDevice *)findDeviceWithConfig:(BPConfiguration *)config andDeviceID:(NSUUID *)deviceID {


if we move deviceID into config, we can have deviceWithConfig

the deviceID could be one of config.useSimUDID or config.deleteSimUDID, so we have to keep it.

oliverhu · 2017-05-11T04:07:20Z

Bluepill-runner/BluepillRunnerTests/BPIntegrationTests.m

@@ -66,14 +66,14 @@ - (void)testOneBPInstance {
    BPRunner *runner = [BPRunner BPRunnerForApp:app withConfig:self.config withBpPath:bpPath];
    int rc = [runner run];
    XCTAssert(rc == 0);
-    //XCTAssert([runner.nsTaskList count] == 0);


any reason to keep this commented out line?

…m-reuse

* renamed a few variables and methods * changed a few class methods to instance methods. * minor fixes to code style.

oliverhu

almost there:D

oliverhu · 2017-05-12T21:23:57Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

@@ -145,6 +169,55 @@ - (NSError *)waitForDeviceReady {
    return nil;
 }

+- (BOOL)installAppWithBundleID:(NSString *)hostBundleID


I don't think we need this method. Device should be self's device

oliverhu · 2017-05-12T21:53:27Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

+                                error:error];
+}
+
+- (BOOL)uninstallAppWithBundleID:(NSString *)hostBundleID


oliverhu · 2017-05-12T21:53:37Z

Bluepill-cli/Bluepill-cli/Simulator/BPSimulator.m

@@ -171,8 +244,16 @@ - (BOOL)installApplicationAndReturnError:(NSError *__autoreleasing *)error {
    return YES;
 }

- (void)launchApplicationAndExecuteTestsWithParser:(BPTreeParser *)parser andCompletion:(void (^)(NSError *, pid_t))completion isHostApp:(BOOL)isHostApp {
+- (BOOL)uninstallApplicationAndReturnError:(NSError *__autoreleasing *)error {


possibly also here?

oliverhu · 2017-05-15T05:45:38Z

Bluepill-runner/Bluepill-runner/BPRunner.m

@@ -210,6 +245,12 @@ - (int)run {
        seconds += 1;
    }

+    for (int i=0; i<[deviceList count]; i++) {


spaces between operators!!!

i = 0, i < xx

…m-reuse

njuliyu and others added 17 commits January 25, 2017 12:24

＊ support reusing existing running simulators between test bundles.

6fc1cd0

Merge branch 'master' of https://github.com/linkedin/bluepill

2af4564

Merge branch 'master' of https://github.com/linkedin/bluepill

ea7d7d6

Merge branch 'master' of https://github.com/linkedin/bluepill

b0f6e50

* fixed the bug that the specified device id may be incorrectly reuse…

2fb4d65

…d when Retry when it has been deleted due to crash or hang in the first run. * added test case testReuseSimulatorRetryAppCrashingTestsSet();

* added the CLI option --reuse-simulator

6f585d1

* support recreate simulator when the specified device id is not valid and retry is allowed. * added test case testReuseSimulatorNotExistWithRetry

* added option "reuse-simulator" to README.md

50e6d8d

Merge branch 'master' of https://github.com/linkedin/bluepill

f0a1ba4

minor change to the CLI usage information.

9436ced

Deleted empty test case testRunWithConfigurationFile(), testRunWithRe…

8e674f7

…lativePathInConfigurationFile to sync with the Linkedin repository.

* print the device UUID when reuse and delete a device

befbacd

* fixed some minor style issues.

* uninstall the existing app first when reusing a simulator

41d8cdc

* minor style fixes.

Fixed the parameter passed to SimDevice:uninstallApplication()

b84823e

* rename the configuration attribute name "useDeviceID" and "delDevic…

85b32c5

…eID" to "useSimUDID" and "deleteSimUDID" respectively.

Review changes.

56cdfaf

Merge branch 'sim-reuse-yuli' into sim-reuse

8f09f5d

ob mentioned this pull request Feb 10, 2017

Reuse simulator #51

Closed

Fixes for style and function names

1dc1caa

- fixed some minor style issue - changed some function names from "deviceID" to "UDID"

njuliyu commented Feb 12, 2017

View reviewed changes

oliverhu suggested changes Feb 13, 2017

View reviewed changes

njuliyu and others added 9 commits February 22, 2017 18:03

Don't modify self.config when we don't want to reuse a specified but …

cd0239a

…invalid simulator in retry - add a new property mayUseSim to Bluepill class to indicate if we may reuse the specified simulator - added test case testDeleteSimulatorNotExistWithRetry

Merge branch 'master' into sim-reuse (half-baked, Yu will fix)

53836d7

Merge branch 'sim-reuse' of github.com:linkedin/bluepill into sim-reuse

78cfa4d

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

1503d8a

…m-reuse

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

2d6abf3

…m-reuse

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

4b6d679

…m-reuse

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

7dbc5ef

…m-reuse

njuliyu added 8 commits March 19, 2017 12:58

Added test case testTwoBPInstancesReuseSim

52cccfc

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

62db783

…m-reuse

Fixed a string formating bug which could cause memory issue

419258e

Removed some lines of garbage

78a0026

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

c98aa0c

…m-reuse

Test switching to Circle CI

72a85e8

Added all tests to circle CI

98e6616

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

272a6b8

…m-reuse

oliverhu reviewed May 11, 2017

View reviewed changes

njuliyu added 4 commits May 11, 2017 15:50

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

8d8e9db

…m-reuse

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

283aec3

…m-reuse

Some minor fixes based on the code review feedback

8fef39e

* renamed a few variables and methods * changed a few class methods to instance methods. * minor fixes to code style.

Removed non-used method createDeviceWithConfig()

5d575ee

oliverhu reviewed May 12, 2017

View reviewed changes

Removed methods: installAppWithBundleID() & uninstallAppWithBundleID

5eb8bb7

oliverhu approved these changes May 15, 2017

View reviewed changes

njuliyu added 3 commits May 15, 2017 00:03

Fixed a minor code style issue

1fcfcfa

Change the log tag [SIM-%d] to [BP-%d] to avoid confusion

26c7b02

Merge branch 'master' of https://github.com/linkedin/bluepill into si…

207bc58

…m-reuse

njuliyu merged commit 3bcef0c into master May 15, 2017

oliverhu mentioned this pull request May 31, 2017

Reuse simulator between test bundles. #5

Closed

oliverhu deleted the sim-reuse branch July 11, 2017 23:54

		@@ -28,4 +28,6 @@

		- (instancetype)init NS_UNAVAILABLE;

		- (NSString *)getSimulatorUDID;

		context.simulatorCreated = YES; //if we don't set this flag, deleteSimulatorWithContext() won't proceed

		NEXT([self deleteSimulatorWithContext:context andStatus:BPExitStatusSimulatorDeleted]);

		@@ -14,6 +14,8 @@

		@interface Bluepill : NSObject

		@property (nonatomic, readonly) NSString *simulatorUDID;

		} else {
		self.mayReuseSim = NO; //prevent reuse this device when RETRY

Reuse Simulator (from sim-reuse branch) #54

Reuse Simulator (from sim-reuse branch) #54

Conversation

njuliyu commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverhu Feb 13, 2017 • edited by njuliyu

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverhu Feb 13, 2017 •

edited by njuliyu