[media][tests] Eliminate flaky audio_fidl_tests behavior
By performing the initial connect-to-Audio-service in a
global (programwide) test set-up phase, we ensure that audio
components have been loaded and are available for normal
operation before testing starts. Today, when a test runner/
environment is highly loaded, we run a chance that by the
time audio_fidl_tests starts running tests, Amber may not
yet have finished loading the Audio and AudioCore components
onto the target machine. When this wait bleeds over into the
first test cases, their initial assumptions may be violated
(they may not be resilient to an initial delay of 100
milliseconds before getting their first response).
Over time these test cases will be rewritten to eliminate
any remaining aspects of "Success == No Response", so that
they can essentially use infinite waits. These places in the
code have been marked by TODO. Between now and that day,
this is an effective and reasonable solution.
In addition to establishing the SetUp/TearDown of the
overall test environment, this CL also introduces one
instance of TestSuite (class) level SetUp/TearDown, in the
AudioRendererSync test set. If in the future the execution
of test programs is sharded to different machines, we can
still expect each TestSuite to be executed on a single
machine; this type of class-level initial setup is how we
would need to solve the problem at that point.
BUG: FLK-47 #done
TEST: build, CQ, overnight loop on NUC, arm64 QEMU and x64 QEMU
Change-Id: I345e20f5377eaa95a7d6b743c66cd670e7379090
diff --git a/garnet/bin/media/audio_core/test/BUILD.gn b/garnet/bin/media/audio_core/test/BUILD.gn
index 4746f74..8c5801a 100644
--- a/garnet/bin/media/audio_core/test/BUILD.gn
+++ b/garnet/bin/media/audio_core/test/BUILD.gn
@@ -1,4 +1,4 @@
-# Copyright 2016 The Fuchsia Authors. All rights reserved.
+# Copyright 2018 The Fuchsia Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
@@ -11,7 +11,7 @@
sources = [
"audio_capturer_test.cc",
- "audio_fidl_tests.cc",
+ "audio_fidl_main.cc",
"audio_renderer_sync_test.cc",
"audio_renderer_test.cc",
"audio_sync_test.cc",
@@ -25,7 +25,6 @@
"//garnet/public/lib/component/cpp/testing",
"//garnet/public/lib/gtest",
"//sdk/fidl/fuchsia.media",
- "//third_party/googletest:gtest_main",
]
}
@@ -47,16 +46,16 @@
output_name = "audio_device_tests"
sources = [
+ "audio_device_main.cc",
"audio_device_test.cc",
"audio_device_test.h",
"audio_tests_shared.h",
]
deps = [
- "//sdk/fidl/fuchsia.media",
"//garnet/public/lib/component/cpp/testing",
"//garnet/public/lib/gtest",
- "//third_party/googletest:gtest_main",
+ "//sdk/fidl/fuchsia.media",
]
}
diff --git a/garnet/bin/media/audio_core/test/audio_capturer_test.cc b/garnet/bin/media/audio_core/test/audio_capturer_test.cc
index f486fdc..4cd5b1c5 100644
--- a/garnet/bin/media/audio_core/test/audio_capturer_test.cc
+++ b/garnet/bin/media/audio_core/test/audio_capturer_test.cc
@@ -84,6 +84,7 @@
return return_val;
}
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool AudioCapturerTest::ExpectTimeout() {
received_callback_ = false;
@@ -159,8 +160,7 @@
// This test case verifies the scenario #1 above.
// TODO(mpuryear): test sequence of pkt return, during Async capture.
//
-// TODO(FLK-47): Test is disabled because it is flaky.
-TEST_F(AudioCapturerTest, DISABLED_DiscardAllWithNone) {
+TEST_F(AudioCapturerTest, DiscardAllWithNone) {
SetNegativeExpectations();
audio_capturer_->DiscardAllPackets([this]() { received_callback_ = true; });
@@ -215,6 +215,8 @@
// Test creation and interface independence of GainControl.
// In a number of tests below, we run the message loop to give the AudioCapturer
// or GainControl binding a chance to disconnect, if an error occurred.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
TEST_F(AudioCapturerTest, BindGainControl) {
// Validate AudioCapturers can create GainControl interfaces.
audio_capturer_->BindGainControl(gain_control_.NewRequest());
diff --git a/garnet/bin/media/audio_core/test/audio_device_main.cc b/garnet/bin/media/audio_core/test/audio_device_main.cc
new file mode 100644
index 0000000..074d5d4
--- /dev/null
+++ b/garnet/bin/media/audio_core/test/audio_device_main.cc
@@ -0,0 +1,66 @@
+// Copyright 2019 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <fuchsia/media/cpp/fidl.h>
+
+#include "garnet/bin/media/audio_core/test/audio_device_test.h"
+#include "gtest/gtest.h"
+#include "lib/component/cpp/environment_services_helper.h"
+
+namespace media::audio::test {
+
+class AudioDeviceEnvironment : public ::testing::Environment {
+ public:
+ // Before any test cases in this program, synchronously connect to the service
+ // to ensure that the audio and audio_core components are present and loaded.
+ void SetUp() override {
+ // This is an unchanging input for the entire component; get it once here.
+ auto environment_services = component::GetEnvironmentServices();
+
+ // Unlike environment_services, each test case creates fresh FIDL instances.
+ // In this one-time setup code we use a temp local var instance: it merely
+ // "demand-pages" other components and is not subsequently referenced.
+ //
+ // Note that we are using the Synchronous version of this interface....
+ fuchsia::media::AudioDeviceEnumeratorSyncPtr audio_dev_enum_sync;
+ environment_services->ConnectToService(audio_dev_enum_sync.NewRequest());
+
+ // This FIDL method has a callback; calling it SYNCHRONOUSLY guarantees
+ // that services are loaded and running before the method itself returns.
+ //
+ // This is not the case for sync calls without callback (nor async calls),
+ // because of the pipelining inherent in FIDL's design.
+ uint64_t default_input;
+ bool connected_to_audio_device_enumerator_service =
+ (audio_dev_enum_sync->GetDefaultInputDevice(&default_input) == ZX_OK);
+
+ // On assert-false, no test cases run, and they may display as passed.
+ // However, the overall binary returns non-zero (fail).
+ ASSERT_TRUE(connected_to_audio_device_enumerator_service);
+
+ // Save this for all test cases to use
+ AudioDeviceTest::SetEnvironmentServices(environment_services);
+ }
+
+ ///// If needed, these (overriding) functions would also need to be public.
+ // void TearDown() override {}
+ // ~AudioFidlEnvironment() override {}
+};
+
+} // namespace media::audio::test
+
+int main(int argc, char** argv) {
+ ::testing::InitGoogleTest(&argc, argv);
+
+ // gtest takes ownership of registered environments: **do not delete them**!
+ ::testing::AddGlobalTestEnvironment(
+ new ::media::audio::test::AudioDeviceEnvironment);
+
+ // TODO(mpuryear): create and use a '--stress' switch here, to execute a set
+ // of longhaul resource-exhaustion-focused tests on these interfaces.
+
+ int result = RUN_ALL_TESTS();
+
+ return result;
+}
diff --git a/garnet/bin/media/audio_core/test/audio_device_test.cc b/garnet/bin/media/audio_core/test/audio_device_test.cc
index 041c973..ddadf9f 100644
--- a/garnet/bin/media/audio_core/test/audio_device_test.cc
+++ b/garnet/bin/media/audio_core/test/audio_device_test.cc
@@ -17,18 +17,10 @@
namespace media::audio::test {
//
-// AudioDeviceEnumeratorSync test cases (smoke test)
+// AudioDeviceTest static variables
//
-// Synchronously connect; verify interface is alive.
-TEST(AudioDeviceSyncTest, Connect) {
- fuchsia::media::AudioDeviceEnumeratorSyncPtr audio_dev_enum_sync;
-
- auto environment_services = component::GetEnvironmentServices();
- environment_services->ConnectToService(audio_dev_enum_sync.NewRequest());
-
- uint64_t default_input;
- ASSERT_EQ(ZX_OK, audio_dev_enum_sync->GetDefaultInputDevice(&default_input));
-}
+std::shared_ptr<const ::component::Services>
+ AudioDeviceTest::environment_services_;
uint16_t AudioDeviceTest::initial_input_device_count_ = kInvalidDeviceCount;
uint16_t AudioDeviceTest::initial_output_device_count_ = kInvalidDeviceCount;
@@ -46,7 +38,6 @@
::gtest::RealLoopFixture::SetUp();
auto err_handler = [this](zx_status_t error) { error_occurred_ = true; };
- environment_services_ = component::GetEnvironmentServices();
environment_services_->ConnectToService(audio_dev_enum_.NewRequest());
audio_dev_enum_.set_error_handler(err_handler);
@@ -83,10 +74,8 @@
return return_val;
}
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool AudioDeviceTest::ExpectTimeout() {
- EXPECT_EQ(received_old_token_, kInvalidDeviceToken) << "Before ExpectTimeout";
- EXPECT_TRUE(audio_dev_enum_.is_bound());
-
received_callback_ = false;
received_device_ = kInvalidDeviceInfo;
received_removed_token_ = kInvalidDeviceToken;
@@ -111,6 +100,8 @@
<< "Received Remove event";
EXPECT_EQ(received_default_token_, kInvalidDeviceToken)
<< "Received Default event";
+ EXPECT_EQ(received_old_token_, kInvalidDeviceToken)
+ << "Received Default event";
EXPECT_EQ(received_gain_token_, kInvalidDeviceToken)
<< "Received Gain event";
}
@@ -121,8 +112,6 @@
}
void AudioDeviceTest::SetOnDeviceAddedEvent() {
- received_device_ = kInvalidDeviceInfo;
-
audio_dev_enum_.events().OnDeviceAdded =
[this](fuchsia::media::AudioDeviceInfo dev) {
received_callback_ = true;
@@ -131,8 +120,6 @@
}
void AudioDeviceTest::SetOnDeviceRemovedEvent() {
- received_removed_token_ = kInvalidDeviceToken;
-
audio_dev_enum_.events().OnDeviceRemoved = [this](uint64_t token_id) {
received_callback_ = true;
received_removed_token_ = token_id;
@@ -140,9 +127,6 @@
}
void AudioDeviceTest::SetOnDeviceGainChangedEvent() {
- received_gain_token_ = kInvalidDeviceToken;
- received_gain_info_ = kInvalidGainInfo;
-
audio_dev_enum_.events().OnDeviceGainChanged =
[this](uint64_t dev_token, fuchsia::media::AudioGainInfo dev_gain_info) {
received_callback_ = true;
@@ -152,9 +136,6 @@
}
void AudioDeviceTest::SetOnDefaultDeviceChangedEvent() {
- received_default_token_ = kInvalidDeviceToken;
- received_old_token_ = kInvalidDeviceToken;
-
audio_dev_enum_.events().OnDefaultDeviceChanged =
[this](uint64_t old_default_token, uint64_t new_default_token) {
received_callback_ = true;
diff --git a/garnet/bin/media/audio_core/test/audio_device_test.h b/garnet/bin/media/audio_core/test/audio_device_test.h
index cdcafbd..cd76eba 100644
--- a/garnet/bin/media/audio_core/test/audio_device_test.h
+++ b/garnet/bin/media/audio_core/test/audio_device_test.h
@@ -33,6 +33,12 @@
.is_default = true};
class AudioDeviceTest : public gtest::RealLoopFixture {
+ public:
+ static void SetEnvironmentServices(
+ std::shared_ptr<const ::component::Services> environment_services) {
+ environment_services_ = environment_services;
+ }
+
protected:
void SetUp() override;
void TearDown() override;
@@ -65,7 +71,7 @@
static uint32_t initial_input_gain_flags_;
static uint32_t initial_output_gain_flags_;
- std::shared_ptr<component::Services> environment_services_;
+ static std::shared_ptr<const ::component::Services> environment_services_;
fuchsia::media::AudioDeviceEnumeratorPtr audio_dev_enum_;
bool error_occurred_ = false;
diff --git a/garnet/bin/media/audio_core/test/audio_fidl_main.cc b/garnet/bin/media/audio_core/test/audio_fidl_main.cc
new file mode 100644
index 0000000..a05f40b
--- /dev/null
+++ b/garnet/bin/media/audio_core/test/audio_fidl_main.cc
@@ -0,0 +1,65 @@
+// Copyright 2018 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <fuchsia/media/cpp/fidl.h>
+
+#include "garnet/bin/media/audio_core/test/audio_tests_shared.h"
+#include "gtest/gtest.h"
+#include "lib/component/cpp/environment_services_helper.h"
+#include "lib/fxl/logging.h"
+
+namespace media::audio::test {
+
+class AudioFidlEnvironment : public ::testing::Environment {
+ public:
+ // Before any test cases in this test program, synchronously connect to Audio,
+ // to ensure that the audio and audio_core components are present and loaded.
+ void SetUp() override {
+ auto environment_services = component::GetEnvironmentServices();
+
+ // Each test case creates fresh FIDL instances. This one-time setup code
+ // uses a temp local var instance to "demand-page" other components and does
+ // not subsequently reference it.
+ fuchsia::media::AudioSyncPtr audio;
+ environment_services->ConnectToService(audio.NewRequest());
+
+ // Note that we are using Synchronous versions of these interfaces....
+ fuchsia::media::AudioRendererSyncPtr audio_renderer;
+ audio->CreateAudioRenderer(audio_renderer.NewRequest());
+
+ // This FIDL method has a callback; calling it SYNCHRONOUSLY guarantees
+ // that services are loaded and running before the method itself returns.
+ //
+ // This is not the case for sync calls WITHOUT callback (nor async calls),
+ // because of the pipelining inherent in FIDL's design.
+ zx_duration_t lead_time;
+ bool connected_to_audio_service =
+ (audio_renderer->GetMinLeadTime(&lead_time) == ZX_OK);
+
+ // On assert-false, no test cases run, and they may display as passed.
+ // However, the overall binary returns non-zero (fail).
+ ASSERT_TRUE(connected_to_audio_service);
+ }
+
+ ///// If needed, these (overriding) functions would also need to be public.
+ // void TearDown() override {}
+ // ~AudioFidlEnvironment() override {}
+};
+
+} // namespace media::audio::test
+
+int main(int argc, char** argv) {
+ ::testing::InitGoogleTest(&argc, argv);
+
+ // gtest takes ownership of registered environments: **do not delete them**!
+ ::testing::AddGlobalTestEnvironment(
+ new ::media::audio::test::AudioFidlEnvironment);
+
+ // TODO(mpuryear): create and use a '--stress' switch here, to execute a set
+ // of longhaul resource-exhaustion-focused tests on these interfaces.
+
+ int result = RUN_ALL_TESTS();
+
+ return result;
+}
diff --git a/garnet/bin/media/audio_core/test/audio_fidl_tests.cc b/garnet/bin/media/audio_core/test/audio_fidl_tests.cc
deleted file mode 100644
index f7c6557..0000000
--- a/garnet/bin/media/audio_core/test/audio_fidl_tests.cc
+++ /dev/null
@@ -1,16 +0,0 @@
-// Copyright 2018 The Fuchsia Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "gtest/gtest.h"
-
-int main(int argc, char** argv) {
- testing::InitGoogleTest(&argc, argv);
-
- // TODO(mpuryear): create a '--stress' switch here, to execute a set of
- // longhaul resource-exhaustion-focused tests on these interfaces.
-
- int result = RUN_ALL_TESTS();
-
- return result;
-}
diff --git a/garnet/bin/media/audio_core/test/audio_renderer_sync_test.cc b/garnet/bin/media/audio_core/test/audio_renderer_sync_test.cc
index caac04a..e41949b 100644
--- a/garnet/bin/media/audio_core/test/audio_renderer_sync_test.cc
+++ b/garnet/bin/media/audio_core/test/audio_renderer_sync_test.cc
@@ -23,23 +23,37 @@
// done on the async interfaces) should not be needed.
class AudioRendererSyncTest : public gtest::RealLoopFixture {
protected:
+ // "Regional" per-test-suite set-up. Called before first test in this suite.
+ static void SetUpTestSuite() {
+ std::shared_ptr<component::Services> environment_services =
+ component::GetEnvironmentServices();
+ environment_services->ConnectToService(audio_sync_.NewRequest());
+ }
+
+ // Per-test-suite tear-down. Called after last test in this suite.
+ static void TearDownTestSuite() { audio_sync_.Unbind(); }
+
void SetUp() override {
::gtest::RealLoopFixture::SetUp();
- environment_services_ = component::GetEnvironmentServices();
- environment_services_->ConnectToService(audio_sync_.NewRequest());
- ASSERT_TRUE(audio_sync_.is_bound());
-
ASSERT_EQ(ZX_OK, audio_sync_->CreateAudioRenderer(
audio_renderer_sync_.NewRequest()));
- ASSERT_TRUE(audio_renderer_sync_.is_bound());
}
- std::shared_ptr<component::Services> environment_services_;
- fuchsia::media::AudioSyncPtr audio_sync_;
+ void TearDown() override {
+ audio_renderer_sync_.Unbind();
+ ::gtest::RealLoopFixture::TearDown();
+ }
+
+ // Declare singleton resource shared by all test cases.
+ static fuchsia::media::AudioSyncPtr audio_sync_;
+
+ // One of these is created anew, for each test case.
fuchsia::media::AudioRendererSyncPtr audio_renderer_sync_;
};
+fuchsia::media::AudioSyncPtr AudioRendererSyncTest::audio_sync_ = nullptr;
+
//
// AudioRendererSync validation
//
diff --git a/garnet/bin/media/audio_core/test/audio_renderer_test.cc b/garnet/bin/media/audio_core/test/audio_renderer_test.cc
index b0f97c7..0409cc7 100644
--- a/garnet/bin/media/audio_core/test/audio_renderer_test.cc
+++ b/garnet/bin/media/audio_core/test/audio_renderer_test.cc
@@ -131,6 +131,8 @@
// Before renderers are operational, multiple SetPcmStreamTypes should succeed.
// We test twice because of previous bug, where the first succeeded but any
// subsequent call (before Play) would cause a FIDL channel disconnect.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
TEST_F(AudioRendererTest, SetPcmStreamType) {
fuchsia::media::AudioStreamType format;
format.sample_format = fuchsia::media::AudioSampleFormat::FLOAT;
@@ -202,6 +204,8 @@
}
// Validate MinLeadTime events, when disabled.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
TEST_F(AudioRendererTest, DisableMinLeadTimeEvents) {
int64_t min_lead_time = -1;
audio_renderer_.events().OnMinLeadTimeChanged =
@@ -244,6 +248,8 @@
// Test creation and interface independence of GainControl.
// In a number of tests below, we run the message loop to give the AudioRenderer
// or GainControl binding a chance to disconnect, if an error occurred.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
TEST_F(AudioRendererTest, BindGainControl) {
// Validate AudioRenderers can create GainControl interfaces.
audio_renderer_->BindGainControl(gain_control_.NewRequest());
diff --git a/garnet/bin/media/audio_core/test/audio_test.cc b/garnet/bin/media/audio_core/test/audio_test.cc
index 9746f2c3..e25bb83 100644
--- a/garnet/bin/media/audio_core/test/audio_test.cc
+++ b/garnet/bin/media/audio_core/test/audio_test.cc
@@ -76,6 +76,8 @@
}
// Expecting NOT to receive a disconnect. Wait, then check for errors.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool AudioBase::ReceiveNoDisconnectCallback() {
bool timed_out = !RunLoopWithTimeoutOrUntil(
[this]() { return error_occurred_; }, kDurationTimeoutExpected);
@@ -93,6 +95,7 @@
void AudioTest::SetUp() {
AudioBase::SetUp();
+ // TODO(mpuryear): Refactor to eliminate "wait for nothing bad to happen".
ASSERT_TRUE(RunLoopWithTimeout(kDurationTimeoutExpected)) << kConnectionErr;
ASSERT_TRUE(audio_.is_bound());
}
@@ -209,6 +212,8 @@
// 2. Audio persists after created AudioRenderer is destroyed.
// 3. AudioRenderer persists after Audio is destroyed.
// 4. Asynchronous Audio can create synchronous AudioCapturers, too.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
TEST_F(AudioTest, CreateAudioRenderer) {
auto err_handler = [this](zx_status_t error) { error_occurred_ = true; };
@@ -296,6 +301,8 @@
// 2. Audio persists after created AudioCapturer is destroyed.
// 3. AudioCapturer persists after Audio is destroyed.
// 4. Asynchronous Audio can create synchronous AudioCapturers, too.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
TEST_F(AudioTest, CreateAudioCapturer) {
auto err_handler = [this](zx_status_t error) { error_occurred_ = true; };
diff --git a/garnet/bin/media/audio_core/test/audio_tests_shared.h b/garnet/bin/media/audio_core/test/audio_tests_shared.h
index fa8e177..1035a177 100644
--- a/garnet/bin/media/audio_core/test/audio_tests_shared.h
+++ b/garnet/bin/media/audio_core/test/audio_tests_shared.h
@@ -27,6 +27,7 @@
// by checking more frequently than the default 10 ms. The kDurationGranularity
// constant should only be used in conjunction with kDurationResponseExpected.
//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
constexpr zx::duration kDurationResponseExpected = zx::sec(5);
constexpr zx::duration kDurationTimeoutExpected = zx::msec(100);
constexpr zx::duration kDurationGranularity = zx::msec(1);
diff --git a/garnet/bin/media/audio_core/test/gain_control_test.cc b/garnet/bin/media/audio_core/test/gain_control_test.cc
index 0264dc7..025b9da 100644
--- a/garnet/bin/media/audio_core/test/gain_control_test.cc
+++ b/garnet/bin/media/audio_core/test/gain_control_test.cc
@@ -185,6 +185,8 @@
}
// Tests expect to receive neither gain callback nor error; assert this.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool GainControlTestBase::ReceiveNoGainCallback() {
received_gain_callback_ = false;
@@ -421,6 +423,8 @@
}
// Tests expect neither gain interface to receive gain callback nor error.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool SiblingGainControlsTest::ReceiveNoGainCallback() {
received_gain_callback_ = received_gain_callback_2_ = false;
@@ -541,6 +545,8 @@
// Tests expect a gain callback and no error, and neither on the independent
// API binding and gain_control (thus we wait for timeout below).
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool IndependentGainControlsTest::ReceiveGainCallback(float gain_db,
bool mute) {
received_gain_callback_ = received_gain_callback_2_ = false;
@@ -574,6 +580,8 @@
}
// Tests expect to receive neither gain callback nor error, on both gains.
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool IndependentGainControlsTest::ReceiveNoGainCallback() {
received_gain_callback_ = received_gain_callback_2_ = false;
@@ -604,6 +612,8 @@
// generates a gain callback, this is unexpected and treated as an error. We
// still expect nothing from the independent API binding and its gain_control
// (thus we wait for timeout).
+//
+// TODO(mpuryear): Refactor tests to eliminate "wait for nothing bad to happen".
bool IndependentGainControlsTest::ReceiveDisconnectCallback() {
received_gain_callback_ = received_gain_callback_2_ = false;
