ReadRows integration test #403
Conversation
googlebot
added
the
cla: yes
|
During testing I found a couple of corner cases with the empty This one failed against the emulator, but succeeded in production, when added to TableReadRowsAllRows: auto read4 =
table->ReadRows(bigtable::RowSet(), bigtable::Filter::PassAllFilter());
CheckEqualUnordered(created, MoveCellsFromReader(read4));it's testing that the default This failed in both, in TableReadRowsNoRows: ASSERT_TRUE(bigtable::RowSet(bigtable::RowRange::Empty()).IsEmpty());
auto read3 = table->ReadRows(bigtable::RowSet(bigtable::RowRange::Empty()),
bigtable::Filter::PassAllFilter());
CheckEqualUnordered(expected, MoveCellsFromReader(read3));and was broken because we'd expect that reading an empty |
|
Thanks for the tests! For For |
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
Please consider adding the tests for the corner cases you described, and using std::move(). Otherwise looks pretty good.
| bigtable::RowReader& reader) { | ||
| std::vector<bigtable::Cell> result; | ||
| for (auto const& row : reader) { | ||
| std::copy(row.cells().begin(), row.cells().end(), |
There was a problem hiding this comment.
Consider using std::move() (the function in <algorithm> not the function in <utility>)
There was a problem hiding this comment.
And if you think it is not a good idea or not possible tell me here!
|
I tried replacing the RowRange::Empty() implementation to return ("", "\0") and the integration test passed, but other tests failed because we have some bugs in the RowRange class. I filed #404, and I could take care of it next week. |
|
@DanielMahu any news? |
| table.ReadRows(bigtable::RowSet(bigtable::RowRange::InfiniteRange()), | ||
| bigtable::Filter::PassAllFilter()); | ||
| #if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS | ||
| EXPECT_THROW(read1.begin(), std::runtime_error); |
There was a problem hiding this comment.
I think I know why this fails with no exceptions enabled. Calling read1.begin() is an error, but we no longer crash the application when the iterator first fails. The application needs to call read1.Finish() to get the error status, and only if they fail to retrieve the status does the code crash (to avoid silent errors).
There was a problem hiding this comment.
Thanks for pointing that out. But I actually got confused by the line here https://github.com/GoogleCloudPlatform/google-cloud-cpp/blob/dc5c782d69c8f455f075ba602aa90183fbb427c5/bigtable/client/table.cc#L74
that always sets raise_on_error to true... and I still am, it seems to me it's still going down that code path in the noex:: namespace?
I reran the test, with
EXPECT_EQ(read1.begin(), read1.end());and got this:
Aborting because exceptions are disabled: Unretriable error: table "projects/emulated-1524060748/instances/data-test/tables/table-read-rows-wrong-table" not found
so the program does die.
Is the no-exception code path ready? otherwise we could submit without this test and fix it separately together with that.
There was a problem hiding this comment.
I've changed noex::Table::ReadRows to use raise_on_error=false, I think that should do the trick. PTAL I was obviously wrong, sadly the unit tests seem to pass on my Mac.
3 participants
This fixes #77.
I tried to cover the basic cases in a style similar to the other tests