From c021fad3febf4c6aba4a1257eda4d459e7a2c7b8 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 20 May 2024 04:15:55 -0400 Subject: [PATCH] Add a pigweed-string-format adapter for CHIP_ERROR since that makes unit test debugability a LOT better (#33505) * Define StringBuilderAdapters * Make use of string builder adapters, switch gtest to pw unittest framework * Restyle * Give explicit examples of how string adapters help * Place implementation of ToString in a cpp file * Add missing file --------- Co-authored-by: Andrei Litvin --- src/lib/core/BUILD.gn | 13 +++++ src/lib/core/StringBuilderAdapters.cpp | 31 +++++++++++ src/lib/core/StringBuilderAdapters.h | 53 +++++++++++++++++++ src/setup_payload/tests/BUILD.gn | 1 + .../tests/TestAdditionalDataPayload.cpp | 1 + src/setup_payload/tests/TestManualCode.cpp | 6 +-- src/setup_payload/tests/TestQRCode.cpp | 4 +- src/setup_payload/tests/TestQRCodeTLV.cpp | 4 +- 8 files changed, 108 insertions(+), 5 deletions(-) create mode 100644 src/lib/core/StringBuilderAdapters.cpp create mode 100644 src/lib/core/StringBuilderAdapters.h diff --git a/src/lib/core/BUILD.gn b/src/lib/core/BUILD.gn index 43a6763a709697..ea09ddd01bbaad 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -15,6 +15,7 @@ import("//build_overrides/build.gni") import("//build_overrides/chip.gni") import("//build_overrides/nlio.gni") +import("//build_overrides/pigweed.gni") import("${chip_root}/build/chip/buildconfig_header.gni") import("${chip_root}/build/chip/tests.gni") @@ -106,6 +107,18 @@ source_set("error") { ] } +source_set("string-builder-adapters") { + sources = [ + "StringBuilderAdapters.cpp", + "StringBuilderAdapters.h", + ] + + public_deps = [ + ":error", + "$dir_pw_string", + ] +} + source_set("encoding") { sources = [ "CHIPEncoding.h" ] public_deps = [ "${nlio_root}:nlio" ] diff --git a/src/lib/core/StringBuilderAdapters.cpp b/src/lib/core/StringBuilderAdapters.cpp new file mode 100644 index 00000000000000..d072c1ee905ab7 --- /dev/null +++ b/src/lib/core/StringBuilderAdapters.cpp @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include + +namespace pw { + +template <> +StatusWithSize ToString(const CHIP_ERROR & err, pw::span buffer) +{ + if (CHIP_ERROR::IsSuccess(err)) + { + // source location probably does not matter + return pw::string::Format(buffer, "CHIP_NO_ERROR"); + } + return pw::string::Format(buffer, "CHIP_ERROR:<%" CHIP_ERROR_FORMAT ">", err.Format()); +} + +} // namespace pw diff --git a/src/lib/core/StringBuilderAdapters.h b/src/lib/core/StringBuilderAdapters.h new file mode 100644 index 00000000000000..f173d56b46e6a1 --- /dev/null +++ b/src/lib/core/StringBuilderAdapters.h @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +/// This header includes pigweed stringbuilder adaptations for various chip types. +/// You can see https://pigweed.dev/pw_string/guide.html as a reference. +/// +/// In particular, pigweed code generally looks like: +/// +/// pw::StringBuffer<42> sb; +/// sb << "Here is a value: "; +/// sb << value; +/// +/// Where specific formatters exist for "value". In particular these are used when +/// reporting unit test assertions such as ASSERT_EQ/EXPECT_EQ so if you write code +/// like: +/// +/// ASSERT_EQ(SomeCall(), CHIP_NO_ERROR); +/// +/// On failure without adapters, the objects are reported as "24-byte object at 0x....." +/// which is not as helpful as a full CHIP_ERROR formatted output. +/// +/// Example output WITHOUT the adapters +/// Expected: .... == CHIP_ERROR(0, "src/setup_payload/tests/TestAdditionalDataPayload.cpp", 234) +/// Actual: <24-byte object at 0x7ffe39510b80> == <24-byte object at 0x7ffe39510ba0> +/// +/// Example output WITH the adapters: +/// Expected: .... == CHIP_ERROR(0, "src/setup_payload/tests/TestAdditionalDataPayload.cpp", 234) +/// Actual: CHIP_ERROR: == CHIP_NO_ERROR + +#include + +#include + +namespace pw { + +template <> +StatusWithSize ToString(const CHIP_ERROR & err, pw::span buffer); + +} // namespace pw diff --git a/src/setup_payload/tests/BUILD.gn b/src/setup_payload/tests/BUILD.gn index b08d18a5a4fc15..23022aad8ffafc 100644 --- a/src/setup_payload/tests/BUILD.gn +++ b/src/setup_payload/tests/BUILD.gn @@ -34,6 +34,7 @@ chip_test_suite("tests") { cflags = [ "-Wconversion" ] public_deps = [ + "${chip_root}/src/lib/core:string-builder-adapters", "${chip_root}/src/platform", "${chip_root}/src/setup_payload", ] diff --git a/src/setup_payload/tests/TestAdditionalDataPayload.cpp b/src/setup_payload/tests/TestAdditionalDataPayload.cpp index 36ec0e6a970b90..d59eac34a0b20b 100644 --- a/src/setup_payload/tests/TestAdditionalDataPayload.cpp +++ b/src/setup_payload/tests/TestAdditionalDataPayload.cpp @@ -28,6 +28,7 @@ #include +#include #include #include #include diff --git a/src/setup_payload/tests/TestManualCode.cpp b/src/setup_payload/tests/TestManualCode.cpp index 0b9be9a906011e..abf6a530b51b3e 100644 --- a/src/setup_payload/tests/TestManualCode.cpp +++ b/src/setup_payload/tests/TestManualCode.cpp @@ -22,15 +22,15 @@ * */ -#include +#include #include +#include +#include #include #include #include -#include - #include #include #include diff --git a/src/setup_payload/tests/TestQRCode.cpp b/src/setup_payload/tests/TestQRCode.cpp index 6482b9ad0e7b0a..44c50d9e1e8f5b 100644 --- a/src/setup_payload/tests/TestQRCode.cpp +++ b/src/setup_payload/tests/TestQRCode.cpp @@ -26,7 +26,9 @@ #include -#include +#include + +#include #include using namespace chip; diff --git a/src/setup_payload/tests/TestQRCodeTLV.cpp b/src/setup_payload/tests/TestQRCodeTLV.cpp index cdf6048a49b9a0..7ea2b974070275 100644 --- a/src/setup_payload/tests/TestQRCodeTLV.cpp +++ b/src/setup_payload/tests/TestQRCodeTLV.cpp @@ -16,9 +16,11 @@ */ #include "TestHelpers.h" -#include +#include + #include +#include #include using namespace chip;