-
Notifications
You must be signed in to change notification settings - Fork 69
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
Test passage in the compatibility paths, add a save by Reach #3076
Conversation
using ::testing::NotNull; | ||
using ::testing::Pair; | ||
using ::testing::internal::CaptureStderr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not access the internals, please. It's going to break whenever Google feel like changing their implementation.
The same effect could be achieved in a portable manner with EXPECT_EXIT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, because then it needs to be a death test and this makes it tricky (and unreadable) to have more complicated tests like the Reach one.
The proper way to do this would be to take advantage of the fact that we have decided to diverge our glog and have it be instrumentable, instead of reaching all the way to standard error (either directly or even more circuitously via EXPECT_EXIT).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A LogSink
does the job.
#include "base/file.hpp" | ||
#include "base/not_null.hpp" | ||
#include "base/pull_serializer.hpp" | ||
#include "base/push_deserializer.hpp" | ||
#include "glog/logging.h" | ||
#include "gmock/gmock.h" | ||
#include "google/protobuf/io/coded_stream.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
GetCapturedStderr(), | ||
AllOf( | ||
HasSubstr( | ||
"pre-Cohen ContinuousTrajectory"), // Regression test for #3039. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not convinced that the comments here add value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least this one seems useful, since this this is what #3039 was about, and it’s not particularly obvious from the phrase « pre-Cohen ContinuousTrajectory ».
The new save is interesting in the sense of #3072. It is 294 kiB, much smaller than the existing one.