Implement Table::ReadRow() #182
Conversation
While in principle these warnings can be ignored, they hide other missing calls that we are interested in.
This test is not executed by the CI builds and it increases the build time. It is convenient for humans, so leave it in the CMakeLists.txt file.
googlebot
added
the
cla: yes
DanielMahu
left a comment
DanielMahu
left a comment
There was a problem hiding this comment.
LGTM with an open question
| #include "bigtable/client/testing/table_test_fixture.h" | ||
|
|
||
| namespace { | ||
| class MockResponseStream : public grpc::ClientReaderInterface< |
There was a problem hiding this comment.
maybe move to table_test_fixture.h, since it's the same with the one in table_readrows_test?
There was a problem hiding this comment.
Good idea. Will refactor before merging.
| EXPECT_CALL(*bigtable_stub_, ReadRowsRaw(_, _)) | ||
| .WillOnce(Invoke([&stream, this](grpc::ClientContext *, | ||
| btproto::ReadRowsRequest const &req) { | ||
| EXPECT_EQ(1, req.rows().row_keys_size()); |
There was a problem hiding this comment.
Nice, I didn't realize the EXPECT_* macros work in lambdas as well. I wrote some silly matchers in my tests.
There was a problem hiding this comment.
EXPECT_EQ() works fine inside lambdas. ASSERT_EQ() does not always work because it expands to if (blah) { return; }, and the lambda may return something other than void. googletest should use exceptions 😁
| TEST_F(RowReaderTest, EmptyReaderHasNoRows) { | ||
| EXPECT_CALL(*bigtable_stub_, ReadRowsRaw(_, _)).WillOnce(Return(stream_)); | ||
| EXPECT_CALL(*stream_, Read(_)).WillOnce(Return(false)); | ||
| EXPECT_CALL(*stream_, Finish()).WillOnce(Return(grpc::Status::OK)); |
There was a problem hiding this comment.
What happens (in general) if client code does not call Finish? Couldn't find an answer to this in the gRPC documentation:
https://grpc.io/grpc/cpp/classgrpc_1_1internal_1_1_client_streaming_interface.html
but if the answer is: a call is left dangling, then I should take care of this in the RowReader
There was a problem hiding this comment.
The gRPC documentation is awful about these things. I would expect that not calling Finish() leaks whatever resources are needed to return the result of Finish() when it is eventually called, right?
On a separate (but related note) I have observed assertion errors for any streaming gRPC that does not follow exactly the right sequence of calls:
GoogleCloudPlatform/cpp-samples#16
In short, I think if we step outside the exact sequence of calls that gRPC expects we are wandering into undefined behavior territory.
Mainly refactor MockResponseStream to bigtable/client/testing.
3 participants
This fixes #29.