[CDRIVER-6107] Add Test Case Tag Support #2135

vector-of-bool · 2025-10-03T16:41:00Z

This changeset introduces the ability to apply label/tags to test cases, and exposes those as CTest labels.

This also includes new general mutable string and array/vector handling utilities, and refactors a lot of our test suite to use those.

Changes:

6d908aa supports a because clause on all mlib_check assertions. This message is included in the output when the program terminates, and acts as inline documentation for why a particular assertion is present.
d636e33 Brings mstr, the data-owning counterpart to mstr_view. This piece has been in a stash for a while, and was brought out as a better way to handle mutable strings.
9675ae5 Adds vec.t.h, a generic template header that is used to define vector types. Basically:
- #define a type T, and then #include <mlib/vec.t.h>, and it will define a T_vec type that acts as a dynamically sized contiguous array of T objects.
- Additional macros can be defined to add copy/destroy semantics to the vector objects.
f97883e Adds mstr_trim for removing whitespace from string views.
47629d7 Refactors a lot of our test suite code to use mtsr and vec.t.h types. Also adds mlib/str_vec.h which just defines the mstr_vec for the very common "array of strings" type.
b39b795 Adds support for specifying tags/labels associated with test cases:
- The tags are specified within the same string as the test name. (There was an attempt to add a second string parameter for tags, but this was extremely cumbersome to update all of the test cases across the codebase to pass a second empty string, especially with all the different signatures for adding test cases. A future refactor may want to consolidate our many "TestSuite_Add" functions/macros.)
- After the test case name, place one or more spaces, and then a list of [bracketed][tags].
- To support exporting of tags for LoadTests.cmake, the --tests-cmake switch will print CMake code that defines all the test cases. I wanted to use JSON to emit the test cases, but CMake's JSON handling functionality is incredibly slow for very large JSON blobs, so directly emitting CMake code was chosen instead.
- LoadTests.cmake will evaluate the emitted CMake code and use it to call add_test for all the test cases, as well as apply the test case labels and fixtures.
- If a test has a tag [uses:foo], then LoadTests.cmake will add a FIXTURES_REQUIRED of mongoc/fixtures/foo. This currently only applies to the IMDS tests, but will eventually be used for other test case fixtures.

This change allows for test cases to declare any number of associated "tags". The tags are specified after the test case name string as a list of bracketed tags, mimicking Catch2's syntax. The LoadTests.cmake script has been modified to apply test case's declared tags as CTest labels. This also gives us the ability to apply test fixture requirements granularly on only tests that declare their requirement (via a tag).

eramongodb

Initial feedback. TIL RESOURCE_LOCK, this is fantastic. I am also very excited about the mlib_str and mlib_vec APIs.

