Fix Darwin CI random failures. (#35337)

* Fix Darwin CI random failures. We were starting an example app up front, then commissioning it when the test ran, but sometimes that takes longer than 15 minutes. The fix is to start example apps as needed as parts of the pairing tests that need them. The logging changes in MTRTestCase and MTRTestServerAppRunner are from trying to figure out why things were not being torn down right. That's also what the added super-calls for setUp and tearDown fix. * Adjust MTRCommissionableBrowserTests too.
project-chip · Aug 31, 2024 · a4a8c11 · a4a8c11
1 parent 815195b
commit a4a8c11
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 29 deletions.
diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml
@@ -114,7 +114,6 @@ jobs:
                   mkdir -p /tmp/darwin/framework-tests
                   echo "This is a simple log" > /tmp/darwin/framework-tests/end_user_support_log.txt
                   ../../../out/debug/all-clusters-app/chip-all-clusters-app --interface-id -1 --end_user_support_log /tmp/darwin/framework-tests/end_user_support_log.txt > >(tee /tmp/darwin/framework-tests/all-cluster-app.log) 2> >(tee /tmp/darwin/framework-tests/all-cluster-app-err.log >&2) &
-                  ../../../out/debug/all-clusters-app/chip-all-clusters-app --interface-id -1 --dac_provider ../../../credentials/development/commissioner_dut/struct_cd_origin_pid_vid_correct/test_case_vector.json --product-id 32768 --discriminator 3839 --secured-device-port 5539 --KVS /tmp/chip-all-clusters-app-kvs2 > >(tee /tmp/darwin/framework-tests/all-cluster-app-origin-vid.log) 2> >(tee /tmp/darwin/framework-tests/all-cluster-app-origin-vid-err.log >&2) &
 
                   export TEST_RUNNER_ASAN_OPTIONS=__CURRENT_VALUE__:detect_stack_use_after_return=1
 

diff --git a/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m b/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m
@@ -30,12 +30,10 @@
 
 static const uint16_t kLocalPort = 5541;
 static const uint16_t kTestVendorId = 0xFFF1u;
-static const uint16_t kTestProductId1 = 0x8000u;
-static const uint16_t kTestProductId2 = 0x8001u;
-static const uint16_t kTestDiscriminator1 = 3840u;
-static const uint16_t kTestDiscriminator2 = 3839u;
+static const __auto_type kTestProductIds = @[ @(0x8001u) ];
+static const __auto_type kTestDiscriminators = @[ @(3840u) ];
 static const uint16_t kDiscoverDeviceTimeoutInSeconds = 10;
-static const uint16_t kExpectedDiscoveredDevicesCount = 2;
+static const uint16_t kExpectedDiscoveredDevicesCount = 1;
 
 // Singleton controller we use.
 static MTRDeviceController * sController = nil;
@@ -96,8 +94,8 @@ - (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice
 
     XCTAssertEqual(instanceName.length, 16); // The  instance name is random, so just ensure the len is right.
     XCTAssertEqualObjects(vendorId, @(kTestVendorId));
-    XCTAssertTrue([productId isEqual:@(kTestProductId1)] || [productId isEqual:@(kTestProductId2)]);
-    XCTAssertTrue([discriminator isEqual:@(kTestDiscriminator1)] || [discriminator isEqual:@(kTestDiscriminator2)]);
+    XCTAssertTrue([kTestProductIds containsObject:productId]);
+    XCTAssertTrue([kTestDiscriminators containsObject:discriminator]);
     XCTAssertEqual(commissioningMode, YES);
 
     NSLog(@"Found Device (%@) with discriminator: %@ (vendor: %@, product: %@)", instanceName, discriminator, vendorId, productId);

diff --git a/src/darwin/Framework/CHIPTests/MTRPairingTests.m b/src/darwin/Framework/CHIPTests/MTRPairingTests.m
@@ -19,18 +19,11 @@
 #import <Matter/Matter.h>
 
 #import "MTRErrorTestUtils.h"
+#import "MTRTestCase.h"
 #import "MTRTestKeys.h"
-#import "MTRTestResetCommissioneeHelper.h"
+#import "MTRTestServerAppRunner.h"
 #import "MTRTestStorage.h"
 
-// system dependencies
-#import <XCTest/XCTest.h>
-
-// Fixture: chip-all-clusters-app --KVS "$(mktemp -t chip-test-kvs)" --interface-id -1 \
-    --dac_provider credentials/development/commissioner_dut/struct_cd_origin_pid_vid_correct/test_case_vector.json \
-    --product-id 32768 --discriminator 3839
-// For manual testing, CASE retry code paths can be tested by adding --faults chip_CASEServerBusy_f1 (or similar)
-
 static const uint16_t kPairingTimeoutInSeconds = 10;
 static const uint16_t kTimeoutInSeconds = 3;
 static uint64_t sDeviceId = 100000000;
@@ -138,14 +131,16 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr
 
 @end
 
-@interface MTRPairingTests : XCTestCase
+@interface MTRPairingTests : MTRTestCase
 @property (nullable) MTRPairingTestControllerDelegate * controllerDelegate;
 @end
 
 @implementation MTRPairingTests
 
 + (void)setUp
 {
+    [super setUp];
+
     __auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
     XCTAssertNotNil(factory);
 
@@ -170,6 +165,8 @@ + (void)tearDown
     sController = nil;
 
     [[MTRDeviceControllerFactory sharedInstance] stopControllerFactory];
+
+    [super tearDown];
 }
 
 - (void)setUp
@@ -182,11 +179,37 @@ - (void)tearDown
 {
     [sController setDeviceControllerDelegate:(id _Nonnull) nil queue:dispatch_get_main_queue()]; // TODO: do we need a clearDeviceControllerDelegate API?
     self.controllerDelegate = nil;
+
+    [super 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"],
+                                                                        @"--product-id",
+                                                                        @"32768",
+                                                                    ]
+                                                                      payload:kOnboardingPayload
+                                                                     testcase:self];
+    XCTAssertNotNil(appRunner);
 }
 
 // attestationDelegate and failSafeExtension can both be nil
 - (void)doPairingTestWithAttestationDelegate:(id<MTRDeviceAttestationDelegate>)attestationDelegate failSafeExtension:(NSNumber *)failSafeExtension
 {
+    [self doPairingTestWithAttestationDelegate:attestationDelegate failSafeExtension:failSafeExtension startServerApp:YES];
+}
+
+- (void)doPairingTestWithAttestationDelegate:(id<MTRDeviceAttestationDelegate>)attestationDelegate failSafeExtension:(NSNumber *)failSafeExtension startServerApp:(BOOL)startServerApp
+{
+    if (startServerApp) {
+        [self startServerApp];
+    }
+
     // Don't reuse node ids, because that will confuse us.
     ++sDeviceId;
     XCTestExpectation * expectation = [self expectationWithDescription:@"Commissioning Complete"];
@@ -209,9 +232,6 @@ - (void)doPairingTestWithAttestationDelegate:(id<MTRDeviceAttestationDelegate>)a
 
     [self waitForExpectations:@[ expectation ] timeout:kPairingTimeoutInSeconds];
     XCTAssertNil(controllerDelegate.commissioningCompleteError);
-
-    ResetCommissionee([MTRBaseDevice deviceWithNodeID:@(sDeviceId) controller:sController], dispatch_get_main_queue(), self,
-        kTimeoutInSeconds);
 }
 
 - (void)test001_PairWithoutAttestationDelegate
@@ -297,6 +317,8 @@ - (void)doPairingAndWaitForProgress:(NSString *)trigger attestationDelegate:(nul
 
 - (void)doPairingTestAfterCancellationAtProgress:(NSString *)trigger attestationDelegate:(nullable id<MTRDeviceAttestationDelegate>)attestationDelegate
 {
+    [self startServerApp];
+
     // Run pairing up and wait for the trigger
     [self doPairingAndWaitForProgress:trigger attestationDelegate:attestationDelegate];
 
@@ -314,7 +336,7 @@ - (void)doPairingTestAfterCancellationAtProgress:(NSString *)trigger attestation
     XCTAssertEqual(error.code, MTRErrorCodeCancelled);
 
     // Now pair again. If the previous attempt was cancelled correctly this should work fine.
-    [self doPairingTestWithAttestationDelegate:nil failSafeExtension:nil];
+    [self doPairingTestWithAttestationDelegate:nil failSafeExtension:nil startServerApp:NO];
 }
 
 - (void)doPairingTestAfterCancellationAtProgress:(NSString *)trigger

diff --git a/src/darwin/Framework/CHIPTests/MTRSwiftPairingTests.swift b/src/darwin/Framework/CHIPTests/MTRSwiftPairingTests.swift
@@ -1,12 +1,15 @@
 import Matter
 import XCTest
 
-// This more or less parallels the "no delegate" case in MTRPairingTests
+// This more or less parallels the "no delegate" case in MTRPairingTests, but uses the "normal"
+// all-clusters-app, since it does not do any of the "interesting" VID/PID notification so far.  If
+// it ever starts needing to do that, we should figure out a way to use MTRTestServerAppRunner from
+// here.
 
 struct PairingConstants {
     static let localPort = 5541
     static let vendorID = 0xFFF1
-    static let onboardingPayload = "MT:Y.K90SO527JA0648G00"
+    static let onboardingPayload = "MT:-24J0AFN00KA0648G00"
     static let deviceID = 0x12344321
     static let timeoutInSeconds : UInt16 = 3
 }

diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm
@@ -32,23 +32,24 @@ - (void)setUp
 #endif // HAVE_NSTASK
 }
 
-/**
- * Unfortunately, doing this in "+ (void)tearDown" (the global suite teardown)
- * does not trigger a test failure even if the XCTAssertEqual fails.
- */
 - (void)tearDown
 {
 #if defined(ENABLE_LEAK_DETECTION) && ENABLE_LEAK_DETECTION
     if (_detectLeaks) {
         int pid = getpid();
         __auto_type * cmd = [NSString stringWithFormat:@"leaks %d", pid];
         int ret = system(cmd.UTF8String);
+        /**
+         * Unfortunately, doing this in "+ (void)tearDown" (the global suite teardown)
+         * does not trigger a test failure even if the XCTAssertEqual fails.
+         */
         XCTAssertEqual(ret, 0, "LEAKS DETECTED");
     }
 #endif
 
 #if HAVE_NSTASK
     for (NSTask * task in _runningTasks) {
+        NSLog(@"Terminating task %@", task);
         [task terminate];
     }
     _runningTasks = nil;

diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestServerAppRunner.m b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestServerAppRunner.m
@@ -92,7 +92,7 @@ - (instancetype)initWithAppName:(NSString *)name arguments:(NSArray<NSString *>
 
     [testcase launchTask:_appTask];
 
-    NSLog(@"Started chip-%@-app with arguments %@ stdout=%@ and stderr=%@", name, allArguments, outFile, errorFile);
+    NSLog(@"Started chip-%@-app (%@) with arguments %@ stdout=%@ and stderr=%@", name, _appTask, allArguments, outFile, errorFile);
 
     return self;
 #endif // HAVE_NSTASK