-
Notifications
You must be signed in to change notification settings - Fork 92
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
fix: Fix for flaky test_read_timestamp_client_side_autocommit test #1071
Conversation
@@ -420,6 +420,7 @@ def test_read_timestamp_client_side_autocommit(self): | |||
assert self._cursor.description[0].name == "SHOW_READ_TIMESTAMP" | |||
assert isinstance(read_timestamp_query_result_1[0][0], DatetimeWithNanoseconds) | |||
|
|||
time.sleep(0.25) | |||
self._cursor.execute("SELECT * FROM contacts") |
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.
Do you feel we need to test this command twice? This way we can avoid sleep, just thoughts.
Java does it only once and validates the type of result. https://github.com/googleapis/java-spanner/blob/58f94b200276d879f83e4432716b49baf3206226/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java#L1003
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 I understand your comment clearly. We are executing command twice because we want to confirm that the state is updated in the cursor class and that read timestamps are different.
Also removed the sleep command and inserting a row after discussion with Knut
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.
nit: it should not be necessary to set the connection in read-only mode for this as long as we are in auto-commit mode. (Having it in read-only mode also does not hurt, so feel free to leave as-is, as long as we are confident that it would work with self._conn.read_only=False` as well.)
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, but wondering why do we need to execute the command twice and verify the read_timestamp of both.
…ent for the 2 transactions
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
@@ -420,6 +420,7 @@ def test_read_timestamp_client_side_autocommit(self): | |||
assert self._cursor.description[0].name == "SHOW_READ_TIMESTAMP" | |||
assert isinstance(read_timestamp_query_result_1[0][0], DatetimeWithNanoseconds) | |||
|
|||
time.sleep(0.25) | |||
self._cursor.execute("SELECT * FROM contacts") |
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.
nit: it should not be necessary to set the connection in read-only mode for this as long as we are in auto-commit mode. (Having it in read-only mode also does not hurt, so feel free to leave as-is, as long as we are confident that it would work with self._conn.read_only=False` as well.)
No description provided.