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

eramongodb · 2024-09-09T15:52:27Z

Catch2

Summary

Followup to #1200. Verified by this patch.

Refactors the test suite to use the new SKIP() macro. Updates EVG test scripts to use the Catch2 compact reporter. Reduces output verbosity in EVG compile-and-test tasks.

Skipping Tests

Catch2 3.3.0 introduced the SKIP() macro to explicitly skip test cases and sections at runtime, which uses the same exception-throwing pattern as FAIL() but marks the test case as "skipped" instead of either "succeeded" or "failed".

This means the following pattern of code in the current test suite:

TEST_CASE("example") {
    if (check_skip_conditions()) {
        WARN("Skipping - Skip Reason");
        return;
    }

    test_actions();
}

can now be written as the more straightforward pattern:

TEST_CASE("example") {
    if (check_skip_conditions()) {
        SKIP("Skip Reason");
    }

    test_actions(); // Not evaluated.
}

This also changes Catch2 output from (using [index_view] test cases as an example):

-------------------------------------------------------------------------------
create_one
  commitQuorum option
-------------------------------------------------------------------------------
src/mongocxx/test/index_view.cpp:180
...............................................................................

src/mongocxx/test/index_view.cpp:182: warning:
  skip: commitQuorum option requires a replica set

===============================================================================
All tests passed (38 assertions in 4 test cases)

to:

-------------------------------------------------------------------------------
create_one
  commitQuorum option
-------------------------------------------------------------------------------
src/mongocxx/test/index_view.cpp:180
...............................................................................

src/mongocxx/test/index_view.cpp:182: SKIPPED:
explicitly with message:
  commitQuorum option requires a replica set

================================================================================
test cases:  4 |  3 passed | 1 skipped
assertions: 38 | 38 passed

Note

The "skipped" metric considers a test case where any section has been skipped as a skipped test case. e.g. given:

TEST_CASE("example") {
  SECTION("one") { CHECK(true); }
  SECTION("two") { CHECK(true); SKIP("evaluate and skip"); }
}

Catch2 reports: test cases: 1 | 1 skipped and assertions: 2 | 2 passed.

This PR also addresses potential issues with test case failure reports due to catchorg/Catch2#1292 (comment), as exposed by introduction of the SKIP() macro to run_unified_format_tests_in_env_dir. (CHECK(fn()) where fn() evaluates its own assertions appears to be somewhat of a Catch2 anti-pattern.)

The --allow-running-no-tests flag is required to avoid an entirely-skipped test executable (no test case succeeded or failed) from being treated as a failure. The removal of this flag (such that at least one test case succeeds per test executable) may be considered in the future, but for now, given our skip-heavy test suite, this flag is used instead.

Compact Reporter

In an effort to reduce the verbosity of CI task output, this PR proposes using Catch2's compact reporter.

Again using [index_view] test cases as example, current output for skipped test cases looks as follows:

-------------------------------------------------------------------------------
create_one
  commitQuorum option
-------------------------------------------------------------------------------
src/mongocxx/test/index_view.cpp:180
...............................................................................

src/mongocxx/test/index_view.cpp:182: warning:
  skip: commitQuorum option requires a replica set

===============================================================================
All tests passed (38 assertions in 4 test cases)

With SKIP() and the compact reporter, the output looks as follows:

src/mongocxx/test/index_view.cpp:182: skipped: 'commitQuorum option requires a replica set'
test cases:  4 |  3 passed | 1 skipped
assertions: 38 | 38 passed

Using the compact reporter reduces total output line count in test tasks by about 2K lines (depending on the task/variant).

The compact reporter does not include test case or section name information. This means some care must be taken to avoid ambiguous skip messages, e.g. (note: no distinguishable information differentiating the skipping of individual tests):

src/mongocxx/test/spec/unified_tests/runner.cpp:1155: skipped: 'none of the runOnRequirements were met'
src/mongocxx/test/spec/unified_tests/runner.cpp:1155: skipped: 'none of the runOnRequirements were met'
src/mongocxx/test/spec/unified_tests/runner.cpp:1155: skipped: 'none of the runOnRequirements were met'

To avoid ambiguity, the SKIP() macro should contain uniquely identifying information in its skip reason, e.g.:

src/mongocxx/test/spec/unified_tests/runner.cpp:1154: skipped: 'description := observeSensitiveCommands' with 1 message: 'observeSensitiveCommands: getnonce is observed with observeSensitiveCommands=true: none of the runOnRequirements were met: [ { "maxServerVersion" : "6.1.99" } ]'
src/mongocxx/test/spec/unified_tests/runner.cpp:1154: skipped: 'description := observeSensitiveCommands' with 1 message: 'observeSensitiveCommands: getnonce is not observed with observeSensitiveCommands=false: none of the runOnRequirements were met: [ { "maxServerVersion" : "6.1.99" } ]'
src/mongocxx/test/spec/unified_tests/runner.cpp:1154: skipped: 'description := observeSensitiveCommands' with 1 message: 'observeSensitiveCommands: getnonce is not observed by default: none of the runOnRequirements were met: [ { "maxServerVersion" : "6.1.99" } ]'

or, preferably, be called within the test case or section that is being skipped for more accurate source location information, e.g.:

src/mongocxx/test/change_streams.cpp:98: skipped: 'change streams require replica set'
src/mongocxx/test/change_streams.cpp:120: skipped: 'change streams require replica set'
src/mongocxx/test/change_streams.cpp:388: skipped: 'change streams require replica set'
...

Most of the substantial changes in this PR pertain to making skip messages uniquely identifiable.

Quirks

When a test case is skipped, informational log messages are truncated to output only the first such message along with the skip reason. These may be used in place of uniquely identifying info in the skip reason itself, although it is still preferable to use the skip reason for reliability. e.g. given:

TEST_CASE("example") {
  INFO("first");
  INFO("second");
  INFO("third");
  SKIP("skip reason"); // Line 5
}

The Catch2 compact reporter outputs:

main.cpp:5: skipped: 'first' with 3 messages: 'skip reason'

When a test case fails, all informational messages are emitted in full. e.g. given:

TEST_CASE("example") {
  INFO("first");
  INFO("second");
  INFO("third");
  FAIL("fail reason"); // Line 5
}

The Catch2 compact reporter outputs:

main.cpp:5: failed: explicitly with 4 messages: 'first' and 'second' and 'third' and 'fail reason'

Given output is typically overwhemingly dominanted by skipped test case messages, the absence of test case or section name(s) on potential assertion failure is considered an acceptable compromise. If further contextual information is required beyond what log messages reveal, the --reporter compact flag may be temporarily disabled in a patch build as needed.

If these quirks of the compact reporter are considered undesirable (more verbose output logs with contextual information included by default is preferable), the changes related to using the compact reporter may be removed from this PR. (However, it is my opinion that the compact reporter encourages writing cleaner tests.)

Further Output Reduction

Some minor adjustments to the compile-and-test scripts further reduce output of EVG tasks by about ~1.5K lines (depending on the task/variant):

Repeated invocation of CMake configure and build commands: these have been reduced when able (taking advantage of CMake's support for multiple --target <target> flags).
The OUTPUT_QUIET argument is added to execute_process_and_check_result to reduce distcheck output (which was nearly doubling the output line count).
Removed a redundant rebuild of examples executables in test.sh.

…LED_OR_SKIP

vector-of-bool

Good changes! My only comments are relatively minor things.

vector-of-bool · 2024-09-10T16:25:47Z

src/bsoncxx/test/CMakeLists.txt

+set(test_args "")
+list(APPEND test_args
+    --reporter compact
+    --allow-running-no-tests
+)
+
+add_test(NAME bson COMMAND test_bson ${test_args})


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

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).

vector-of-bool · 2024-09-10T16:27:01Z

src/mongocxx/test/CMakeLists.txt

+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})


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

Good idea. Done.

vector-of-bool · 2024-09-10T16:29:17Z

src/mongocxx/test/client_helpers.hh

+    }                                                           \
+    ((void)0)


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

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

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.

vector-of-bool · 2024-09-10T16:30:13Z

src/mongocxx/test/client_helpers.hh

+    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)


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

kevinAlbs

The efforts to reduce log noise are much appreciated. LGTM with one suggestion and a question.

src/mongocxx/test/spec/client_side_encryption.cpp

src/mongocxx/test/CMakeLists.txt

eramongodb added 12 commits September 9, 2024 10:41

Scope "Downloading" messages to avoid misleading repeated output

e5791e0

Use compact reporter for Catch2 tests

7ab4ca1

Use DYNAMIC_SECTION within loops

7feb6e2

should_run_client_side_encryption_test -> CLIENT_SIDE_ENCRYPTION_ENAB…

8edc084

…LED_OR_SKIP

server_has_sessions -> SERVER_HAS_SESSIONS_OR_SKIP

e61322a

Replace WARN + control flow with SKIP

adfe32b

Add --allow-running-no-tests to test flags

f39a1c6

Avoid Catch2 issue 1292

b8c3ee1

should_skip_spec_test -> CHECK_IF_SKIP_SPEC_TEST

0d6df30

Reduce configure and build command invocations

1ba41f4

Add OUTPUT_QUIET to execute_process_and_check_result

4831e3f

Remove redundant build of examples in test.sh

88645c0

eramongodb requested a review from kevinAlbs September 9, 2024 15:52

eramongodb self-assigned this Sep 9, 2024

kevinAlbs requested a review from vector-of-bool September 9, 2024 16:36

vector-of-bool approved these changes Sep 10, 2024

View reviewed changes

eramongodb added 4 commits September 10, 2024 12:23

Inline test args for test_bson

d1f873e

Use foreach() over mongocxx test executables

d715633

Use if-else statement for macros

9a690b6

Add missing conditional in if statement

4b7d9e3

kevinAlbs approved these changes Sep 11, 2024

View reviewed changes

src/mongocxx/test/spec/client_side_encryption.cpp Show resolved Hide resolved

src/mongocxx/test/CMakeLists.txt Outdated Show resolved Hide resolved

eramongodb added 2 commits September 11, 2024 13:07

Consistent plurality for spec tests

14bf375

Consistent plurality for spec tests, part 2

1417362

eramongodb merged commit fb26981 into mongodb:master Sep 11, 2024
44 of 49 checks passed

eramongodb deleted the cxx-catch2 branch September 11, 2024 18:57

eramongodb mentioned this pull request Oct 4, 2024

Use SKIP() in mongocxx::spec::run_tests_in_suite #1226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

eramongodb commented Sep 9, 2024

Uh oh!

vector-of-bool left a comment

Uh oh!

vector-of-bool Sep 10, 2024

Uh oh!

eramongodb Sep 10, 2024

Uh oh!

vector-of-bool Sep 10, 2024

Uh oh!

eramongodb Sep 10, 2024

Uh oh!

vector-of-bool Sep 10, 2024

Uh oh!

vector-of-bool Sep 10, 2024

Uh oh!

eramongodb Sep 10, 2024

Uh oh!

vector-of-bool Sep 10, 2024

Uh oh!

eramongodb Sep 10, 2024

Uh oh!

kevinAlbs left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

eramongodb commented Sep 9, 2024

Catch2

Summary

Skipping Tests

Compact Reporter

Quirks

Further Output Reduction

Uh oh!

vector-of-bool left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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