Skip to content

Commit

Permalink
Fix intermittent falure in Darwin browsing test. (#35421)
Browse files Browse the repository at this point in the history
We are running the test against Matter applications that are starting up, and
during startup we do some silly unregistering/re-registering of DNS-SD
advertisements.  The test was counting all the registrations, not subtracting
the un-registrations, and asserting that the resulting count was not too high.
But sometimes it is, if we get a "unregister/re-register" pair in there.

The fix:

1) Fixes the test to better track un-registration bits.
2) Fixes the OnBleScanRemove code to call the right delegate method.
3) Fixes OnBrowseRemove to not call the delegate if we have not notified about
   the add for the relevant result.  We could have just worked around this in
   the test, but notifying "removed" about an object with all fields set to nil
   just doesn't make much sense.
  • Loading branch information
bzbarsky-apple authored Sep 5, 2024
1 parent 17b1a38 commit e78a186
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 10 deletions.
13 changes: 9 additions & 4 deletions src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,14 @@ void OnBrowseRemove(DnssdService service) override
// If there is nothing else to resolve for the given instance name, just remove it
// too and informs the delegate that it is gone.
if ([interfaces count] == 0) {
dispatch_async(mDispatchQueue, ^{
[mDelegate controller:mController didRemoveCommissionableDevice:result];
});
// If result.instanceName is nil, that means we never notified our
// delegate about this result (because we did not get that far in
// resolving it), so don't bother notifying about the removal either.
if (result.instanceName != nil) {
dispatch_async(mDispatchQueue, ^{
[mDelegate controller:mController didRemoveCommissionableDevice:result];
});
}

mDiscoveredResults[key] = nil;
}
Expand Down Expand Up @@ -325,7 +330,7 @@ void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override
MATTER_LOG_METRIC(kMetricBLEDevicesRemoved, ++mBLEDevicesRemoved);

dispatch_async(mDispatchQueue, ^{
[mDelegate controller:mController didFindCommissionableDevice:result];
[mDelegate controller:mController didRemoveCommissionableDevice:result];
});
}

Expand Down
52 changes: 46 additions & 6 deletions src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,27 @@
// Singleton controller we use.
static MTRDeviceController * sController = nil;

static NSString * kInstanceNameKey = @"instanceName";
static NSString * kVendorIDKey = @"vendorID";
static NSString * kProductIDKey = @"productID";
static NSString * kDiscriminatorKey = @"discriminator";
static NSString * kCommissioningModeKey = @"commissioningMode";

static NSDictionary<NSString *, id> * ResultSnapshot(MTRCommissionableBrowserResult * result)
{
return @{
kInstanceNameKey : result.instanceName,
kVendorIDKey : result.vendorID,
kProductIDKey : result.productID,
kDiscriminatorKey : result.discriminator,
kCommissioningModeKey : @(result.commissioningMode),
};
}

@interface DeviceScannerDelegate : NSObject <MTRCommissionableBrowserDelegate>
@property (nonatomic, nullable) XCTestExpectation * expectation;
@property (nonatomic) NSNumber * resultsCount;
@property (nonatomic) NSMutableArray<NSDictionary<NSString *, id> *> * results;
@property (nonatomic) NSMutableArray<NSDictionary<NSString *, id> *> * removedResults;

- (instancetype)initWithExpectation:(XCTestExpectation *)expectation;
- (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device;
Expand All @@ -52,8 +70,9 @@ - (instancetype)initWithExpectation:(XCTestExpectation *)expectation
return nil;
}

_resultsCount = 0;
_expectation = expectation;
_results = [[NSMutableArray alloc] init];
_removedResults = [[NSMutableArray alloc] init];
return self;
}

Expand All @@ -63,8 +82,9 @@ - (instancetype)init
return nil;
}

_resultsCount = 0;
_expectation = nil;
_results = [[NSMutableArray alloc] init];
_removedResults = [[NSMutableArray alloc] init];
return self;
}

Expand All @@ -77,12 +97,26 @@ - (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice
return;
}

_resultsCount = @(_resultsCount.unsignedLongValue + 1);
if ([_resultsCount isEqual:@(kExpectedDiscoveredDevicesCount)]) {
__auto_type * snapshot = ResultSnapshot(device);

XCTAssertFalse([_results containsObject:snapshot], @"Newly discovered device %@ should not be in results already.", snapshot);

[_results addObject:snapshot];

if (_results.count == kExpectedDiscoveredDevicesCount) {
// Do some sanity checking on our results and removedResults to make
// sure we really only saw the relevant set of things.
NSSet<NSDictionary<NSString *, id> *> * finalResultsSet = [NSSet setWithArray:_results];
NSSet<NSDictionary<NSString *, id> *> * allResultsSet = [finalResultsSet copy];
allResultsSet = [allResultsSet setByAddingObjectsFromArray:_removedResults];

// Ensure that we just saw the same results as our final set popping in and out if things
// ever got removed here.
XCTAssertEqualObjects(finalResultsSet, allResultsSet);
[self.expectation fulfill];
}

XCTAssertLessThanOrEqual(_resultsCount.unsignedLongValue, kExpectedDiscoveredDevicesCount);
XCTAssertLessThanOrEqual(_results.count, kExpectedDiscoveredDevicesCount);

__auto_type instanceName = device.instanceName;
__auto_type vendorId = device.vendorID;
Expand All @@ -108,6 +142,12 @@ - (void)controller:(MTRDeviceController *)controller didRemoveCommissionableDevi

NSLog(
@"Removed Device (%@) with discriminator: %@ (vendor: %@, product: %@)", instanceName, discriminator, vendorId, productId);

__auto_type * snapshot = ResultSnapshot(device);
XCTAssertTrue([_results containsObject:snapshot], @"Removed device %@ is not something we found before", snapshot);

[_removedResults addObject:snapshot];
[_results removeObject:snapshot];
}
@end

Expand Down

0 comments on commit e78a186

Please sign in to comment.