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

Add faketime tests #4883

Merged
merged 16 commits into from
May 1, 2024
Merged

Add faketime tests #4883

merged 16 commits into from
May 1, 2024

Conversation

dudoslav
Copy link
Contributor

Rebased onto main branch from: #4843


This PR introduces timing tests that run with mocked time. During the test all calls to acquire timestamp, either by using posix time() or modern C++ std::chrono::clock::now() are mocked and should return the same timestamp. This way we should be able to test sub-millisecond writes/reads.

This PR adds:

  1. libfaketime port
  2. timing tests target
  3. modified timing tests command with proper environment variables

These tests are supposed to fail on TileDB version 2.20.0:

https://github.com/dudoslav/TileDB/actions/runs/8535232007/job/23381209269#step:14:97


TYPE: NO_HISTORY
DESC: Add faketime tests

@dudoslav dudoslav self-assigned this Apr 17, 2024
@dudoslav dudoslav mentioned this pull request Apr 17, 2024
Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Instead of adding another test executable, how about you redefine the tiledb_timing_unit test to run tiledb_unit while LD_PRELOADing libfaketime?

@bekadavis9 bekadavis9 self-requested a review April 17, 2024 13:07
@ihnorton
Copy link
Member

Instead of adding another test executable, how about you redefine the tiledb_timing_unit test to run tiledb_unit while LD_PRELOADing libfaketime?

@teo-tsirpanis do you mean through cmake variables, or as a subprocess through system()? (the former seems kind of equivalent to what's done here - libfaketime linkage handles it IIUC)

One reason to do it this way is so that we can add a specific test to ensure that faketime is doing what we want (like in the corresponding tiledb-py test)

@@ -362,6 +362,45 @@ if (${CMAKE_SYSTEM_NAME} MATCHES "Linux" AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU
)
endif()

# Only produce timing tests for UNIX based systems (faketime constraint)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a note about how this test works (we link libfaketime, and set the environment override at the bottom so that it applies).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to LD_PRELOAD

@teo-tsirpanis
Copy link
Member

@teo-tsirpanis do you mean through cmake variables, or as a subprocess through system()? (the former seems kind of equivalent to what's done here - libfaketime linkage handles it IIUC)

@ihnorton I mean the former. The difference of my proposal from the current arrangement is that we will build only one tiledb_unit executable.

WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
)

set_property(TEST tiledb_timing_unit PROPERTY ENVIRONMENT_MODIFICATION "FAKETIME=set:2020-12-24 20:30:00;LD_PRELOAD=set:${CMAKE_BINARY_DIR}/vcpkg_installed/x64-linux/lib/liblibfaketime.so")
Copy link
Member

Choose a reason for hiding this comment

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

You should call find_library to get the path. And instead of if not on Windows, you should write if the library was not found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 42 to 59
install(EXPORT ${PROJECT_NAME}
FILE unofficial-${PROJECT_NAME}-targets.cmake
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/unofficial-${PROJECT_NAME}
NAMESPACE unofficial::libfaketime::
)

include(GNUInstallDirs)
include(CMakePackageConfigHelpers)

configure_package_config_file(
unofficial-${PROJECT_NAME}-config.cmake.in
unofficial-${PROJECT_NAME}-config.cmake
INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/unofficial-${PROJECT_NAME})

write_basic_package_version_file(
unofficial-${PROJECT_NAME}-config-version.cmake
VERSION ${CMAKE_PROJECT_VERSION}
COMPATIBILITY SameMajorVersion)
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the exported targets anymore. And they are not necessary for the port on second thought since libfaketime does not have dependencies on its own.

ports/libfaketime/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -0,0 +1,38 @@
cmake_minimum_required(VERSION 3.12)

project(faketime VERSION 0.9.10)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
project(faketime VERSION 0.9.10)
project(faketime)

The version is not needed here. You don't have to change this now (to avoid restarting CI), just keep it in mind when upstreaming the port.

@dudoslav
Copy link
Contributor Author

Tested locally and it fails on 2.20.0 so it should be ready to merge

@ihnorton ihnorton merged commit 04f1d83 into dev May 1, 2024
60 checks passed
@ihnorton ihnorton deleted the db/sc-43955/faketime_fix branch May 1, 2024 14:45
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