From 2145ccce6916a865360863f29dc9d0bc8d1d713c Mon Sep 17 00:00:00 2001 From: Qin Wang Date: Wed, 26 Jul 2023 15:14:25 -0700 Subject: [PATCH] Fix flaky test:scanner_broker_impl_test PiperOrigin-RevId: 551329018 --- fastpair/scanning/BUILD | 1 + .../fast_pair_discoverable_scanner.cc | 13 ++- .../fastpair/fast_pair_discoverable_scanner.h | 5 +- .../fast_pair_discoverable_scanner_test.cc | 2 - .../fast_pair_non_discoverable_scanner.cc | 13 ++- .../fast_pair_non_discoverable_scanner.h | 4 + fastpair/scanning/scanner_broker_impl_test.cc | 86 +++++++++++++------ 7 files changed, 88 insertions(+), 36 deletions(-) diff --git a/fastpair/scanning/BUILD b/fastpair/scanning/BUILD index 521dd002..3478b93e 100644 --- a/fastpair/scanning/BUILD +++ b/fastpair/scanning/BUILD @@ -68,6 +68,7 @@ cc_test( "//fastpair/common", "//fastpair/internal/mediums", "//fastpair/proto:fastpair_cc_proto", + "//fastpair/repository:device_repository", "//fastpair/repository:test_support", "//fastpair/testing", "//internal/platform:base", diff --git a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.cc b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.cc index e44f5172..5206bf93 100644 --- a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.cc +++ b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.cc @@ -203,20 +203,27 @@ void FastPairDiscoverableScanner::NotifyDeviceFound(FastPairDevice& device) { NEARBY_LOGS(VERBOSE) << "Notify Device found:" << "BluetoothAddress = " << device.GetBleAddress() << ", Model id = " << device.GetModelId(); + { + MutexLock lock(&mutex_); + notified_devices_[device.GetBleAddress()] = &device; + } found_callback_(device); } void FastPairDiscoverableScanner::OnDeviceLost( const BlePeripheral& peripheral) { NEARBY_LOGS(INFO) << __func__ << ": Running lost callback"; + { + MutexLock lock(&mutex_); + auto node = notified_devices_.extract(peripheral.GetName()); + // Don't invoke callback if we didn't notify this device. + if (node.empty()) return; + } executor_->Execute("device-lost", [this, address = peripheral.GetName()]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { auto opt_device = device_repository_->FindDevice(address); - - // Don't invoke callback if we didn't notify this - // device. if (!opt_device.has_value()) return; FastPairDevice* device = opt_device.value(); lost_callback_(*device); diff --git a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.h b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.h index c53243e0..69606136 100644 --- a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.h +++ b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner.h @@ -26,7 +26,7 @@ #include "fastpair/scanning/fastpair/fast_pair_scanner.h" #include "internal/base/observer_list.h" #include "internal/platform/bluetooth_adapter.h" -#include "internal/platform/logging.h" +#include "internal/platform/mutex.h" #include "internal/platform/single_thread_executor.h" namespace nearby { @@ -84,10 +84,13 @@ class FastPairDiscoverableScanner : public FastPairScanner::Observer { void NotifyDeviceFound(FastPairDevice& device) ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); + Mutex mutex_; FastPairScanner& scanner_; DiscoverableScannerCallback found_callback_ ABSL_GUARDED_BY(*executor_); DiscoverableScannerCallback lost_callback_ ABSL_GUARDED_BY(*executor_); SingleThreadExecutor* executor_; + absl::flat_hash_map notified_devices_ + ABSL_GUARDED_BY(mutex_); FastPairDeviceRepository* device_repository_ ABSL_GUARDED_BY(*executor_); ObserverList observer_list_; }; diff --git a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_test.cc b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_test.cc index cc639a10..f487e463 100644 --- a/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_test.cc +++ b/fastpair/scanning/fastpair/fast_pair_discoverable_scanner_test.cc @@ -309,8 +309,6 @@ TEST_F(FastPairDiscoverableScannerTest, std::make_unique(kTestBleDeviceAddress, kValidModelId); scanner_->NotifyDeviceLost(BlePeripheral(ble_peripheral.get())); EXPECT_FALSE(lost_notification.WaitForNotificationWithTimeout(kWaitTimeout)); - scanner_->NotifyDeviceLost(BlePeripheral(ble_peripheral.get())); - EXPECT_FALSE(lost_notification.WaitForNotificationWithTimeout(kWaitTimeout)); } } // namespace diff --git a/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.cc b/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.cc index 43148584..009ab87e 100644 --- a/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.cc +++ b/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.cc @@ -169,20 +169,27 @@ void FastPairNonDiscoverableScanner::NotifyDeviceFound(FastPairDevice& device) { NEARBY_LOGS(VERBOSE) << "Notify Device found:" << "BluetoothAddress = " << device.GetBleAddress() << ", Model id = " << device.GetModelId(); + { + MutexLock lock(&mutex_); + notified_devices_[device.GetBleAddress()] = &device; + } found_callback_(device); } void FastPairNonDiscoverableScanner::OnDeviceLost( const BlePeripheral& peripheral) { NEARBY_LOGS(INFO) << __func__ << ": Running lost callback"; + { + MutexLock lock(&mutex_); + auto node = notified_devices_.extract(peripheral.GetName()); + // Don't invoke callback if we didn't notify this device. + if (node.empty()) return; + } executor_->Execute("device-lost", [this, address = peripheral.GetName()]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { auto opt_device = device_repository_->FindDevice(address); - - // Don't invoke callback if we didn't notify this - // device. if (!opt_device.has_value()) return; FastPairDevice* device = opt_device.value(); lost_callback_(*device); diff --git a/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.h b/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.h index aa38e825..1db0c2cc 100644 --- a/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.h +++ b/fastpair/scanning/fastpair/fast_pair_non_discoverable_scanner.h @@ -24,6 +24,7 @@ #include "fastpair/repository/fast_pair_device_repository.h" #include "fastpair/scanning/fastpair/fast_pair_scanner.h" #include "internal/base/observer_list.h" +#include "internal/platform/mutex.h" #include "internal/platform/single_thread_executor.h" namespace nearby { @@ -90,10 +91,13 @@ class FastPairNonDiscoverableScanner : public FastPairScanner::Observer { void NotifyDeviceFound(FastPairDevice& device) ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); + Mutex mutex_; FastPairScanner& scanner_; NonDiscoverableScannerCallback found_callback_ ABSL_GUARDED_BY(*executor_); NonDiscoverableScannerCallback lost_callback_ ABSL_GUARDED_BY(*executor_); SingleThreadExecutor* executor_; + absl::flat_hash_map notified_devices_ + ABSL_GUARDED_BY(mutex_); FastPairDeviceRepository* device_repository_ ABSL_GUARDED_BY(*executor_); ObserverList observer_list_; }; diff --git a/fastpair/scanning/scanner_broker_impl_test.cc b/fastpair/scanning/scanner_broker_impl_test.cc index 58f3eb1c..6e00d9a4 100644 --- a/fastpair/scanning/scanner_broker_impl_test.cc +++ b/fastpair/scanning/scanner_broker_impl_test.cc @@ -26,16 +26,17 @@ #include "fastpair/internal/mediums/mediums.h" #include "fastpair/proto/fastpair_rpcs.proto.h" #include "fastpair/repository/fake_fast_pair_repository.h" +#include "fastpair/repository/fast_pair_device_repository.h" #include "fastpair/scanning/scanner_broker.h" #include "fastpair/testing/fast_pair_service_data_creator.h" #include "internal/platform/byte_array.h" #include "internal/platform/count_down_latch.h" #include "internal/platform/medium_environment.h" +#include "internal/platform/single_thread_executor.h" namespace nearby { namespace fastpair { namespace { -constexpr absl::Duration kTaskWaitTimeout = absl::Milliseconds(1000); constexpr int kNotDiscoverableAdvHeader = 0b00000110; constexpr int kAccountKeyFilterHeader = 0b01100000; constexpr int kSaltHeader = 0b00010001; @@ -70,38 +71,62 @@ class ScannerBrokerObserver : public ScannerBroker::Observer { CountDownLatch* lost_latch_ = nullptr; }; +class MediumEnvironmentStarter { + public: + MediumEnvironmentStarter() { MediumEnvironment::Instance().Start(); } + ~MediumEnvironmentStarter() { MediumEnvironment::Instance().Stop(); } +}; + class ScannerBrokerImplTest : public testing::Test { protected: - MediumEnvironment& env_{MediumEnvironment::Instance()}; + void SetUp() override { + MediumEnvironment::Instance().Start(); + advertiser_ble_address_ = + mediums_advertiser_.GetBle().GetMedium().GetAdapter().GetMacAddress(); + } + + void TearDown() override { MediumEnvironment::Instance().Stop(); } + + // The medium environment must be initialized (started) + // before registering medium. + MediumEnvironmentStarter env_; + Mediums mediums_scanner_; + Mediums mediums_advertiser_; + std::string advertiser_ble_address_; }; TEST_F(ScannerBrokerImplTest, FoundDiscoverableAdvertisement) { - env_.Start(); + SingleThreadExecutor executor; + FastPairDeviceRepository devices{&executor}; + // Setup FakeFastPairRepository std::string decoded_key; absl::Base64Unescape(kPublicAntiSpoof, &decoded_key); - SingleThreadExecutor executor; - FastPairDeviceRepository devices(&executor); proto::Device metadata; auto repository_ = std::make_unique(); metadata.mutable_anti_spoofing_key_pair()->set_public_key(decoded_key); repository_->SetFakeMetadata(kModelId, metadata); - // Create Fast Pair Scanner and add its observer - Mediums mediums_1; - auto scanner_broker = - std::make_unique(mediums_1, &executor, &devices); + // Create Scanner and ScannerBrokerObserver + auto scanner_broker = std::make_unique( + mediums_scanner_, &executor, &devices); CountDownLatch accept_latch(1); CountDownLatch lost_latch(1); + CountDownLatch device_removed(1); + FastPairDeviceRepository::RemoveDeviceCallback callback = + [&](const FastPairDevice& device) { + EXPECT_EQ(device.GetBleAddress(), advertiser_ble_address_); + device_removed.CountDown(); + }; + devices.AddObserver(&callback); ScannerBrokerObserver observer(scanner_broker.get(), &accept_latch, &lost_latch); // Create Advertiser and startAdvertising - Mediums mediums_2; std::string service_id(kServiceID); ByteArray advertisement_bytes{absl::HexStringToBytes(kModelId)}; std::string fast_pair_service_uuid(kFastPairServiceUuid); - mediums_2.GetBle().GetMedium().StartAdvertising( + mediums_advertiser_.GetBle().GetMedium().StartAdvertising( service_id, advertisement_bytes, fast_pair_service_uuid); // Fast Pair scanner startScanning @@ -109,38 +134,44 @@ TEST_F(ScannerBrokerImplTest, FoundDiscoverableAdvertisement) { scanner_broker->StartScanning(Protocol::kFastPairInitialPairing); // Notify device found - EXPECT_TRUE(accept_latch.Await(kTaskWaitTimeout).result()); + accept_latch.Await(); // Advertiser stopAdvertising - mediums_2.GetBle().GetMedium().StopAdvertising(service_id); + mediums_advertiser_.GetBle().GetMedium().StopAdvertising(service_id); // Notify device lost - EXPECT_TRUE(lost_latch.Await(kTaskWaitTimeout).result()); + lost_latch.Await(); + device_removed.Await(); scanning_session.reset(); - env_.Stop(); } TEST_F(ScannerBrokerImplTest, FoundNonDiscoverableAdvertisement) { - env_.Start(); SingleThreadExecutor executor; - FastPairDeviceRepository devices(&executor); + FastPairDeviceRepository devices{&executor}; + + // Setup FakeFastPairRepository auto repository = std::make_unique(); proto::Device metadata; repository->SetFakeMetadata(kModelId, metadata); repository->SetResultOfCheckIfAssociatedWithCurrentAccount(AccountKey(), kModelId); - // Create Fast Pair Scanner and add its observer - Mediums mediums_1; - auto scanner_broker = - std::make_unique(mediums_1, &executor, &devices); + // Create Scanner and ScannerBrokerObserver + auto scanner_broker = std::make_unique( + mediums_scanner_, &executor, &devices); CountDownLatch accept_latch(1); CountDownLatch lost_latch(1); + CountDownLatch device_removed(1); + FastPairDeviceRepository::RemoveDeviceCallback callback = + [&](const FastPairDevice& device) { + EXPECT_EQ(device.GetBleAddress(), advertiser_ble_address_); + device_removed.CountDown(); + }; + devices.AddObserver(&callback); ScannerBrokerObserver observer(scanner_broker.get(), &accept_latch, &lost_latch); // Create Advertiser and startAdvertising - Mediums mediums_2; std::string service_id(kServiceID); std::vector service_data = FastPairServiceDataCreator::Builder() @@ -155,7 +186,7 @@ TEST_F(ScannerBrokerImplTest, FoundNonDiscoverableAdvertisement) { ByteArray advertisement_bytes( std::string(service_data.begin(), service_data.end())); std::string fast_pair_service_uuid(kFastPairServiceUuid); - mediums_2.GetBle().GetMedium().StartAdvertising( + mediums_advertiser_.GetBle().GetMedium().StartAdvertising( service_id, advertisement_bytes, fast_pair_service_uuid); // Fast Pair scanner startScanning @@ -163,16 +194,17 @@ TEST_F(ScannerBrokerImplTest, FoundNonDiscoverableAdvertisement) { scanner_broker->StartScanning(Protocol::kFastPairInitialPairing); // Notify device found - EXPECT_TRUE(accept_latch.Await(kTaskWaitTimeout).result()); + accept_latch.Await(); // Advertiser stopAdvertising - mediums_2.GetBle().GetMedium().StopAdvertising(service_id); + mediums_advertiser_.GetBle().GetMedium().StopAdvertising(service_id); // Notify device lost - EXPECT_TRUE(lost_latch.Await(kTaskWaitTimeout).result()); + lost_latch.Await(); + device_removed.Await(); scanning_session.reset(); - env_.Stop(); } + } // namespace } // namespace fastpair } // namespace nearby