Skip to content
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

DRIVERS-1851 Skip crud-v1 tests for $out and collation on Serverless #1041

Merged

Conversation

patrickfreed
Copy link
Contributor

DRIVERS-1851

Atlas Serverless doesn't currently support $out aggregations or collation, so the associated crud-v1 tests need to be skipped. This involved slightly amending the crud-v1 format.

@patrickfreed patrickfreed requested a review from a team as a code owner July 15, 2021 18:04
@patrickfreed patrickfreed requested review from benjirewis and removed request for a team July 15, 2021 18:04
and placed under ``unified/``. Until such time that all original tests have been ported
to the unified test format, tests in each format will be grouped in their own subdirectory:

- ``v1/``: Legacy-v1 format tests
- ``unified/``: Tests using the `unified test format <../../../../unified-test-format/unified-test-format.rst>`_
- ``unified/``: Tests using the `unified test format <../../unified-test-format/unified-test-format.rst>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks for fixing these.

Copy link
Contributor

@MikalaiMazurenka MikalaiMazurenka left a comment

Choose a reason for hiding this comment

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

Looks good, but before approving I will need to check with serverless team whether bulkWrite operation succeeding on serverless when request has a collation is an intended behavior. If it is, changes in bulkWrite and adjacent tests should be reverted. If it's a server bug, then no further changes required.

@@ -22,6 +22,7 @@
}
],
"minServerVersion": "3.4",
"serverless": "forbid",
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed this test because it passed in C# driver.
The bulkWrite operation didn't fail despite having a collation present. The difference with other operations is that they had collation specified as operation option while bulkWrite has this option set for the individual requests.
Which means that it is either an intended behavior or a bug in serverless implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

To add another data point, these tests were failing in Node.

@@ -14,6 +14,7 @@
}
],
"minServerVersion": "3.4",
"serverless": "forbid",
Copy link
Contributor

Choose a reason for hiding this comment

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

This test didn't fail in C# driver either, because the deleteMany is executed as bulkWrite operation with a single deleteMany request.

@@ -14,6 +14,7 @@
}
],
"minServerVersion": "3.4",
"serverless": "forbid",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for deleteMany

@@ -10,6 +10,7 @@
}
],
"minServerVersion": "3.4",
"serverless": "forbid",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same behavior with the operation being an individual request of bulkWrite operation.

@@ -14,6 +14,7 @@
}
],
"minServerVersion": "3.4",
"serverless": "forbid",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same behavior with the operation being an individual request of bulkWrite operation.

@@ -10,6 +10,7 @@
}
],
"minServerVersion": "3.4",
"serverless": "forbid",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same behavior with the operation being an individual request of bulkWrite operation.

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

Crud v1 passes in Node with these changes 👍

@MikalaiMazurenka MikalaiMazurenka self-requested a review July 19, 2021 12:10
Copy link
Contributor

@MikalaiMazurenka MikalaiMazurenka left a comment

Choose a reason for hiding this comment

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

LGTM!
This indeed was a bug

@patrickfreed patrickfreed merged commit a73bf05 into mongodb:master Jul 19, 2021
kevinAlbs pushed a commit to kevinAlbs/specifications that referenced this pull request Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants