Skip to content

CXX-3094 Use Catch2 SKIP() and compact reporter #1206

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

Merged
merged 18 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions .evergreen/compile.sh
Original file line number Diff line number Diff line change
Expand Up @@ -72,23 +72,24 @@ if [[ -f "${mongoc_prefix:?}/.evergreen/scripts/find-ccache.sh" ]]; then
find_ccache_and_export_vars "$(pwd)" || true
fi


build_targets=()
cmake_build_opts=()
case "${OSTYPE:?}" in
cygwin)
cmake_build_opts+=("/verbosity:minimal")
cmake_examples_target="examples/examples"
build_targets+=(--target ALL_BUILD --target examples/examples)
;;

darwin* | linux*)
cmake_examples_target="examples"
build_targets+=(--target all --target examples)
;;

*)
echo "unrecognized operating system ${OSTYPE:?}" 1>&2
exit 1
;;
esac
: "${cmake_examples_target:?}"

# Create a VERSION_CURRENT file in the build directory to include in the dist tarball.
python ./etc/calc_release_version.py >./build/VERSION_CURRENT
Expand Down Expand Up @@ -223,21 +224,23 @@ if [[ -n "${REQUIRED_CXX_STANDARD:-}" ]]; then
cmake_flags+=("-DCMAKE_CXX_STANDARD_REQUIRED=ON")
fi

if [[ "${COMPILE_MACRO_GUARD_TESTS:-"OFF"}" == "ON" ]]; then
cmake_flags+=("-DENABLE_MACRO_GUARD_TESTS=ON")
fi

echo "Configuring with CMake flags: ${cmake_flags[*]}"

"${cmake_binary}" "${cmake_flags[@]}" ..

if [[ "${COMPILE_MACRO_GUARD_TESTS:-"OFF"}" == "ON" ]]; then
# We only need to compile the macro guard tests.
"${cmake_binary}" -DENABLE_MACRO_GUARD_TESTS=ON ..
"${cmake_binary}" --build . --config "${build_type:?}" --target test_bsoncxx_macro_guards test_mongocxx_macro_guards -- "${cmake_build_opts[@]}"
exit # Nothing else to be done.
fi

# Regular build and install routine.
"${cmake_binary}" --build . --config "${build_type:?}" -- "${cmake_build_opts[@]}"
"${cmake_binary}" --build . --config "${build_type:?}" --target install -- "${cmake_build_opts[@]}"
"${cmake_binary}" --build . --config "${build_type:?}" --target "${cmake_examples_target:?}" -- "${cmake_build_opts[@]}"
"${cmake_binary}" --build . --config "${build_type:?}" "${build_targets[@]:?}" -- "${cmake_build_opts[@]}"
"${cmake_binary}" --install . --config "${build_type:?}"

if [[ "${_RUN_DISTCHECK:-}" ]]; then
"${cmake_binary}" --build . --config "${build_type:?}" --target distcheck
Expand Down
13 changes: 7 additions & 6 deletions .evergreen/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -262,15 +262,20 @@ else
echo "CRYPT_SHARED_LIB_PATH=${CRYPT_SHARED_LIB_PATH:?}"
fi

run_test() { "$@"; }
test_args=(
--reporter compact
--allow-running-no-tests
)

run_test() { "$@" "${test_args[@]:?}"; }

if [[ "${TEST_WITH_ASAN:-}" == "ON" || "${TEST_WITH_UBSAN:-}" == "ON" ]]; then
export ASAN_OPTIONS="detect_leaks=1"
export UBSAN_OPTIONS="print_stacktrace=1"
export PATH="/usr/lib/llvm-3.8/bin:${PATH:-}"
elif [[ "${TEST_WITH_VALGRIND:-}" == "ON" ]]; then
run_test() {
valgrind --leak-check=full --track-origins=yes --num-callers=50 --error-exitcode=1 --error-limit=no --read-var-info=yes --suppressions=../etc/memcheck.suppressions "$@"
valgrind --leak-check=full --track-origins=yes --num-callers=50 --error-exitcode=1 --error-limit=no --read-var-info=yes --suppressions=../etc/memcheck.suppressions "$@" "${test_args[@]:?}"
}
fi

Expand All @@ -288,10 +293,6 @@ else
run_test ./src/mongocxx/test/test_read_write_concern_specs
run_test ./src/mongocxx/test/test_unified_format_spec

echo "Building examples..."
"${cmake_binary:?}" --build . --target examples
echo "Building examples... done."

