Skip to content

fix: Fix for flaky test_read_timestamp_client_side_autocommit test #1071

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.

Sign up for GitHub

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

Merged
ankiaga merged 4 commits into from
Jan 10, 2024
Merged

fix: Fix for flaky test_read_timestamp_client_side_autocommit test #1071

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a49609f
fix: Fix for flaky test_read_timestamp_client_side_autocommit test
ankiaga Jan 10, 2024
3b99ceb
Merge branch 'main' into fix
harshachinta Jan 10, 2024
24fd4e3
Adding a row between 2 transactions so that read timestamp are differ…
ankiaga Jan 10, 2024
bb23c13
Merge branch 'main' into fix
ankiaga Jan 10, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/system/test_dbapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

@harshachinta harshachinta Jan 10, 2024

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
Copy link
Contributor Author

@ankiaga ankiaga Jan 10, 2024

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
Copy link
Contributor

@olavloite olavloite Jan 10, 2024

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.)
self._cursor.execute("SHOW VARIABLE READ_TIMESTAMP")
read_timestamp_query_result_2 = self._cursor.fetchall()
Expand Down