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

Make timestamp overrides optional in tests and add faketime test #1953

Merged
merged 10 commits into from
Apr 19, 2024

Conversation

kounelisagis
Copy link
Member

@kounelisagis kounelisagis commented Apr 16, 2024

Currently in TileDB-Py we have a number of tests which override the timestamp for writes in order to avoid test failures when writes happen within the same millisecond, e.g.:

arr.set_open_timestamp_start(1)

with tiledb.SparseArray(uri, mode="w", timestamp=timestamp) as T:

In libtiledb 2.22.0 a sub-millisecond temporal disambiguation of random labels from a single-process is implemented (serialized ordering for writes).

To test this from Python, tests that use timestamp ordering overrides are modified. They now run both with and without overriding and they should succeed in both cases. This solution is temporary; in the future, code that uses ordering override will be removed, ensuring a robust solution.

Additionally, to directly check that the new feature is working as expected, libfaketime is utilized. More specifically the faketime wrapper comes in handy in our case to decrease the complexity and have a unified solution for both Linux and macOS.
By using faketime and setting the time multiplier to zero (faketime -f '+x0' python time_test.py), we are able to "freeze" time for the whole duration of the test which runs in a subprocess.
Installation of libfaketime means a new CI step. A new test file, test_timestamp_overrides is added to run write tests under faketime and check that the expected outcome holds.

Checking if libfaketime exists, means a system call to skip test.

Copy link

@bekadavis9 bekadavis9 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

A few suggested changes, but overall great work on the test_timestamp_overrides. When I apply the changes here, copy just that file out

  • run w/ pytest under tiledb-py<0.28, it fails (as expected)
  • run with tiledb-py==0.28, it succeeds. Excellent!

tiledb/tests/test_timestamp_overrides.py Outdated Show resolved Hide resolved
tiledb/tests/test_timestamp_overrides.py Outdated Show resolved Hide resolved
tiledb/tests/test_timestamp_overrides.py Outdated Show resolved Hide resolved
@kounelisagis kounelisagis merged commit 8a98a42 into dev Apr 19, 2024
28 checks passed
@kounelisagis kounelisagis deleted the agis/sc-42967/make-timestamp-overrides-optional branch April 19, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants