From ac0680beb35dff6baef3d76e23cdfa393a91eba3 Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Thu, 20 Feb 2020 13:04:17 -0800 Subject: [PATCH] Clean up dumpstate HAL 1.1 VTS test. We separate out per-mode tests into a separate test suite for better extensibility. If a new DumpstateMode value is added to 1.1, it will no longer require updating a macro in the test. All tests still run the same and if they're per-mode, still include the actual mode string in their final name. No material behavior changes. Test: atest VtsHalDumpstateV1_1TargetTest on cuttlefish Change-Id: Ifefae981705e564c10ee450851d3d2320f8206f3 --- .../VtsHalDumpstateV1_1TargetTest.cpp | 159 ++++++++++-------- 1 file changed, 90 insertions(+), 69 deletions(-) diff --git a/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp b/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp index cbdd87bc78..1bef66358a 100644 --- a/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp +++ b/dumpstate/1.1/vts/functional/VtsHalDumpstateV1_1TargetTest.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -27,6 +28,7 @@ #include #include #include +#include #include #include @@ -39,13 +41,18 @@ using ::android::hardware::dumpstate::V1_1::DumpstateStatus; using ::android::hardware::dumpstate::V1_1::IDumpstateDevice; using ::android::hardware::dumpstate::V1_1::toString; -class DumpstateHidl1_1Test : public ::testing::TestWithParam { +// Base class common to all dumpstate HAL v1.1 tests. +template +class DumpstateHidl1_1TestBase : public ::testing::TestWithParam { protected: virtual void SetUp() override { GetService(); } + virtual std::string GetInstanceName() = 0; + void GetService() { - dumpstate = IDumpstateDevice::getService(GetParam()); - ASSERT_NE(dumpstate, nullptr) << "Could not get HIDL instance"; + const std::string instance_name = GetInstanceName(); + dumpstate = IDumpstateDevice::getService(instance_name); + ASSERT_NE(dumpstate, nullptr) << "Could not get HIDL instance " << instance_name; } void ToggleVerboseLogging(bool enable) { @@ -78,77 +85,76 @@ class DumpstateHidl1_1Test : public ::testing::TestWithParam { sp dumpstate; }; -#define TEST_FOR_DUMPSTATE_MODE(name, body, mode) \ - TEST_P(DumpstateHidl1_1Test, name##_##mode) { body(DumpstateMode::mode); } +// Tests that don't need to iterate every single DumpstateMode value for dumpstateBoard_1_1. +class DumpstateHidl1_1GeneralTest : public DumpstateHidl1_1TestBase { + protected: + virtual std::string GetInstanceName() override { return GetParam(); } +}; -// We use a macro to define individual test cases instead of hidl_enum_range<> because some HAL -// implementations are lazy and may call exit() at the end of dumpstateBoard(), which would cause -// DEAD_OBJECT errors after the first iteration. Separate cases re-get the service each time as part -// of SetUp(), and also provide better separation of concerns when specific modes are problematic. -#define TEST_FOR_ALL_DUMPSTATE_MODES(name, body) \ - TEST_FOR_DUMPSTATE_MODE(name, body, FULL); \ - TEST_FOR_DUMPSTATE_MODE(name, body, INTERACTIVE); \ - TEST_FOR_DUMPSTATE_MODE(name, body, REMOTE); \ - TEST_FOR_DUMPSTATE_MODE(name, body, WEAR); \ - TEST_FOR_DUMPSTATE_MODE(name, body, CONNECTIVITY); \ - TEST_FOR_DUMPSTATE_MODE(name, body, WIFI); \ - TEST_FOR_DUMPSTATE_MODE(name, body, DEFAULT); \ - TEST_FOR_DUMPSTATE_MODE(name, body, PROTO); +// Tests that iterate every single DumpstateMode value for dumpstateBoard_1_1. +class DumpstateHidl1_1PerModeTest + : public DumpstateHidl1_1TestBase> { + protected: + virtual std::string GetInstanceName() override { return std::get<0>(GetParam()); } + + DumpstateMode GetMode() { return std::get<1>(GetParam()); } + + // Will only execute additional_assertions when status == expected. + void AssertStatusForMode(const Return& status, const DumpstateStatus expected, + std::function additional_assertions = nullptr) { + ASSERT_TRUE(status.isOk()) + << "Status should be ok and return a more specific DumpstateStatus: " + << status.description(); + if (GetMode() == DumpstateMode::DEFAULT) { + ASSERT_EQ(expected, status) + << "Required mode (DumpstateMode::" << toString(GetMode()) + << "): status should be DumpstateStatus::" << toString(expected) + << ", but got DumpstateStatus::" << toString(status); + } else { + // The rest of the modes are optional to support, but they MUST return either the + // expected value or UNSUPPORTED_MODE. + ASSERT_TRUE(status == expected || status == DumpstateStatus::UNSUPPORTED_MODE) + << "Optional mode (DumpstateMode::" << toString(GetMode()) + << "): status should be DumpstateStatus::" << toString(expected) + << " or DumpstateStatus::UNSUPPORTED_MODE, but got DumpstateStatus::" + << toString(status); + } + if (status == expected && additional_assertions != nullptr) { + additional_assertions(); + } + } +}; constexpr uint64_t kDefaultTimeoutMillis = 30 * 1000; // 30 seconds -// Will only execute additional_assertions when status == expected. -void AssertStatusForMode(const DumpstateMode mode, const Return& status, - const DumpstateStatus expected, - std::function additional_assertions = nullptr) { - ASSERT_TRUE(status.isOk()) << "Status should be ok and return a more specific DumpstateStatus: " - << status.description(); - if (mode == DumpstateMode::DEFAULT) { - ASSERT_EQ(expected, status) << "Required mode (DumpstateMode::" << toString(mode) - << "): status should be DumpstateStatus::" << toString(expected) - << ", but got DumpstateStatus::" << toString(status); - } else { - // The rest of the modes are optional to support, but they MUST return either the expected - // value or UNSUPPORTED_MODE. - ASSERT_TRUE(status == expected || status == DumpstateStatus::UNSUPPORTED_MODE) - << "Optional mode (DumpstateMode::" << toString(mode) - << "): status should be DumpstateStatus::" << toString(expected) - << " or DumpstateStatus::UNSUPPORTED_MODE, but got DumpstateStatus::" - << toString(status); - } - if (status == expected && additional_assertions != nullptr) { - additional_assertions(); - } -} - // Negative test: make sure dumpstateBoard() doesn't crash when passed a null pointer. -TEST_FOR_ALL_DUMPSTATE_MODES(TestNullHandle, [this](DumpstateMode mode) { +TEST_P(DumpstateHidl1_1PerModeTest, TestNullHandle) { EnableVerboseLogging(); Return status = - dumpstate->dumpstateBoard_1_1(nullptr, mode, kDefaultTimeoutMillis); + dumpstate->dumpstateBoard_1_1(nullptr, GetMode(), kDefaultTimeoutMillis); - AssertStatusForMode(mode, status, DumpstateStatus::ILLEGAL_ARGUMENT); -}); + AssertStatusForMode(status, DumpstateStatus::ILLEGAL_ARGUMENT); +} // Negative test: make sure dumpstateBoard() ignores a handle with no FD. -TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithNoFd, [this](DumpstateMode mode) { +TEST_P(DumpstateHidl1_1PerModeTest, TestHandleWithNoFd) { EnableVerboseLogging(); native_handle_t* handle = native_handle_create(0, 0); ASSERT_NE(handle, nullptr) << "Could not create native_handle"; Return status = - dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + dumpstate->dumpstateBoard_1_1(handle, GetMode(), kDefaultTimeoutMillis); - AssertStatusForMode(mode, status, DumpstateStatus::ILLEGAL_ARGUMENT); + AssertStatusForMode(status, DumpstateStatus::ILLEGAL_ARGUMENT); native_handle_close(handle); native_handle_delete(handle); -}); +} // Positive test: make sure dumpstateBoard() writes something to the FD. -TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { +TEST_P(DumpstateHidl1_1PerModeTest, TestOk) { EnableVerboseLogging(); // Index 0 corresponds to the read end of the pipe; 1 to the write end. @@ -160,9 +166,9 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { handle->data[0] = fds[1]; Return status = - dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + dumpstate->dumpstateBoard_1_1(handle, GetMode(), kDefaultTimeoutMillis); - AssertStatusForMode(mode, status, DumpstateStatus::OK, [&fds]() { + AssertStatusForMode(status, DumpstateStatus::OK, [&fds]() { // Check that at least one byte was written. char buff; ASSERT_EQ(1, read(fds[0], &buff, 1)) << "Dumped nothing"; @@ -170,10 +176,10 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestOk, [this](DumpstateMode mode) { native_handle_close(handle); native_handle_delete(handle); -}); +} // Positive test: make sure dumpstateBoard() doesn't crash with two FDs. -TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { +TEST_P(DumpstateHidl1_1PerModeTest, TestHandleWithTwoFds) { EnableVerboseLogging(); int fds1[2]; @@ -187,9 +193,9 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { handle->data[1] = fds2[1]; Return status = - dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + dumpstate->dumpstateBoard_1_1(handle, GetMode(), kDefaultTimeoutMillis); - AssertStatusForMode(mode, status, DumpstateStatus::OK, [&fds1, &fds2]() { + AssertStatusForMode(status, DumpstateStatus::OK, [&fds1, &fds2]() { // Check that at least one byte was written to one of the FDs. char buff; size_t read1 = read(fds1[0], &buff, 1); @@ -200,10 +206,10 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestHandleWithTwoFds, [this](DumpstateMode mode) { native_handle_close(handle); native_handle_delete(handle); -}); +} // Make sure dumpstateBoard_1_1 actually validates its arguments. -TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) { +TEST_P(DumpstateHidl1_1GeneralTest, TestInvalidModeArgument_Negative) { EnableVerboseLogging(); int fds[2]; @@ -225,7 +231,7 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Negative) { native_handle_delete(handle); } -TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) { +TEST_P(DumpstateHidl1_1GeneralTest, TestInvalidModeArgument_Undefined) { EnableVerboseLogging(); int fds[2]; @@ -248,7 +254,7 @@ TEST_P(DumpstateHidl1_1Test, TestInvalidModeArgument_Undefined) { } // Positive test: make sure dumpstateBoard() from 1.0 doesn't fail. -TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) { +TEST_P(DumpstateHidl1_1GeneralTest, Test1_0MethodOk) { EnableVerboseLogging(); int fds[2]; @@ -272,7 +278,7 @@ TEST_P(DumpstateHidl1_1Test, Test1_0MethodOk) { // Make sure disabling verbose logging behaves correctly. Some info is still allowed to be emitted, // but it can't have privacy/storage/battery impacts. -TEST_FOR_ALL_DUMPSTATE_MODES(TestVerboseLoggingDisabled, [this](DumpstateMode mode) { +TEST_P(DumpstateHidl1_1PerModeTest, TestDeviceLoggingDisabled) { DisableVerboseLogging(); // Index 0 corresponds to the read end of the pipe; 1 to the write end. @@ -284,31 +290,31 @@ TEST_FOR_ALL_DUMPSTATE_MODES(TestVerboseLoggingDisabled, [this](DumpstateMode mo handle->data[0] = fds[1]; Return status = - dumpstate->dumpstateBoard_1_1(handle, mode, kDefaultTimeoutMillis); + dumpstate->dumpstateBoard_1_1(handle, GetMode(), kDefaultTimeoutMillis); // We don't include additional assertions here about the file passed in. If verbose logging is // disabled, the OEM may choose to include nothing at all, but it is allowed to include some // essential information based on the mode as long as it isn't private user information. - AssertStatusForMode(mode, status, DumpstateStatus::OK); + AssertStatusForMode(status, DumpstateStatus::OK); native_handle_close(handle); native_handle_delete(handle); -}); +} // Double-enable is perfectly valid, but the second call shouldn't do anything. -TEST_P(DumpstateHidl1_1Test, TestRepeatedEnable) { +TEST_P(DumpstateHidl1_1GeneralTest, TestRepeatedEnable) { EnableVerboseLogging(); EnableVerboseLogging(); } // Double-disable is perfectly valid, but the second call shouldn't do anything. -TEST_P(DumpstateHidl1_1Test, TestRepeatedDisable) { +TEST_P(DumpstateHidl1_1GeneralTest, TestRepeatedDisable) { DisableVerboseLogging(); DisableVerboseLogging(); } // Toggling in short order is perfectly valid. -TEST_P(DumpstateHidl1_1Test, TestRepeatedToggle) { +TEST_P(DumpstateHidl1_1GeneralTest, TestRepeatedToggle) { EnableVerboseLogging(); DisableVerboseLogging(); EnableVerboseLogging(); @@ -316,8 +322,23 @@ TEST_P(DumpstateHidl1_1Test, TestRepeatedToggle) { } INSTANTIATE_TEST_SUITE_P( - PerInstance, DumpstateHidl1_1Test, + PerInstance, DumpstateHidl1_1GeneralTest, testing::ValuesIn(android::hardware::getAllHalInstanceNames(IDumpstateDevice::descriptor)), android::hardware::PrintInstanceNameToString); +// Includes the mode's name as part of the description string. +static inline std::string PrintInstanceNameToStringWithMode( + const testing::TestParamInfo>& info) { + return android::hardware::PrintInstanceNameToString( + testing::TestParamInfo(std::get<0>(info.param), info.index)) + + "_" + toString(std::get<1>(info.param)); +} + +INSTANTIATE_TEST_SUITE_P( + PerInstanceAndMode, DumpstateHidl1_1PerModeTest, + testing::Combine(testing::ValuesIn(android::hardware::getAllHalInstanceNames( + IDumpstateDevice::descriptor)), + testing::ValuesIn(android::hardware::hidl_enum_range())), + PrintInstanceNameToStringWithMode); + } // namespace