CDRIVER-4093 fix versioned API sample tests #848
Conversation
| @@ -51,7 +51,7 @@ get_mongodb_download_url_for () | |||
| _DISTRO=$1 | |||
| _VERSION=$2 | |||
|
|
|||
| VERSION_50="5.0.0" | |||
| VERSION_50="5.0.2" | |||
There was a problem hiding this comment.
download-mongodb.sh is copied from mongodb-labs/driver-evergreen-tools. This is required to get 5.0 server version with SERVER-58794 fixed.
On 5.0.0, the collection create command fails against sharded clusters when a versioned API is required. That was causing this test-asan-5.0-sharded-noauth-nosasl-nossl test failure.
| @@ -3592,12 +3607,13 @@ _test_sample_versioned_api_example_1 (void) | |||
| * client = mongoc_client_new (uri_sharded); | |||
| */ | |||
|
|
|||
| client = get_client (); | |||
| client = get_client_for_version_api_example (); | |||
There was a problem hiding this comment.
get_client calls test_framework_new_default_client, which calls test_framework_client_new.
test_framework_client_new applies server API options if required by the test environment.
This test sets server API options on the client. To avoid setting duplicate options, the client should not be created with default server API options.
For more background: the environment variable MONGODB_API_VERSION controls whether test clients have server API options set by default..
MONGODB_API_VERSION is set on the versioned-api variant to run the test suite against servers expecting a server API options in all commands. Those tests are specified in the Versioned API test readme
If the environment variable is set, all clients created in tests MUST declare the ServerApiVersion specified.
There was a problem hiding this comment.
Is it worth adding a comment in the code about the API options note?
There was a problem hiding this comment.
Good idea, I added a note to say that get_client_for_version_api_example returns a client without server API options configured.
| @@ -3592,12 +3607,13 @@ _test_sample_versioned_api_example_1 (void) | |||
| * client = mongoc_client_new (uri_sharded); | |||
| */ | |||
|
|
|||
| client = get_client (); | |||
| client = get_client_for_version_api_example (); | |||
There was a problem hiding this comment.
Is it worth adding a comment in the code about the API options note?
|
|
||
| uri = test_framework_get_uri (); | ||
| client = mongoc_client_new_from_uri (uri); | ||
| ASSERT (client); |
There was a problem hiding this comment.
I'm assuming that since this is test code that we expect to just terminate, there's no need to release the "client" resource?
There was a problem hiding this comment.
Callers of get_client_for_version_api_example are expected to call mongoc_client_destroy to release the client.
|
|
||
| mongoc_server_api_version_from_string ("1", &server_api_version); | ||
| server_api = mongoc_server_api_new (server_api_version); | ||
| mongoc_server_api_deprecation_errors (server_api, true); | ||
|
|
||
| mongoc_client_set_server_api (client, server_api, &error); | ||
| ok = mongoc_client_set_server_api (client, server_api, &error); |
There was a problem hiding this comment.
nit: I'm ok with "ok", but it might be okay-er to just check the call with BSON_ASSERT directly, if this is the only place we're using it. But, really, I'm ok either way!
There was a problem hiding this comment.
Done, here and in the other changes!
|
|
||
| mongoc_server_api_version_from_string ("1", &server_api_version); | ||
| server_api = mongoc_server_api_new (server_api_version); | ||
|
|
||
| mongoc_client_set_server_api (client, server_api, &error); | ||
| ok = mongoc_client_set_server_api (client, server_api, &error); | ||
| BSON_ASSERT (ok); |
There was a problem hiding this comment.
Do we care much that BSON_ASSERT is appearing in the API examples (as opposed to the regular C assert())?
There was a problem hiding this comment.
Good question. Though BSON_ASSERT is part of the public libbson API, and is defined in bson-macros.h, I think it makes sense to use assert since readers are likely to be familiar with assert.
I looked through other examples code for reference. There aren't many assertions, but there are instances of both uses of BSON_ASSERT and assert.
https://github.com/mongodb/mongo-c-driver/blob/3a41062e397638399a6e491ec94bd6c861ec212c/src/libmongoc/examples/example-update.c#L52:L52 uses BSON_ASSERT.
- update download-mongodb.sh - create clients in versioned API examples without server API options set
Addresses test failures introduced in #843 to the versioned API tests and 5.0 sharded tests.