Skip to content

Skip tests with dots and dollars in field names on 5.0+ #795

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.

Sign up for GitHub

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 2 commits into from
May 22, 2021
Merged

Skip tests with dots and dollars in field names on 5.0+ #795

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ed1375
Skip tests with dots and dollars in field names on 5.0+
alcaeus May 20, 2021
7b6254f
Add todo and fix clang-format
alcaeus May 21, 2021
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
39 changes: 30 additions & 9 deletions src/libmongoc/tests/test-mongoc-collection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4016,7 +4016,7 @@ _test_insert_validate (insert_fn_t insert_fn)
}

static void
test_insert_bulk_validate (void)
test_insert_bulk_validate (void *ctx)
Copy link
Member

Choose a reason for hiding this comment

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

These tests use error API 2, which means all server-side errors should be reported in the MONGOC_ERROR_SERVER domain. The expectations below use MONGOC_ERROR_COMMAND as the domain, which tells me these are client-side errors.

In that case, why should the server version have any impact on these tests? If anything, I would expect these tests to be removed entirely as part of CDRIVER-3895 since that will remove all client-side validation of dots and dollars. Alternatively, we could change the documents in these tests to violate some other validation rule that will be left in place (e.g. invalid UTF).

Copy link
Member Author

Choose a reason for hiding this comment

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

This test fails with an assertion error: https://evergreen.mongodb.com/task_log_raw/mongo_c_driver_windows_2017_test_latest_sharded_noauth_nosasl_nossl_patch_d08ef111e1a9cf7e71d916f562b46a9d9bf061c6_60a625487742ae6f40858e18_21_05_20_09_01_04/0?type=T#L2378. You are correct that the first bulk insert expects a command error because it uses no insert validation. There is a second insert that specifies MONGOC_INSERT_NO_VALIDATE which expectedly fails on 5.0 and expects a server error.

I was considering rewriting the tests but in the interest of fixing the build so I can get on with the language refactoring (which has the potential of breaking lots of things) I decided to skip these tests for now and revisit them when we implement changes for handling field names with dots and dollars. I've added a TODO to re-enable the tests (also fixed clang-format while I was at it)

{
mongoc_client_t *client;
mongoc_collection_t *collection;
Expand Down Expand Up @@ -4067,14 +4067,14 @@ test_insert_bulk_validate (void)


static void
test_insert_one_validate (void)
test_insert_one_validate (void *ctx)
{
_test_insert_validate (insert_one);
}


static void
test_insert_many_validate (void)
test_insert_many_validate (void *ctx)
{
_test_insert_validate (insert_many);
}
Expand Down Expand Up @@ -6674,10 +6674,24 @@ test_collection_install (TestSuite *suite)
NULL,
test_framework_skip_if_slow_or_live);
TestSuite_AddLive (suite, "/Collection/many_return", test_many_return);
TestSuite_AddLive (
suite, "/Collection/insert_one_validate", test_insert_one_validate);
TestSuite_AddLive (
suite, "/Collection/insert_many_validate", test_insert_many_validate);
TestSuite_AddFull (
suite,
"/Collection/insert_one_validate",
test_insert_one_validate,
NULL,
NULL,
TestSuite_CheckLive,
/* TODO: remove checks when removing validation for dots and dollars */
test_framework_skip_if_max_wire_version_more_than_9);
TestSuite_AddFull (
suite,
"/Collection/insert_many_validate",
test_insert_many_validate,
NULL,
NULL,
TestSuite_CheckLive,
/* TODO: remove checks when removing validation for dots and dollars */
test_framework_skip_if_max_wire_version_more_than_9);
TestSuite_AddMockServerTest (suite, "/Collection/limit", test_find_limit);
TestSuite_AddMockServerTest (
suite, "/Collection/batch_size", test_find_batch_size);
Expand Down Expand Up @@ -6729,8 +6743,15 @@ test_collection_install (TestSuite *suite)
TestSuite_AddLive (suite,
"/Collection/estimated_document_count_live",
test_estimated_document_count_live);
TestSuite_AddLive (
suite, "/Collection/insert_bulk_validate", test_insert_bulk_validate);
TestSuite_AddFull (
suite,
"/Collection/insert_bulk_validate",
test_insert_bulk_validate,
NULL,
NULL,
TestSuite_CheckLive,
/* TODO: remove checks when removing validation for dots and dollars */
test_framework_skip_if_max_wire_version_more_than_9);
TestSuite_AddMockServerTest (suite,
"/Collection/aggregate_with_batch_size",
test_aggregate_with_batch_size);
Expand Down