From 85a126ec5d76457381333dd5abdde36da0f06c1c Mon Sep 17 00:00:00 2001 From: Alessio Bazzica Date: Thu, 11 Aug 2022 15:48:54 +0200 Subject: [PATCH] `ApmTest.VerifyDebugDump(Int|Float)` more insightful with failures Instead of showing individual byte differences, this CL detects differences in the expected and actual byte streams of the evaluated AEC dump and, if detected, parses the `audioproc::Event` proto lite messages and calls EXPECT_EQ() for a subset of individual (sub-)fields. Note that messages are parsed only if the byte streams of each message pair do not match, so with no failures the test runs at no extra cost. Plus, the the added funcionality can only be enabled for local debugging by flipping the `kDumpWhenExpectMessageEqFails` flag - a code change cannot land if the flag is set to true. Note that `MessageDifferencer` (see [1]) could not be used because it is not implemented for `MessageLite` protos. [1] https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.util.message_differencer Bug: b/241923537 Change-Id: I8e0eda3b1ecfe06945b6dad5ee8871f8200d76d7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/270922 Reviewed-by: Jesus de Vicente Pena Commit-Queue: Alessio Bazzica Cr-Commit-Position: refs/heads/main@{#37765} --- .../audio_processing_unittest.cc | 76 ++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-) diff --git a/modules/audio_processing/audio_processing_unittest.cc b/modules/audio_processing/audio_processing_unittest.cc index 32cf9a866c..5c934338ff 100644 --- a/modules/audio_processing/audio_processing_unittest.cc +++ b/modules/audio_processing/audio_processing_unittest.cc @@ -50,6 +50,7 @@ #include "test/testsupport/file_utils.h" RTC_PUSH_IGNORING_WUNDEF() +#include "modules/audio_processing/debug.pb.h" #ifdef WEBRTC_ANDROID_PLATFORM_BUILD #include "external/webrtc/webrtc/modules/audio_processing/test/unittest.pb.h" #else @@ -335,6 +336,72 @@ std::string GetReferenceFilename() { #endif } +// Flag that can temporarily be enabled for local debugging to inspect +// `ApmTest.VerifyDebugDump(Int|Float)` failures. Do not upload code changes +// with this flag set to true. +constexpr bool kDumpWhenExpectMessageEqFails = false; + +// Checks the debug constants values used in this file so that no code change is +// submitted with values temporarily used for local debugging. +TEST(ApmUnitTests, CheckDebugConstants) { + ASSERT_FALSE(kDumpWhenExpectMessageEqFails); +} + +// Expects the equality of `actual` and `expected` by inspecting a hard-coded +// subset of `audioproc::Stream` fields. +void ExpectStreamFieldsEq(const audioproc::Stream& actual, + const audioproc::Stream& expected) { + EXPECT_EQ(actual.input_data(), expected.input_data()); + EXPECT_EQ(actual.output_data(), expected.output_data()); + EXPECT_EQ(actual.delay(), expected.delay()); + EXPECT_EQ(actual.drift(), expected.drift()); + EXPECT_EQ(actual.level(), expected.level()); + EXPECT_EQ(actual.keypress(), expected.keypress()); +} + +// Expects the equality of `actual` and `expected` by inspecting a hard-coded +// subset of `audioproc::Event` fields. +void ExpectEventFieldsEq(const audioproc::Event& actual, + const audioproc::Event& expected) { + EXPECT_EQ(actual.type(), expected.type()); + if (actual.type() != expected.type()) { + return; + } + switch (actual.type()) { + case audioproc::Event::STREAM: + ExpectStreamFieldsEq(actual.stream(), expected.stream()); + break; + default: + // Not implemented. + break; + } +} + +// Returns true if the `actual` and `expected` byte streams share the same size +// and contain the same data. If they differ and `kDumpWhenExpectMessageEqFails` +// is true, checks the equality of a subset of `audioproc::Event` (nested) +// fields. +bool ExpectMessageEq(rtc::ArrayView actual, + rtc::ArrayView expected) { + EXPECT_EQ(actual.size(), expected.size()); + if (actual.size() != expected.size()) { + return false; + } + if (memcmp(actual.data(), expected.data(), actual.size()) == 0) { + // Same message. No need to parse. + return true; + } + if (kDumpWhenExpectMessageEqFails) { + // Parse differing messages and expect equality to produce detailed error + // messages. + audioproc::Event event_actual, event_expected; + RTC_DCHECK(event_actual.ParseFromArray(actual.data(), actual.size())); + RTC_DCHECK(event_expected.ParseFromArray(expected.data(), expected.size())); + ExpectEventFieldsEq(event_actual, event_expected); + } + return false; +} + class ApmTest : public ::testing::Test { protected: ApmTest(); @@ -1525,8 +1592,13 @@ void ApmTest::VerifyDebugDumpTest(Format format) { bytes_read_limited += limited_size; EXPECT_EQ(ref_size, out_size); EXPECT_GE(ref_size, limited_size); - EXPECT_EQ(0, memcmp(ref_bytes.get(), out_bytes.get(), ref_size)); - EXPECT_EQ(0, memcmp(ref_bytes.get(), limited_bytes.get(), limited_size)); + EXPECT_TRUE(ExpectMessageEq(/*actual=*/{out_bytes.get(), out_size}, + /*expected=*/{ref_bytes.get(), ref_size})); + if (limited_size > 0) { + EXPECT_TRUE( + ExpectMessageEq(/*actual=*/{limited_bytes.get(), limited_size}, + /*expected=*/{ref_bytes.get(), ref_size})); + } ref_size = ReadMessageBytesFromFile(ref_file, &ref_bytes); out_size = ReadMessageBytesFromFile(out_file, &out_bytes); limited_size = ReadMessageBytesFromFile(limited_file, &limited_bytes);