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

feat(tests): [Bigtable] add conformance tests and proxy #7959

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Dec 20, 2024

b/372508972

Fixed Tests

These are tests which have been fixed by small changes to the library

  • TestMutateRows_NoRetry_NonTransientErrors - Fixed by adding "index" to$rowMutationsFailedResponse. See this line. This should not be a breaking change, but it is a change, so it should be confirmed.
  • *_Generic_CloseClient - these are fixed by ensuring the appProfileId is propagated correctly to the requests. This functionality worked in V1 but was broken in the V2 release, and is now fixed (see this line for an example)

Failing Tests

  • TestReadRows_NoRetry_OutOfOrderError and TestReadRows_NoRetry_OutOfOrderError_Reverse - no error is thrown (an error should be thrown which contains "increasing" and "decreasing" in the error message, respectively). This seems to be an issue which requires fixing.

  • TestReadRows_Retry_PausedScan, TestReadRows_Retry_StreamReset, and TestReadRows_Retry_LastScannedRow - I believe what's happening here is there is an error because the retry request doesn't contain "RowRanges" as expected. This may also be an issue which requires fixing.

  • TestReadRows_Generic_DeadlineExceeded and TestReadRow_Generic_DeadlineExceeded - failing because the deadline exceeded error is being suppressed by the retries. This seems to be WAI.

  • TestMutateRows_Generic_DeadlineExceeded - Failing because the Deadline Exceeded exception is suppressed and added to the BigtableDataOperationException. This seems to be WAI.

@bshaffer bshaffer added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 20, 2024
@bshaffer bshaffer marked this pull request as ready for review January 16, 2025 17:33
@bshaffer bshaffer requested review from a team as code owners January 16, 2025 17:33
@bshaffer bshaffer removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 16, 2025
Copy link

@mutianf mutianf left a comment

Choose a reason for hiding this comment

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

LGTM but can we remove a few tests from known failures? Thank you!

_Retry_WithRetryInfo
TestMutateRows_Generic_DeadlineExceeded
TestReadRow_Generic_DeadlineExceeded
TestReadRows_NoRetry_OutOfOrderError
Copy link

Choose a reason for hiding this comment

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

Can we remove TestReadRows_NoRetry_OutOfOrderError, TestReadRows_Retry_StreamReset, TestReadRows_Retry_PausedScan and TestReadRows_Retry_LastScannedRow from this list? So we don't forget about them :) We don't have to mark conformance test passing as a required step for presubmit for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants