Skip to content

Commit 68b196b

Browse files
authored
Remove use of protobuf debug APIs in test assertions to fix breaking change (#2287)
Summary: Remove use of protobuf debug APIs in test assertions to fix breaking change Protobuf v30 and later intentionally malform the `DebugString` string output to prevent it from being parsed as a protobuf message ([announcement details](https://protobuf.dev/news/2024-12-04/)). This breaks our protobuf test assertions and is something we need to fix ahead of migrating to bazel 7. Relevant Issues: #2282 Type of change: /kind cleanup Test Plan: Build should pass Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
1 parent 770c509 commit 68b196b

File tree

6 files changed

+33
-13
lines changed

6 files changed

+33
-13
lines changed

src/carnot/planner/compiler_error_context/compiler_error_context_test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ TEST(CompilerErrorContextStatus, Default) {
5151
ASSERT_TRUE(status_pb.context().Is<compilerpb::CompilerErrorGroup>());
5252

5353
status_pb.context().UnpackTo(&errorgroup_out);
54-
EXPECT_EQ(errorgroup_in.DebugString(), errorgroup_out.DebugString());
54+
EXPECT_TRUE(google::protobuf::util::MessageDifferencer::Equals(errorgroup_in, errorgroup_out));
5555
for (int64_t i = 0; i < errorgroup_in.errors_size(); i++) {
5656
auto error_parent_out = errorgroup_in.errors(i);
5757
auto error_out = error_parent_out.line_col_error();

src/carnot/planner/distributed/distributed_plan/distributed_plan_test.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#include <gmock/gmock.h>
2020
#include <google/protobuf/text_format.h>
21+
#include <google/protobuf/util/message_differencer.h>
2122
#include <gtest/gtest.h>
2223

2324
#include <unordered_map>
@@ -208,7 +209,8 @@ TEST_F(DistributedPlanTest, construction_test) {
208209

209210
for (const auto& [carnot_id, carnot_info] : carnot_id_to_carnot_info_map) {
210211
CarnotInstance* carnot_instance = physical_plan->Get(carnot_id);
211-
EXPECT_THAT(carnot_instance->carnot_info(), EqualsProto(carnot_info.DebugString()));
212+
EXPECT_TRUE(google::protobuf::util::MessageDifferencer::Equals(carnot_instance->carnot_info(),
213+
carnot_info));
212214
EXPECT_THAT(carnot_instance->QueryBrokerAddress(), carnot_info.query_broker_address());
213215
auto new_graph = std::make_shared<IR>();
214216
SwapGraphBeingBuilt(new_graph);

src/carnot/planner/docs/doc_extractor_main.cc

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,14 @@
1616
* SPDX-License-Identifier: Apache-2.0
1717
*/
1818

19+
#include <fstream>
20+
#include <string>
21+
22+
#include <google/protobuf/text_format.h>
23+
1924
#include "src/carnot/planner/docs/doc_extractor.h"
2025
#include "src/carnot/udf_exporter/udf_exporter.h"
2126

22-
#include <fstream>
23-
2427
namespace px {
2528
namespace carnot {
2629
namespace planner {
@@ -79,8 +82,11 @@ int main(int argc, char** argv) {
7982
}
8083
auto docs = docs_or_s.ConsumeValueOrDie();
8184

85+
std::string text_output;
86+
google::protobuf::TextFormat::PrintToString(docs, &text_output);
87+
8288
std::ofstream output_file;
8389
output_file.open(FLAGS_output_file);
84-
output_file << docs.DebugString();
90+
output_file << text_output;
8591
output_file.close();
8692
}

src/common/base/status_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ TEST(Status, context_copy_tests) {
111111
Status s2 = s1;
112112
EXPECT_TRUE(s2.has_context());
113113
EXPECT_EQ(s1, s2);
114-
EXPECT_EQ(s1.context()->DebugString(), s2.context()->DebugString());
114+
EXPECT_TRUE(google::protobuf::util::MessageDifferencer::Equals(*s1.context(), *s2.context()));
115115
}
116116

117117
TEST(Status, context_vs_no_context_status) {
@@ -120,7 +120,7 @@ TEST(Status, context_vs_no_context_status) {
120120
EXPECT_NE(s1, s2);
121121
EXPECT_FALSE(s2.has_context());
122122
EXPECT_EQ(s2.context(), nullptr);
123-
EXPECT_NE(s1.ToProto().DebugString(), s2.ToProto().DebugString());
123+
EXPECT_FALSE(google::protobuf::util::MessageDifferencer::Equals(s1.ToProto(), s2.ToProto()));
124124
}
125125

126126
TEST(StatusAdapter, proto_with_context_test) {

src/common/testing/protobuf.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include <optional>
2828
#include <ostream>
2929
#include <string>
30+
#include <type_traits>
3031
#include <utility>
3132

3233
#include "src/common/base/logging.h"
@@ -109,6 +110,17 @@ inline ::testing::PolymorphicMatcher<EqualsProtoMatcher> EqualsProto(std::string
109110
return ::testing::MakePolymorphicMatcher(EqualsProtoMatcher(std::move(text_pb)));
110111
}
111112

113+
// Overload that takes a protobuf message directly.
114+
// This avoids the need to call DebugString() which in newer protobuf versions
115+
// adds a "goo.gle/debugonly" prefix that breaks parsing.
116+
template <typename ProtoType,
117+
typename = std::enable_if_t<std::is_base_of_v<google::protobuf::Message, ProtoType>>>
118+
inline ::testing::PolymorphicMatcher<EqualsProtoMatcher> EqualsProto(const ProtoType& proto) {
119+
std::string text_pb;
120+
google::protobuf::TextFormat::PrintToString(proto, &text_pb);
121+
return ::testing::MakePolymorphicMatcher(EqualsProtoMatcher(std::move(text_pb)));
122+
}
123+
112124
inline ::testing::PolymorphicMatcher<PartiallyEqualsProtoMatcher> Partially(
113125
const ::testing::PolymorphicMatcher<EqualsProtoMatcher>& matcher) {
114126
return ::testing::MakePolymorphicMatcher(

src/vizier/services/agent/pem/tracepoint_manager_test.cc

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,8 @@ TEST_F(TracepointManagerTest, CreateTracepoint) {
115115
tracepoint->set_name("test_tracepoint");
116116

117117
EXPECT_CALL(stirling_,
118-
RegisterTracepoint(tracepoint_id, ::testing::Pointee(testing::proto::EqualsProto(
119-
tracepoint->DebugString()))));
118+
RegisterTracepoint(tracepoint_id,
119+
::testing::Pointee(testing::proto::EqualsProto(*tracepoint))));
120120
EXPECT_OK(tracepoint_manager_->HandleMessage(std::move(msg)));
121121

122122
EXPECT_CALL(stirling_, GetTracepointInfo(tracepoint_id))
@@ -152,8 +152,8 @@ TEST_F(TracepointManagerTest, CreateTracepointFailed) {
152152
tracepoint->set_name("test_tracepoint");
153153

154154
EXPECT_CALL(stirling_,
155-
RegisterTracepoint(tracepoint_id, ::testing::Pointee(testing::proto::EqualsProto(
156-
tracepoint->DebugString()))));
155+
RegisterTracepoint(tracepoint_id,
156+
::testing::Pointee(testing::proto::EqualsProto(*tracepoint))));
157157
EXPECT_OK(tracepoint_manager_->HandleMessage(std::move(msg)));
158158

159159
EXPECT_CALL(stirling_, GetTracepointInfo(tracepoint_id))
@@ -185,8 +185,8 @@ TEST_F(TracepointManagerTest, CreateTracepointPreconditionFailed) {
185185
tracepoint->set_name("test_tracepoint");
186186

187187
EXPECT_CALL(stirling_,
188-
RegisterTracepoint(tracepoint_id, ::testing::Pointee(testing::proto::EqualsProto(
189-
tracepoint->DebugString()))));
188+
RegisterTracepoint(tracepoint_id,
189+
::testing::Pointee(testing::proto::EqualsProto(*tracepoint))));
190190
EXPECT_OK(tracepoint_manager_->HandleMessage(std::move(msg)));
191191

192192
EXPECT_CALL(stirling_, GetTracepointInfo(tracepoint_id))

0 commit comments

Comments
 (0)