Some benchmarks, averaged across 10 runs ('~' is standard deviation), testing only /bson/* (438 tests, all safe to execute in parallel) with --no-fork (/bson/value/big_copy is skipped):

test-libmongoc (current): 0.79 secs (~0.05)
test-libmongoc (PR): 0.82 secs (~0.04)
ctest: 4.56 secs (~0.75)
ctest -j 0: 1.57 secs (~0.12)

It is surprising to me that ctest with parallelism is almost twice as slow as the single-threaded test-libmongoc. We may need to explore improving the test registration and filtering setup (in a followup PR is fine) to avoid this performance overhead as much as possible (the eager test registration skips implemented by this PR in TestSuite_Add*() do not seem to be sufficient).

Just a note that the current output of ctest --print-labels is:

All Labels:
  ipv6
  lock:fake-kms
  lock:live-server
  test-libmongoc-generated
  timeout:10
  timeout:20
  timeout:30
  uses:fake_kms_provider_server
  uses:simple-http-server-18000

Update: "json" and "slow" have been removed.

Some questions:

What motivated the addition of [timeout:N] to certain tests and not others? Is the default timeout of 10 seconds too aggressive (e.g. perhaps 60 secs may be preferable)?
What is the purpose of the [json] and [slow] tags, both of which are only used by JSON tests?

src/libmongoc/tests/TestSuite.c

src/libmongoc/src/mongoc/mongoc-openssl-private.h

src/common/src/mlib/str.h

src/common/src/mlib/vec.th

src/libmongoc/tests/TestSuite.c

src/libmongoc/tests/TestSuite.h

src/libmongoc/tests/test-mongoc-client-session.c

src/libmongoc/tests/json-test.c

build/cmake/TestFixtures.cmake

vector-of-bool · 2025-10-24T16:13:57Z

@eramongodb I'm unfortunately aware of the big slowness when running tests via CTest. The issue is indeed caused by the overhead of spawning the subprocess which then needs to register all the test cases, even if it only needs one of them.

I'm hoping in a future change, after more adoption of CTest, to speed it up significantly by simplifying the startup process, but for now it runs fast enough that I don't worry about it.

re: [slow][json] tags: Those were leftover from an earlier attempt to handle timeouts and live tests. I've removed them now.

re: [timeout:N] I wanted to have a much stricter timeout on tests, and only exempt those that were really really slow, and have tests declare their own timeout in-situ, so [timeout:N] was added as a way to do that. It may be sufficient to just set the default timeout really high, but I wanted to catch tests that were slow and specifically call them out in-source. This also gives the ability to select tests that don't need a big timeout (i.e. will run very fast) with ctest --label-exclude timeout:.

src/libmongoc/src/mongoc/mongoc-openssl-private.h

src/common/src/mlib/vec.th

src/common/src/mlib/str.h

src/common/src/mlib/vec.t.h

eramongodb

Minor feedback remaining; otherwise, LGTM!

src/common/src/mlib/str.h

kevinAlbs

Sorry for the slow review. The string and vector utilities look very useful!

benchmarks

How is ctest expected to be invoked? I see a bigger difference locally:

./cmake-build/src/libmongoc/test-libmongoc --no-fork -d --match "/bson/*"         # ~1 second
ctest --test-dir cmake-build -R "^mongoc/bson/*" --output-on-failure --parallel   # ~8 seconds

Granted, ctest is not running in CI yet. So I am OK with addressing performance as future work.

kevinAlbs · 2025-10-27T13:40:08Z

src/common/src/mlib/str.h

+
+/**
+ * @brief Resize an existing or null `mstr`, without initializing any of the
+ * added content other than the null terminator. This operation is potientially


Suggested change

* added content other than the null terminator. This operation is potientially

* added content other than the null terminator. This operation is potentially

kevinAlbs · 2025-10-27T14:00:17Z

src/common/src/mlib/str.h

+      return false;
+   }
+   // Try to (re)allocate the region
+   char *data = (char *)realloc(str->data, alloc_size);


Calls to realloc and free ignore functions set by bson_mem_set_vtable. bson_mem_set_vtable is used by the PHP driver.

I expect mlib is designed not to depend on libbson. Consider adding a function to set custom allocators from bson_mem_set_vtable.

I've thought about how to approach that, and I see two alternatives. I created CDRIVER-6141 to track it separately, as adding that would be a significant change.

kevinAlbs · 2025-10-27T14:19:58Z

src/common/src/mlib/str.h

+ * @brief Replace the content of the given string, attempting to reuse the buffer
+ *
+ * @param inout Pointer to a valid or null `mstr` to be replaced
+ * @param s The new string contents


Suggested change

* @param s The new string contents

* @param s The new string contents. Must not be a view referring to data in `inout`.

Suggest documenting self-assignment is not supported. I expect the mstr_resize_for_overwrite invalidates views to inout.

I foresee this being a footgun going forward, especially if one doesn't know the provenance of the strings in question. I've modified the code to properly handle and test self-assignment/self-insertion/self-formatting.

kevinAlbs · 2025-10-27T14:22:52Z

src/common/src/mlib/str.h

+   mlib_check(n_delete <= n_chars_avail_to_delete);
+   // Compute the new string length
+   size_t new_len = str->len;
+   // This should never fail, because we should try to delete more chars than we have


Suggested change

// This should never fail, because we should try to delete more chars than we have

// This should never fail, because we should never try to delete more chars than we have

kevinAlbs · 2025-10-27T14:35:04Z

src/common/src/mlib/str.h

+ *
+ * @param str The string object to be updated
+ * @param splice_pos The position at which to do the splice
+ * @param n_delete The number of characters to delete at `splice_pos`


Suggested change

* @param n_delete The number of characters to delete at `splice_pos`

* @param n_delete The number of characters to delete at `splice_pos`. Must not delete beyond string length.

Suggest documenting to alert callers of possible assert. Alternatively consider clamping the delete count:

if (n_delete > n_chars_avail_to_delete) { // Clamp the number of chars to delete to the number that are available n_delete = n_chars_avail_to_delete; }

That appears consistent with Javascript's splice, but I do not see an immediate use-case.

Chosen to clamp the deletion

kevinAlbs · 2025-10-27T14:40:53Z

src/common/src/mlib/str.h

+ * @param str The string object to be updated
+ * @param splice_pos The position at which to do the splice
+ * @param n_delete The number of characters to delete at `splice_pos`
+ * @param insert A string to be inserted at `split_pos` after chars are deleted


Suggested change

* @param insert A string to be inserted at `split_pos` after chars are deleted

* @param insert A string to be inserted at `split_pos` after chars are deleted. Must not be a view referring to data in `str`

kevinAlbs · 2025-10-27T14:41:30Z

src/common/src/mlib/str.h

+ * @brief Append a string to the end of some other string.
+ *
+ * @param str The string to be modified
+ * @param suffix The suffix string to be appended onto `*str`


Suggested change

* @param suffix The suffix string to be appended onto `*str`

* @param suffix The suffix string to be appended onto `*str`. Must not be a view referring to data in `str`

Suggest documenting, since this may surprise callers:

mstr s = mstr_copy_cstring("foo"); mstr_append(&s, s); // use-after-free

kevinAlbs · 2025-10-27T14:43:36Z

src/common/src/mlib/str.h

+ *
+ * @param str The string object to be updated
+ * @param needle The non-empty needle string to be searched for.s
+ * @param sub The string to be inserted in place of each `needle`


Suggested change

* @param sub The string to be inserted in place of each `needle`

* @param sub The string to be inserted in place of each `needle`. Must not be a view referring to data in `str`

kevinAlbs · 2025-10-28T13:46:03Z

src/common/src/mlib/vec.th

+ *   will be destroyed and the overall copy will fail.
+ *
+ *   To add a trival copying function, define `VecCopyElement` to
+ *   `VecTrivialCopyElement`.


VecTrivialCopyElement appears unused in this file (aside from Intellisense supporting macros). Is this comment out-dated?

The purpose of VecTrivialCopyElement to be used by includers, like this:

#define T int #define VecCopyElement VecTrivialCopyElement #include "vec.th"

kevinAlbs · 2025-10-30T18:30:02Z

build/cmake/LoadTests.cmake

+    list_select(labels SELECT "^uses:(.*)$" REPLACE "mongoc/fixtures/\\1" OUT fixtures)
+
+    # For any "lock:..." labels, add a resource lock with the corresponding name
+    list_select(labels SELECT "^lock:(.*)$" REPLACE "\\1" OUT lock)


Suggested change

list_select(labels SELECT "^lock:(.*)$" REPLACE "\\1" OUT lock)

list_select(labels SELECT "^lock:(.*)$" REPLACE "\\1" OUT locks)

For consistency with labels and fixtures since there may be more than one lock.

vector-of-bool added 28 commits September 29, 2025 14:32

mlib_check supports an explanatory string with all assertions

6d908aa

Define a mutable string type

d636e33

Add a "vector" template header

9675ae5

Wrapping indexing for vec

8f4ec31

String functions for trimming whitespace

f97883e

Use mlib strings and vec types in TestSuite files

47629d7

Add parens around assignment-as-condition

4e844f6

Add Earthly alias for CTest with uvx

6837d55

The GCP test also requires the fake KMS server

319e351

Windows-only compile error

9de54ab

No SSIZE_MAX on Windows

aed388d

uninit-var in MSVC

b4d93bb

Fix UBSan issue for null pointer arithmetic

11c9aa8

Fix another nullptr-arith warning

91d517b

Tests can declare resource locks in tags

6a0c534

Yet another nullptr-arith

5ac0881

Suppress an over-eager Clang warning

5329c7a

Clean up automatic test properties from tags

24225c0

No -Wmissing-braces anywhere

535b88b

Add an mstr formatting function, change to destroy mstr objects in-place

a8ea90f

Define a CTest fixture for our simple HTTP server

d023131

Tweak default timeout and locking behavior

a044a4c

Add test tags and properties throughout

651d7db

Fix OpenSSL test that requires an SSL context

b9c15bb

Merge branch 'master' into CDRIVER-6107-test-tags

40de7d2

Add missing tags following merge

4ec209f

Conditional content in opeen-private.h

a3f1c7e

vector-of-bool marked this pull request as ready for review October 20, 2025 23:26

vector-of-bool requested a review from a team as a code owner October 20, 2025 23:26

kevinAlbs requested a review from eramongodb October 22, 2025 16:32

eramongodb requested changes Oct 24, 2025

View reviewed changes

vector-of-bool added 8 commits October 24, 2025 09:17

Fix NRVO warning

c530f98

Shift conditional inclusion of openssl detail header

095c845

Tweak error message on invalid test specifier string

09d04a6

Fix: Wrong argument when invoking exec() with mstr

b258f16

Clarify the purpose of JSON suite name matching

427ccd2

Adjust position of lock:live-server for session tests

d448c7a

Merge branch 'master' into CDRIVER-6107-test-tags

f538f6f

Remove extraneous [slow][json] test tags

49189ee

vector-of-bool requested a review from eramongodb October 24, 2025 16:22

eramongodb requested changes Oct 24, 2025

View reviewed changes

vector-of-bool added 7 commits October 27, 2025 09:23

Restore accidentally removed inclusion

bc96437

Switch extension: .t.h -> .th

6bdaa7a

Fix missing extern_c around vec_index_adjust

3cbeb29

PR feedback, change behavior of replace() on empty

bdc88d4

Add "unlikely" annotations around direct overflow checks

6c58ddc

Conditionally select the appropriate printf() attribute

ae5bd52

More PR tweaks

6f41703

vector-of-bool requested a review from eramongodb October 27, 2025 17:27

eramongodb approved these changes Oct 27, 2025

View reviewed changes

src/common/src/mlib/str.h Outdated Show resolved Hide resolved

src/common/src/mlib/str.h Show resolved Hide resolved

src/common/src/mlib/str.h Outdated Show resolved Hide resolved

vector-of-bool added 2 commits October 27, 2025 12:13

Fix: Not adjusting elements after erasure

efa5055

Minor tweaks from PR feedback

ccb2521

kevinAlbs reviewed Oct 30, 2025

View reviewed changes

vector-of-bool added 4 commits October 30, 2025 18:01

Fix mstr APIs to support self-assign/self-insert

445b8c5

mstr splice clamps deletion count

ec37524

spelling

d561be5

Plural "locks"

0d9efd4

vector-of-bool requested a review from kevinAlbs October 31, 2025 05:39

	* added content other than the null terminator. This operation is potientially
	* added content other than the null terminator. This operation is potentially

	* @param s The new string contents
	* @param s The new string contents. Must not be a view referring to data in `inout`.

	// This should never fail, because we should try to delete more chars than we have
	// This should never fail, because we should never try to delete more chars than we have

	* @param n_delete The number of characters to delete at `splice_pos`
	* @param n_delete The number of characters to delete at `splice_pos`. Must not delete beyond string length.

	* @param insert A string to be inserted at `split_pos` after chars are deleted
	* @param insert A string to be inserted at `split_pos` after chars are deleted. Must not be a view referring to data in `str`

	* @param suffix The suffix string to be appended onto `*str`
	* @param suffix The suffix string to be appended onto `*str`. Must not be a view referring to data in `str`

	* @param sub The string to be inserted in place of each `needle`
	* @param sub The string to be inserted in place of each `needle`. Must not be a view referring to data in `str`

	list_select(labels SELECT "^lock:(.*)$" REPLACE "\\1" OUT lock)
	list_select(labels SELECT "^lock:(.*)$" REPLACE "\\1" OUT locks)

[CDRIVER-6107] Add Test Case Tag Support #2135

Are you sure you want to change the base?

[CDRIVER-6107] Add Test Case Tag Support #2135

Uh oh!

Conversation

vector-of-bool commented Oct 3, 2025

Changes:

Uh oh!

eramongodb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vector-of-bool commented Oct 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eramongodb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinAlbs 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eramongodb left a comment •

edited

Loading

eramongodb left a comment •

edited

Loading