Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add safe proto diff helper #433

Closed
eric846 opened this issue Aug 3, 2020 · 8 comments · Fixed by #890
Closed

Add safe proto diff helper #433

eric846 opened this issue Aug 3, 2020 · 8 comments · Fixed by #890
Labels

Comments

@eric846
Copy link
Contributor

eric846 commented Aug 3, 2020

Need to do this separately after test/adaptive_load/utility.h goes in.

The helper should do:
EXPECT_EQ(message->DebugString(), expected_config.DebugString());
EXPECT_TRUE(Envoy::MessageUtil()(*message, expected_config));

oschaaf:
I learned that with proto3 default values may not get serialised. I'm not 100% sure that equivalence here guarantees actual equivalence with respect to set/not-set/default-value. Therefore, to make sure no edge cases slip past, maybe it makes sense to also double down with MessageUtil()(message1, message1). This won't produce a helpful test message, so I'd put it after comparing the debug strings (which should produce a nice diff), maybe in a helper method.

@eric846 eric846 closed this as completed Aug 3, 2020
@eric846
Copy link
Contributor Author

eric846 commented Aug 3, 2020

Actually this isn't specific to adaptive_load, could go in an existing file.

@eric846
Copy link
Contributor Author

eric846 commented Aug 3, 2020

Actually I don't see an existing file to put it in, will handle this after all these PRs are done.

@eric846
Copy link
Contributor Author

eric846 commented Aug 20, 2020

This may be clearer than the construct with MessageUtil():

  EXPECT_TRUE(::Envoy::Protobuf::util::MessageDifferencer::Equivalent(*empty_config, expected_config));

@oschaaf
Copy link
Member

oschaaf commented Aug 20, 2020

Oh that's much nicer indeed

@oschaaf
Copy link
Member

oschaaf commented Oct 1, 2020

In the context of this issue, also making a note of a helpful review comment by @mum4k [1].
TIL there's ::testing::EqualsProto
Sounds like we may want to use that as a replacement where applicable?

[1] #522 (comment)

@oschaaf oschaaf added tech-debt good first issue Good for newcomers labels Oct 1, 2020
@mum4k
Copy link
Collaborator

mum4k commented Oct 1, 2020

Interestingly enough, testing::EqualsProto looks to be internal only:

google/googletest#1761

While it is in the public documentation of gtest and gmock, it isn't actually present in the code. So back to plan B.

@mum4k
Copy link
Collaborator

mum4k commented Aug 18, 2021

Fyi, we now have a proto matcher:

// Compares two proto messages for equality.
// Prints diff on failures.
//
// Example use:
// proto2::Message actual_proto;
// proto2::Message expected_proto;
//
// EXPECT_THAT(actual_proto, EqualsProto(expected_proto));
MATCHER_P(EqualsProto, expected_proto, "is equal to the expected_proto") {

@eric846
Copy link
Contributor Author

eric846 commented Aug 18, 2021 via email

mum4k pushed a commit that referenced this issue Aug 4, 2022
In some environments, whitespace differences cause these tests to fail.
String matching is less stable than the proto matchers that we now have.

Works on #433 

Signed-off-by: Nathan Perry <[email protected]>
@mum4k mum4k closed this as completed in #890 Aug 5, 2022
mum4k pushed a commit that referenced this issue Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants