fix: update conformance test to determine if request is full table scan#211
fix: update conformance test to determine if request is full table scan#211mutianf merged 3 commits intogoogleapis:mainfrom
Conversation
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
tests/readrows_test.go
Outdated
| t.Errorf("diff found (-want +got):\n%s", diff) | ||
| origRows := origReq.req.GetRows() | ||
| // Check if rows or row ranges are present in requests | ||
| if origRows == nil || origRows.GetRowRanges() == nil || len(origRows.GetRowRanges()) == 1 { |
There was a problem hiding this comment.
I think we should verify that getRowRanges is an empty range
There was a problem hiding this comment.
Can you clarify what you mean by that with an example?
There was a problem hiding this comment.
if we're checking len(originRows.GetRowRanges) = 1, does that mean [{keyA, keyB}] will also be valid? But we want to verify it's [{}].
There was a problem hiding this comment.
Added extra check.
tests/readrows_test.go
Outdated
| if diff := cmp.Diff(clientReq, origReq.req, protocmp.Transform(), protocmp.IgnoreEmptyMessages()); diff != "" { | ||
| t.Errorf("diff found (-want +got):\n%s", diff) | ||
| origRows := origReq.req.GetRows() | ||
| // Check if rows or row ranges are present in requests |
There was a problem hiding this comment.
Also can we update the comment to something like: "This check is a workaround for nodejs client. In nodejs, we add an empty row range to a full table scan request to simplify the resumption logic."
tests/readrows_test.go
Outdated
| if origRows == nil || origRows.GetRowRanges() == nil || len(origRows.GetRowRanges()) == 1 { | ||
| // Check if rows or row ranges are present in requests. This is a workaround for the NodeJS client. | ||
| // In Node we add an empty row range to a full table scan request to simplify the resumption logic. | ||
| if origRows == nil || origRows.GetRowRanges() == nil || (len(origRows.GetRowRanges()) == 1 && len(origRows.GetRowKeys()) == 0){ |
There was a problem hiding this comment.
Let's split this up:
if origRows == nil || origRows.GetRowRanges() == nil {
// skip
} else if len(origRows.getRowRanges()) == 1 && origRows.getRowRanges().getStartKey() == nil && origRows.getRowRanges().getEndKey() == nil {
// skip
} else {
t.Errorf("diff found... ")
}
The NodeJS CBT client fails the
TestReadRows_Retry_PausedScantest with the following error message:This is due to the fact that if the original request is a full table scan, the client request will look different on a retry (the
rowsmight not exist or if it does therowswill be an object with an empty array because there are norow_keys). This was causing a false diff failure in the conformance test so we're updating this conformance test to handle this edge case.More info at: b/380268261