Address review comment.

bzbarsky-apple · bzbarsky-apple · commit da8d550cceab · 2024-09-05T21:13:23.000-04:00
Switch to just having app-starting functionality as a category on MTRTestCase.
This is much easier to use.
diff --git a/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m b/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m
@@ -184,11 +184,10 @@ + (void)setUp
              @"MT:Y.K90SO527JA0648G00",
              @"MT:-24J0AFN00I40648G00",
          ]) {
-        __auto_type * appRunner = [[MTRTestServerAppRunner alloc] initCrossTestWithAppName:@"all-clusters"
-                                                                                 arguments:@[]
-                                                                                   payload:payload
-                                                                                 testsuite:self];
-        XCTAssertNotNil(appRunner);
+        BOOL started = [self startAppWithName:@"all-clusters"
+                                    arguments:@[]
+                                      payload:payload];
+        XCTAssertTrue(started);
     }
 }
 
diff --git a/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m b/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m
@@ -80,7 +80,7 @@ - (NSString *)createImageFromRawImage:(NSString *)rawImage withVersion:(NSNumber
 - (MTRDevice *)commissionDeviceWithPayload:(NSString *)payloadString nodeID:(NSNumber *)nodeID;
 @end
 
-@interface MTROTARequestorAppRunner : MTRTestServerAppRunner
+@interface MTROTARequestorAppRunner : NSObject
 @property (nonatomic, copy) NSString * downloadFilePath;
 
 - (instancetype)initWithPayload:(NSString *)payload testcase:(MTROTAProviderTests *)testcase;
@@ -99,14 +99,15 @@ - (MTRDevice *)commissionWithNodeID:(NSNumber *)nodeID
 
 - (instancetype)initWithPayload:(NSString *)payload testcase:(MTROTAProviderTests *)testcase
 {
-    __auto_type * downloadFilePath = [NSString stringWithFormat:@"/tmp/chip-ota-requestor-downloaded-image%u", [MTRTestServerAppRunner nextUniqueIndex]];
+    __auto_type * downloadFilePath = [NSString stringWithFormat:@"/tmp/chip-ota-requestor-downloaded-image%u", [MTROTAProviderTests nextUniqueIndex]];
     __auto_type * extraArguments = @[
         @"--otaDownloadPath",
         downloadFilePath,
         @"--autoApplyImage",
     ];
 
-    if (!(self = [super initWithAppName:@"ota-requestor" arguments:extraArguments payload:payload testcase:testcase])) {
+    BOOL started = [testcase startAppWithName:@"ota-requestor" arguments:extraArguments payload:payload];
+    if (!started) {
         return nil;
     }
 
diff --git a/src/darwin/Framework/CHIPTests/MTRPairingTests.m b/src/darwin/Framework/CHIPTests/MTRPairingTests.m
@@ -186,14 +186,13 @@ - (void)tearDown
 - (void)startServerApp
 {
     // For manual testing, CASE retry code paths can be tested by adding --faults chip_CASEServerBusy_f1 (or similar)
-    __auto_type * appRunner = [[MTRTestServerAppRunner alloc] initWithAppName:@"all-clusters"
-                                                                    arguments:@[
-                                                                        @"--dac_provider",
-                                                                        [self absolutePathFor:@"credentials/development/commissioner_dut/struct_cd_origin_pid_vid_correct/test_case_vector.json"],
-                                                                    ]
-                                                                      payload:kOnboardingPayload
-                                                                     testcase:self];
-    XCTAssertNotNil(appRunner);
+    BOOL started = [self startAppWithName:@"all-clusters"
+                                arguments:@[
+                                    @"--dac_provider",
+                                    [self absolutePathFor:@"credentials/development/commissioner_dut/struct_cd_origin_pid_vid_correct/test_case_vector.json"],
+                                ]
+                                  payload:kOnboardingPayload];
+    XCTAssertTrue(started);
 }
 
 // attestationDelegate and failSafeExtension can both be nil
diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
@@ -2437,11 +2437,10 @@ - (void)testSubscriptionPool
     // Start our helper apps.
     __auto_type * sortedKeys = [[deviceOnboardingPayloads allKeys] sortedArrayUsingSelector:@selector(compare:)];
     for (NSNumber * deviceID in sortedKeys) {
-        __auto_type * appRunner = [[MTRTestServerAppRunner alloc] initWithAppName:@"all-clusters"
-                                                                        arguments:@[]
-                                                                          payload:deviceOnboardingPayloads[deviceID]
-                                                                         testcase:self];
-        XCTAssertNotNil(appRunner);
+        BOOL started = [self startAppWithName:@"all-clusters"
+                                    arguments:@[]
+                                      payload:deviceOnboardingPayloads[deviceID]];
+        XCTAssertTrue(started);
     }
 
     [self doTestSubscriptionPoolWithSize:1 deviceOnboardingPayloads:deviceOnboardingPayloads];
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h
@@ -57,7 +57,7 @@ NS_ASSUME_NONNULL_BEGIN
  * Launch a cross-test task.  The task will be automatically terminated when the testsuite
  * tearDown happens.
  */
-+ (void)launchCrossTestTask:(NSTask *)task;
++ (void)launchTask:(NSTask *)task;
 #endif // HAVE_NSTASK
 
 /**
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm
@@ -85,18 +85,18 @@ - (void)tearDown
 }
 
 #if HAVE_NSTASK
+- (NSTask *)createTaskForPath:(NSString *)path
+{
+    return [self.class createTaskForPath:path];
+}
+
 + (NSTask *)createTaskForPath:(NSString *)path
 {
     NSTask * task = [[NSTask alloc] init];
     [task setLaunchPath:[self absolutePathFor:path]];
     return task;
 }
 
-- (NSTask *)createTaskForPath:(NSString *)path
-{
-    return [self.class createTaskForPath:path];
-}
-
 - (void)runTask:(NSTask *)task
 {
     NSError * launchError;
@@ -121,14 +121,19 @@ - (void)launchTask:(NSTask *)task
     [_runningTasks addObject:task];
 }
 
-+ (void)launchCrossTestTask:(NSTask *)task
++ (void)launchTask:(NSTask *)task
 {
     [self doLaunchTask:task];
 
     [runningCrossTestTasks addObject:task];
 }
 #endif // HAVE_NSTASK
 
+- (NSString *)absolutePathFor:(NSString *)matterRootRelativePath
+{
+    return [self.class absolutePathFor:matterRootRelativePath];
+}
+
 + (NSString *)absolutePathFor:(NSString *)matterRootRelativePath
 {
     // Start with the absolute path to our file, then remove the suffix that
@@ -140,8 +145,4 @@ + (NSString *)absolutePathFor:(NSString *)matterRootRelativePath
     return [NSString pathWithComponents:pathComponents];
 }
 
-- (NSString *)absolutePathFor:(NSString *)matterRootRelativePath
-{
-    return [self.class absolutePathFor:matterRootRelativePath];
-}
 @end
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestServerAppRunner.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestServerAppRunner.h
@@ -16,21 +16,14 @@
 
 #import <Foundation/Foundation.h>
 
-@class MTRTestCase;
+#import "MTRTestCase.h"
 
 NS_ASSUME_NONNULL_BEGIN
 
-/**
- * A representation of a server application instance.
- *
- * Server applications are assumed to be compiled into out/debug/${APPNAME}-app,
- * with the binary being out/debug/${APPNAME}-app/chip-${APPNAME}-app.
- */
-@interface MTRTestServerAppRunner : NSObject
+@interface MTRTestCase (ServerAppRunner)
 
 /**
- * Initialize the app runner with the given app name, arguments, setup payload, and testcase
- * instance.
+ * Start a server app with the given app name, arguments, and setup payload.
  *
  * The payload will be used to determine the discriminator and passcode
  * arguments the app should use, in addition to the provided arguments.
@@ -43,15 +36,13 @@ NS_ASSUME_NONNULL_BEGIN
  * subtracting 1111 from the discriminator and adding 5542 (so as not to collide
  * with any existing Matter things running on 5540/5541).
  */
-- (instancetype)initWithAppName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload testcase:(MTRTestCase *)testcase;
+- (BOOL)startAppWithName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload;
 
 /**
- * Same thing, but initialize as a "cross test" helper, which is not killed at
- * the end of the current test (but is killed at the end of the current suite).
- *
- * The passed-in testsuite must be a subclass of MTRTestCase.
+ * Same thing, but the server will be killed at the end of the current suite,
+ * and is not bound to a particular test in the suite.
  */
-- (instancetype)initCrossTestWithAppName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload testsuite:(Class)testsuite;
++ (BOOL)startAppWithName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload;
 
 /**
  * Get the unique index that will be used for the next initialization.  This
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestServerAppRunner.m b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestServerAppRunner.m
@@ -16,7 +16,6 @@
 
 #import <Matter/Matter.h>
 
-#import "MTRTestCase.h"
 #import "MTRTestServerAppRunner.h"
 
 static unsigned sAppRunnerIndex = 1;
@@ -29,24 +28,12 @@
 static const uint16_t kBasePort = 5542 - kMinDiscriminator;
 #endif // HAVE_NSTASK
 
-@implementation MTRTestServerAppRunner {
-    unsigned _uniqueIndex;
-#if HAVE_NSTASK
-    NSTask * _appTask;
-#endif
-}
+@implementation MTRTestCase (ServerAppRunner)
 
-- (instancetype)initInternalWithAppName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload
+#if HAVE_NSTASK
++ (NSTask *)doStartAppWithName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload
 {
-#if !HAVE_NSTASK
-    XCTFail("Unable to start server app when we do not have NSTask");
-    return nil;
-#else // HAVE_NSTASK
-    if (!(self = [super init])) {
-        return nil;
-    }
-
-    _uniqueIndex = sAppRunnerIndex++;
+    __auto_type uniqueIndex = sAppRunnerIndex++;
 
     NSError * error;
     __auto_type * parsedPayload = [MTRSetupPayload setupPayloadWithOnboardingPayload:payload error:&error];
@@ -61,7 +48,7 @@ - (instancetype)initInternalWithAppName:(NSString *)name arguments:(NSArray<NSSt
     NSNumber * passcode = parsedPayload.setupPasscode;
 
     __auto_type * executable = [NSString stringWithFormat:@"out/debug/%@-app/chip-%@-app", name, name];
-    _appTask = [MTRTestCase createTaskForPath:executable];
+    __auto_type * appTask = [self createTaskForPath:executable];
 
     __auto_type * forcedArguments = @[
         // Make sure we only advertise on the local interface.
@@ -74,55 +61,50 @@ - (instancetype)initInternalWithAppName:(NSString *)name arguments:(NSArray<NSSt
         @"--passcode",
         [NSString stringWithFormat:@"%llu", passcode.unsignedLongLongValue],
         @"--KVS",
-        [NSString stringWithFormat:@"/tmp/chip-%@-kvs%u", name, _uniqueIndex],
+        [NSString stringWithFormat:@"/tmp/chip-%@-kvs%u", name, uniqueIndex],
         @"--product-id",
         [NSString stringWithFormat:@"%u", parsedPayload.productID.unsignedShortValue],
     ];
 
     __auto_type * allArguments = [forcedArguments arrayByAddingObjectsFromArray:arguments];
-    [_appTask setArguments:allArguments];
+    [appTask setArguments:allArguments];
 
-    NSString * outFile = [NSString stringWithFormat:@"/tmp/darwin/framework-tests/%@-app-%u.log", name, _uniqueIndex];
-    NSString * errorFile = [NSString stringWithFormat:@"/tmp/darwin/framework-tests/%@-app-err-%u.log", name, _uniqueIndex];
+    NSString * outFile = [NSString stringWithFormat:@"/tmp/darwin/framework-tests/%@-app-%u.log", name, uniqueIndex];
+    NSString * errorFile = [NSString stringWithFormat:@"/tmp/darwin/framework-tests/%@-app-err-%u.log", name, uniqueIndex];
 
     // Make sure the files exist.
     [[NSFileManager defaultManager] createFileAtPath:outFile contents:nil attributes:nil];
     [[NSFileManager defaultManager] createFileAtPath:errorFile contents:nil attributes:nil];
 
-    _appTask.standardOutput = [NSFileHandle fileHandleForWritingAtPath:outFile];
-    _appTask.standardError = [NSFileHandle fileHandleForWritingAtPath:errorFile];
+    appTask.standardOutput = [NSFileHandle fileHandleForWritingAtPath:outFile];
+    appTask.standardError = [NSFileHandle fileHandleForWritingAtPath:errorFile];
 
-    NSLog(@"Started chip-%@-app (%@) with arguments %@ stdout=%@ and stderr=%@", name, _appTask, allArguments, outFile, errorFile);
+    NSLog(@"Started chip-%@-app (%@) with arguments %@ stdout=%@ and stderr=%@", name, appTask, allArguments, outFile, errorFile);
 
-    return self;
-#endif // HAVE_NSTASK
+    return appTask;
 }
+#endif // HAVE_NSTASK
 
-- (instancetype)initWithAppName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload testcase:(MTRTestCase *)testcase
+- (BOOL)startAppWithName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload
 {
-    if (!(self = [self initInternalWithAppName:name arguments:arguments payload:payload])) {
-        return nil;
-    }
-
-    [testcase launchTask:_appTask];
-
-    return self;
+#if !HAVE_NSTASK
+    XCTFail("Unable to start server app when we do not have NSTask");
+    return NO;
+#else
+    [self launchTask:[self.class doStartAppWithName:name arguments:arguments payload:payload]];
+    return YES;
+#endif // HAVE_NSTASK
 }
 
-- (instancetype)initCrossTestWithAppName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload testsuite:(Class)testsuite
++ (BOOL)startAppWithName:(NSString *)name arguments:(NSArray<NSString *> *)arguments payload:(NSString *)payload
 {
-    if (![testsuite isSubclassOfClass:MTRTestCase.class]) {
-        NSLog(@"%@ is not a subclass of MTRTestCase", testsuite);
-        return nil;
-    }
-
-    if (!(self = [self initInternalWithAppName:name arguments:arguments payload:payload])) {
-        return nil;
-    }
-
-    [MTRTestCase launchCrossTestTask:_appTask];
-
-    return self;
+#if !HAVE_NSTASK
+    XCTFail("Unable to start server app when we do not have NSTask");
+    return NO;
+#else
+    [self launchTask:[self doStartAppWithName:name arguments:arguments payload:payload]];
+    return YES;
+#endif // HAVE_NSTASK
 }
 
 + (unsigned)nextUniqueIndex

Original file line number	Diff line number	Diff line change
`@@ -85,18 +85,18 @@ - (void)tearDown`
`85`	`85`	`}`
`86`	`86`
`87`	`87`	`#if HAVE_NSTASK`
	`88`	`+- (NSTask )createTaskForPath:(NSString )path`
	`89`	`+{`
	`90`	`+ return [self.class createTaskForPath:path];`
	`91`	`+}`
	`92`	`+`
`88`	`93`	`+ (NSTask )createTaskForPath:(NSString )path`
`89`	`94`	`{`
`90`	`95`	`NSTask * task = [[NSTask alloc] init];`
`91`	`96`	`[task setLaunchPath:[self absolutePathFor:path]];`
`92`	`97`	`return task;`
`93`	`98`	`}`
`94`	`99`
`95`		`-- (NSTask )createTaskForPath:(NSString )path`
`96`		`-{`
`97`		`- return [self.class createTaskForPath:path];`
`98`		`-}`
`99`		`-`
`100`	`100`	`- (void)runTask:(NSTask *)task`
`101`	`101`	`{`
`102`	`102`	`NSError * launchError;`
`@@ -121,14 +121,19 @@ - (void)launchTask:(NSTask *)task`
`121`	`121`	`[_runningTasks addObject:task];`
`122`	`122`	`}`
`123`	`123`
`124`		`-+ (void)launchCrossTestTask:(NSTask *)task`
	`124`	`++ (void)launchTask:(NSTask *)task`
`125`	`125`	`{`
`126`	`126`	`[self doLaunchTask:task];`
`127`	`127`
`128`	`128`	`[runningCrossTestTasks addObject:task];`
`129`	`129`	`}`
`130`	`130`	`#endif // HAVE_NSTASK`
`131`	`131`
	`132`	`+- (NSString )absolutePathFor:(NSString )matterRootRelativePath`
	`133`	`+{`
	`134`	`+ return [self.class absolutePathFor:matterRootRelativePath];`
	`135`	`+}`
	`136`	`+`
`132`	`137`	`+ (NSString )absolutePathFor:(NSString )matterRootRelativePath`
`133`	`138`	`{`
`134`	`139`	`// Start with the absolute path to our file, then remove the suffix that`
`@@ -140,8 +145,4 @@ + (NSString )absolutePathFor:(NSString )matterRootRelativePath`
`140`	`145`	`return [NSString pathWithComponents:pathComponents];`
`141`	`146`	`}`
`142`	`147`
`143`		`-- (NSString )absolutePathFor:(NSString )matterRootRelativePath`
`144`		`-{`
`145`		`- return [self.class absolutePathFor:matterRootRelativePath];`
`146`		`-}`
`147`	`148`	`@end`