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

Test passage in the compatibility paths, add a save by Reach #3076

Merged
merged 11 commits into from
Jul 31, 2021

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Jul 25, 2021

The new save is interesting in the sense of #3072. It is 294 kiB, much smaller than the existing one.

using ::testing::NotNull;
using ::testing::Pair;
using ::testing::internal::CaptureStderr;
Copy link
Member

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.

Copy link
Member Author

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).

Copy link
Member Author

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.

Eq("1970-08-14T08:03:46"_DateTime));
EXPECT_THAT(TTSecond(infnity->psychohistory().back().time),
Eq("1970-08-14T08:47:05"_DateTime));
EXPECT_THAT(infnity->has_flight_plan(), IsTrue());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXPECT_TRUE, or rather ASSERT_TRUE (the next statement would die if this is false.)

Sorry, something went wrong.

GetCapturedStderr(),
AllOf(
HasSubstr(
"pre-Cohen ContinuousTrajectory"), // Regression test for #3039.
Copy link
Member

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.

Sorry, something went wrong.

Copy link
Member Author

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 ».

@pleroy pleroy added the LGTM label Jul 31, 2021
@eggrobin eggrobin merged commit 2594c74 into mockingbirdnest:master Jul 31, 2021
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 this pull request may close these issues.

None yet

2 participants