# Only run examples if MONGODB_API_VERSION is unset. We do not append
# API version to example clients, so examples will fail when requireApiVersion
# is true.
Expand Down
8 changes: 4 additions & 4 deletions cmake/FetchCatch2.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

include(FetchContent)

message(STATUS "Downloading Catch2...")

function(fetch_catch2)
set(fetch_args "")
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.25.0")
Expand All @@ -26,6 +24,8 @@ function(fetch_catch2)
FetchContent_GetProperties(EP_Catch2)

if(NOT ep_catch2_POPULATED)
message(STATUS "Downloading Catch2...")

# Avoid Catch2 compile warnings from being treated as errors.
string(REPLACE " -Werror" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
string(REPLACE " -Werror" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
Expand All @@ -38,9 +38,9 @@ function(fetch_catch2)
# Catch2 config vars.
set_property(CACHE CATCH_INSTALL_DOCS PROPERTY VALUE OFF)
set_property(CACHE CATCH_INSTALL_EXTRAS PROPERTY VALUE OFF)

message (STATUS "Downloading Catch2... done.")
endif()
endfunction()

fetch_catch2()

message (STATUS "Downloading Catch2... done.")
1 change: 1 addition & 0 deletions cmake/make_dist/MakeDistFiles.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ function (EXECUTE_PROCESS_AND_CHECK_RESULT)
${VARS_COMMAND}
WORKING_DIRECTORY ${VARS_WORKING_DIRECTORY}
RESULT_VARIABLE RESULT
OUTPUT_QUIET
)
if (NOT "${RESULT}" STREQUAL "0")
message (FATAL_ERROR ${VARS_ERROR_MSG})
Expand Down
9 changes: 8 additions & 1 deletion src/bsoncxx/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,14 @@ target_link_libraries(bsoncxx_test_properties_with_main INTERFACE bsoncxx::test_
add_library(bsoncxx::test_properties_with_main ALIAS bsoncxx_test_properties_with_main)

target_link_libraries (test_bson PRIVATE bsoncxx::test_properties_with_main)
add_test(NAME bson COMMAND test_bson)

set(test_args "")
list(APPEND test_args
--reporter compact
--allow-running-no-tests
)

add_test(NAME bson COMMAND test_bson ${test_args})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the test_args variable really needed? Can arguments be inlined into the add_test call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just for consistency with mongocxx tests. I suppose it can be inlined for now until more bsoncxx test executables are added (if any).


# Generate test to ensure macro guards behave properly.
if(ENABLE_MACRO_GUARD_TESTS)
Expand Down
32 changes: 19 additions & 13 deletions src/mongocxx/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -206,23 +206,29 @@ set(THREADS_PREFER_PTHREAD_FLAG ON)
find_package(Threads REQUIRED)
target_link_libraries(test_driver PRIVATE Threads::Threads)

add_test(NAME driver COMMAND test_driver)
add_test(NAME logging COMMAND test_logging)
add_test(NAME instance COMMAND test_instance)
add_test(NAME crud_specs COMMAND test_crud_specs)
add_test(NAME gridfs_specs COMMAND test_gridfs_specs)
add_test(NAME client_side_encryption_specs COMMAND test_client_side_encryption_specs)
add_test(NAME command_monitoring_specs COMMAND test_command_monitoring_specs)
add_test(NAME transactions_specs COMMAND test_transactions_specs)
add_test(NAME retryable_reads_spec COMMAND test_retryable_reads_specs)
add_test(NAME read_write_concern_specs COMMAND test_read_write_concern_specs)
add_test(NAME unified_format_spec COMMAND test_unified_format_spec)
add_test(NAME versioned_api COMMAND test_versioned_api)
set(test_args "")
list(APPEND test_args
--reporter compact
--allow-running-no-tests
)

add_test(NAME driver COMMAND test_driver ${test_args})
add_test(NAME logging COMMAND test_logging ${test_args})
add_test(NAME instance COMMAND test_instance ${test_args})
add_test(NAME crud_specs COMMAND test_crud_specs ${test_args})
add_test(NAME gridfs_specs COMMAND test_gridfs_specs ${test_args})
add_test(NAME client_side_encryption_specs COMMAND test_client_side_encryption_specs ${test_args})
add_test(NAME command_monitoring_specs COMMAND test_command_monitoring_specs ${test_args})
add_test(NAME transactions_specs COMMAND test_transactions_specs ${test_args})
add_test(NAME retryable_reads_spec COMMAND test_retryable_reads_specs ${test_args})
add_test(NAME read_write_concern_specs COMMAND test_read_write_concern_specs ${test_args})
add_test(NAME unified_format_spec COMMAND test_unified_format_spec ${test_args})
add_test(NAME versioned_api COMMAND test_versioned_api ${test_args})
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be refactored as a loop over the test names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.


# Adding this as a test will run it as part of the RUN_TESTS command in MSVC.
# Do not add, since we only test mongohouse on Linux.
if(0)
add_test(mongohouse_specs NAME test_mongohouse_specs COMMAND)
add_test(NAME mongohouse_specs COMMAND test_mongohouse_specs ${test_args})
endif()

set_property(TEST crud_specs APPEND PROPERTY ENVIRONMENT
Expand Down
21 changes: 7 additions & 14 deletions src/mongocxx/test/change_streams.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@ TEST_CASE("Change stream options") {
client mongodb_client{uri{}, test_util::add_test_server_api()};

if (!test_util::is_replica_set(mongodb_client)) {
WARN("skip: change streams require replica set");
return;
SKIP("change streams require replica set");
}

SECTION("Error if both resumeAfter and startAfter are set") {
Expand All @@ -118,8 +117,7 @@ TEST_CASE("Spec Prose Tests") {
client client{uri{}, test_util::add_test_server_api()};

if (!test_util::is_replica_set(client)) {
WARN("skip: change streams require replica set");
return;
SKIP("change streams require replica set");
}

auto db = client["db"];
Expand Down Expand Up @@ -387,8 +385,7 @@ TEST_CASE("Create streams.events and assert we can read a single event", "[min36
instance::current();
client mongodb_client{uri{}, test_util::add_test_server_api()};
if (!test_util::is_replica_set(mongodb_client)) {
WARN("skip: change streams require replica set");
return;
SKIP("change streams require replica set");
}

collection events = mongodb_client["streams"]["events"];
Expand All @@ -408,8 +405,7 @@ TEST_CASE("Give an invalid pipeline", "[min36]") {
instance::current();
client mongodb_client{uri{}, test_util::add_test_server_api()};
if (!test_util::is_replica_set(mongodb_client)) {
WARN("skip: change streams require replica set");
return;
SKIP("change streams require replica set");
}

options::change_stream options{};
Expand Down Expand Up @@ -439,8 +435,7 @@ TEST_CASE("Documentation Examples", "[min36]") {
mongocxx::pool pool{uri{}, options::pool(test_util::add_test_server_api())};
auto mongodb_client = pool.acquire();
if (!test_util::is_replica_set(*mongodb_client)) {
WARN("skip: change streams require replica set");
return;
SKIP("change streams require replica set");
}

collection events = (*mongodb_client)["streams"]["events"];
Expand Down Expand Up @@ -546,8 +541,7 @@ TEST_CASE("Watch 2 collections", "[min36]") {
instance::current();
client mongodb_client{uri{}, test_util::add_test_server_api()};
if (!test_util::is_replica_set(mongodb_client)) {
WARN("skip: change streams require replica set");
return;
SKIP("change streams require replica set");
}

options::change_stream options{};
Expand Down Expand Up @@ -594,8 +588,7 @@ TEST_CASE("Watch a Collection", "[min36]") {
instance::current();
client mongodb_client{uri{}, test_util::add_test_server_api()};
if (!test_util::is_replica_set(mongodb_client)) {
WARN("skip: change streams require replica set");
return;
SKIP("change streams require replica set");
}

options::change_stream options{};
Expand Down
62 changes: 28 additions & 34 deletions src/mongocxx/test/client_helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -492,59 +492,53 @@ void check_outcome_collection(mongocxx::collection* coll, bsoncxx::document::vie
REQUIRE(begin(actual) == end(actual));
}

bool server_has_sessions(const client& conn) {
bool server_has_sessions_impl(const client& conn) {
auto result = get_is_master(conn);
auto result_view = result.view();

if (result_view["logicalSessionTimeoutMinutes"]) {
return true;
}

WARN("skip: server does not support sessions");
return false;
}

bool should_run_client_side_encryption_test(void) {
#ifndef MONGOC_ENABLE_CLIENT_SIDE_ENCRYPTION
WARN("linked libmongoc does not support client side encryption - skipping tests");
return false;
#endif

std::vector<const char*> vars{
"MONGOCXX_TEST_AWS_SECRET_ACCESS_KEY",
"MONGOCXX_TEST_AWS_ACCESS_KEY_ID",
"MONGOCXX_TEST_AZURE_TENANT_ID",
"MONGOCXX_TEST_AZURE_CLIENT_ID",
"MONGOCXX_TEST_AZURE_CLIENT_SECRET",
"MONGOCXX_TEST_CSFLE_TLS_CA_FILE",
"MONGOCXX_TEST_CSFLE_TLS_CERTIFICATE_KEY_FILE",
"MONGOCXX_TEST_GCP_EMAIL",
"MONGOCXX_TEST_GCP_PRIVATEKEY",
};

std::ostringstream os;
os << "Please set environment variables to enable client side encryption tests:\n";
std::copy(std::begin(vars), std::end(vars), std::ostream_iterator<const char*>(os, "\n"));
#if defined(MONGOC_ENABLE_CLIENT_SIDE_ENCRYPTION)

if (std::none_of(std::begin(vars), std::end(vars), std::getenv)) {
os << "Skipping client side encryption tests.\n";
cseeos_result client_side_encryption_enabled_or_skip_impl() {
static const cseeos_result result = [] {
std::vector<const char*> vars{
"MONGOCXX_TEST_AWS_SECRET_ACCESS_KEY",
"MONGOCXX_TEST_AWS_ACCESS_KEY_ID",
"MONGOCXX_TEST_AZURE_TENANT_ID",
"MONGOCXX_TEST_AZURE_CLIENT_ID",
"MONGOCXX_TEST_AZURE_CLIENT_SECRET",
"MONGOCXX_TEST_CSFLE_TLS_CA_FILE",
"MONGOCXX_TEST_CSFLE_TLS_CERTIFICATE_KEY_FILE",
"MONGOCXX_TEST_GCP_EMAIL",
"MONGOCXX_TEST_GCP_PRIVATEKEY",
};

WARN(os.str());
const auto is_set = [](const char* var) -> bool { return std::getenv(var) != nullptr; };

return false;
}
const auto count = std::count_if(vars.begin(), vars.end(), is_set);

if (!std::all_of(std::begin(vars), std::end(vars), std::getenv)) {
os << "Failing client side encryption tests (some environment variables were not set).\n";
if (count == 0) {
return cseeos_result::skip;
}

FAIL(os.str());
if (static_cast<std::size_t>(count) < vars.size()) {
return cseeos_result::fail;
}

return false;
}
return cseeos_result::enable;
}();

return true;
return result;
}

#endif // defined(MONGOC_ENABLE_CLIENT_SIDE_ENCRYPTION)

std::string getenv_or_fail(const std::string env_name) {
auto val = std::getenv(env_name.c_str());
if (!val) {
Expand Down
34 changes: 31 additions & 3 deletions src/mongocxx/test/client_helpers.hh
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,37 @@ auto size(Container c) -> decltype(std::distance(std::begin(c), std::end(c))) {
//
// @return Whether sessions are supported by the client's topology.
//
bool server_has_sessions(const client& conn);

bool should_run_client_side_encryption_test(void);
bool server_has_sessions_impl(const client& conn);

#define SERVER_HAS_SESSIONS_OR_SKIP(conn) \
if (!mongocxx::test_util::server_has_sessions_impl(conn)) { \
SKIP("server does not support session"); \
} \
((void)0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Recommend an else here so that the macro expands to a single if statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually: Does this need to be a macro? Does the SKIP need to expand within the body of the test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this need to be a macro? Does the SKIP need to expand within the body of the test case?

Yes, in order for source location information (filename and line number) to be unique in the skip reason message.


#if defined(MONGOC_ENABLE_CLIENT_SIDE_ENCRYPTION)
enum struct cseeos_result {
enable,
skip,
fail,
};

cseeos_result client_side_encryption_enabled_or_skip_impl();

#define CLIENT_SIDE_ENCRYPTION_ENABLED_OR_SKIP() \
switch (mongocxx::test_util::client_side_encryption_enabled_or_skip_impl()) { \
case mongocxx::test_util::cseeos_result::enable: \
break; \
case mongocxx::test_util::cseeos_result::skip: \
SKIP("CSE environemnt variables not set"); \
case mongocxx::test_util::cseeos_result::fail: \
FAIL("One or more CSE environment variable is missing"); \
} \
((void)0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend wrapping in if (1) ... else ((void)0) so that the macro acts as a single statement.

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.

#else
#define CLIENT_SIDE_ENCRYPTION_ENABLED_OR_SKIP() \
SKIP("linked libmongoc does not support client side encryption")
#endif // defined(MONGOC_ENABLE_CLIENT_SIDE_ENCRYPTION)

std::string getenv_or_fail(const std::string env_name);

Expand Down
Loading