-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Random test migrations to Protobuf #8072
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
Random test migrations to Protobuf #8072
Conversation
Generated by 🚫 Danger |
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.
lgtm
Status resulting_status; | ||
datastore->LookupDocuments( | ||
{}, [&](const StatusOr<std::vector<MaybeDocument>>& maybe_documents) { | ||
{}, [&](const StatusOr<std::vector<Document>>& maybe_documents) { |
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.
s/maybe_documents/documents
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.
Fixed
Status resulting_status; | ||
datastore->LookupDocuments( | ||
{}, [&](const StatusOr<std::vector<MaybeDocument>>& maybe_documents) { | ||
{}, [&](const StatusOr<std::vector<Document>>& maybe_documents) { |
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.
same here and below
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.
Fixed
expected_data = | ||
expected_data.Set(Field("foo.bar"), FieldValue::FromServerTimestamp(now)); | ||
expected_data.Set(Field("foo.bar"), | ||
EncodeServerTimestamp(now, absl::nullopt)); |
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 sure if you have another PR to fix this, but now
has type Timestamp
, but EncodeServerTimestamp
requires google_protobuf_Timestamp
.
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.
Brings tests in line with Android.