DRIVERS-555 Unified test format changes for CSOT #959

divjotarora · 2021-04-13T02:07:26Z

This PR is a rebased and updated version of #873. It only contains Unified Test Format changes and modifications/additions to tests in the valid-pass and invalid directories. The actual CSOT spec tests will be introduced in another PR to avoid making this one larger than necessary.

Notes for reviewers:

This PR bumps the schema version in the unified test format to 1.8 because 1.9
Many of the existing tests for collectionData in the invalid directory weren't functioning as intended because they had an additional foo property in the collection entity declaration. As a result, they were considered invalid because the collection entity was not valid and the initialData field was not being checked. These have been fixed in this PR.
The aforementioned collectionData test files were renamed from collectionData* to initialCollectionData* to reflect a similar change in the test format spec. A new set of outcomeCollectionData* tests have also been added.

jmikola

Don't forget version bump and changlog entries in the spec RST.

source/unified-test-format/tests/invalid/expectedError-isTimeoutError-type.yml

source/unified-test-format/tests/invalid/initialCollectionData-additionalProperties.yml

source/unified-test-format/tests/invalid/initialCollectionData-collectionName-required.yml

source/unified-test-format/tests/invalid/initialCollectionData-collectionName-type.yml

source/unified-test-format/schema-1.5.json

source/unified-test-format/tests/valid-pass/matches-lte-operator.yml

source/unified-test-format/unified-test-format.rst

source/unified-test-format/tests/valid-pass/initialCollectionData-collectionOptions.yml

jmikola · 2021-04-14T03:44:40Z

source/unified-test-format/tests/valid-pass/initialCollectionData-collectionOptions.yml

+      - name: runCommand
+        object: *database0
+        arguments:
+          commandName: collStats


I know #944 was closed and moved internally for the time being, but heads up to @patrickfreed in case this test will need to be skipped for serverless. IIRC, $collStats isn't supported but I'm not sure about the generic command.

Perhaps there's a more general concern with tests running arbitrary commands that might not be in the versioned API, which will required auditing during the course of that project.

Thanks for the heads up. I made a note to investigate this and skip any of the tests added in this PR or in #960 if they're incompatible with serverless.

Bumping this previously resolved comment.

Unsupported Commands in Serverless Instances doesn't include collStats so I think we're fine here. Just a reminder to run these tests through Atlas Serverless if you get a chance so we double-check before merging. Feel free to resolve after doing so.

In python we don't run the unified test format tests themselves against serverless. Do you think we're supposed to?

Not required by serverless testing, so totally up to you.

I don't think the page I linked above existed when this PR was originally opened, so I just wanted to circle back and cloe the loop on this thread in case anyone comes across it down the line.

source/unified-test-format/tests/valid-pass/matches-lte-operator.yml

DmitryLukyanov

One question and then LGTM

source/unified-test-format/unified-test-format.rst

benjirewis · 2022-04-15T19:43:03Z

I wonder if we should add a valid-pass test testing the functionality of the new createEntities operation as distinct from the file-level createEntities.

source/unified-test-format/tests/valid-pass/initialCollectionData-collectionOptions.yml

ShaneHarvey · 2022-04-22T23:22:14Z

I wonder if we should add a valid-pass test testing the functionality of the new createEntities operation as distinct from the file-level createEntities.

Done. I also resolved the merge conflicts and updated the spec version to 1.8. The unified tests are passing in Python.

benjirewis

Question about schema version but otherwise LGTM; new test works in the Go driver and seems to test the correct functionality.

source/unified-test-format/tests/valid-pass/createEntities-operation.yml

source/unified-test-format/schema-1.8.json

jmikola · 2022-04-26T16:50:38Z

source/unified-test-format/schema-1.8.json

+      "properties": {
+        "collectionName": { "type": "string" },
+        "databaseName": { "type": "string" },
+        "collectionOptions": { "type": "object" },


Introducing collectionOptions is the only reason you needed to differentiate initialCollectionData from outcomeCollectionData, correct?

To differentiate this from options used when selecting a collection (e.g. collectionOrDatabaseOptions), what do you think about calling this createOptions? I realize the purpose of this field is covered in the spec itself, but I was thinking that may make the test files more self-documenting. I don't feel strongly about this if you'd rather stick with what you have here.

source/unified-test-format/tests/invalid/collectionOrDatabaseOptions-timeoutMS-type.yml

source/unified-test-format/tests/valid-pass/createEntities-operation.yml

jmikola · 2022-04-26T16:59:31Z

source/unified-test-format/tests/valid-pass/createEntities-operation.yml

+        arguments:
+          entities:
+            - client:
+                id: client1


Would it make sense to use YAML anchors/references for entity IDs? I see you did so in some of the other valid-pass tests in this PR. If this gets used as an example for writing actual spec tests it may be prudent to demonstrate the behavior we'd like others to follow.

I don't feel strongly if you'd rather leave this as-is, though.

I wrote this test myself, the other test were written by Divjot ages ago hence the discrepancy. Is there any benefit to using anchors here? They make the file more verbose with no benefit in my opinion.

Only benefit is if other spec authors happen to use these files as examples down the line, but even then we'd likely catch the lack of anchors in code review. I have no objection to leaving this as-is.

Feel free to resolve after reading.

To be clear, I don't understand what benefit there is to using anchors for entity IDs in any test at all (not just this one).

In the case of entity IDs where the anchor and value are almost always identical, I can see why it seems pointless. I think one small benefit is that a typo is more likely to be caught during JSON conversion instead of a runtime error when executing the tests (due to a failed entity lookup).

For database and collection names, I think the anchors serve a much greater purpose since the values are often very different from the anchor names.

Gotcha. Catching typos does seem like a useful benefit.

source/unified-test-format/tests/valid-pass/matches-lte-operator.yml

source/unified-test-format/unified-test-format.rst

source/unified-test-format/schema-1.8.json

source/unified-test-format/unified-test-format.rst

…ke timeoutMS an integer, other fixes

ShaneHarvey

Rebased and updated to schema version 1.9 with the suggested changes.

source/unified-test-format/schema-1.8.json

source/unified-test-format/tests/invalid/collectionOrDatabaseOptions-timeoutMS-type.yml

source/unified-test-format/tests/valid-pass/createEntities-operation.yml

ShaneHarvey · 2022-04-26T20:40:15Z

source/unified-test-format/tests/valid-pass/createEntities-operation.yml

+        arguments:
+          entities:
+            - client:
+                id: client1


I wrote this test myself, the other test were written by Divjot ages ago hence the discrepancy. Is there any benefit to using anchors here? They make the file more verbose with no benefit in my opinion.

source/unified-test-format/tests/valid-pass/entity-cursor-iterateOnce.yml

ShaneHarvey · 2022-04-26T20:48:02Z

source/unified-test-format/tests/valid-pass/initialCollectionData-collectionOptions.yml

+      - name: runCommand
+        object: *database0
+        arguments:
+          commandName: collStats


In python we don't run the unified test format tests themselves against serverless. Do you think we're supposed to?

source/unified-test-format/tests/valid-pass/matches-lte-operator.yml

source/unified-test-format/unified-test-format.rst

source/unified-test-format/schema-1.9.json

ShaneHarvey · 2022-04-27T21:16:07Z

I resolved the merge conflicts and cleaned up the duplicate initialCollectionData and outcomeCollectionData tests. Ready for another look.

jmikola

LGTM. Optional suggestion to remove some lines from createEntities-operation.yml.

jmikola · 2022-04-27T22:03:58Z

source/unified-test-format/tests/valid-pass/createEntities-operation.yml

+      database: *database0
+      collectionName: &collection0Name coll0
+
+initialData:


Conceivably, you could delete the top-level createEntities and initialData blocks in this test. They don't appear to be used for anything.

divjotarora requested a review from jmikola as a code owner April 13, 2021 02:07

divjotarora mentioned this pull request Apr 13, 2021

DRIVERS-555 Add spec and prose tests for CSOT #960

Merged

jmikola reviewed Apr 13, 2021

View reviewed changes

divjotarora requested a review from jmikola April 13, 2021 17:04

jmikola reviewed Apr 14, 2021

View reviewed changes

divjotarora requested a review from jmikola April 14, 2021 04:54

jmikola approved these changes Apr 14, 2021

View reviewed changes

divjotarora requested review from rozza and DmitryLukyanov April 15, 2021 18:59

DmitryLukyanov approved these changes Apr 21, 2021

View reviewed changes

source/unified-test-format/unified-test-format.rst Show resolved Hide resolved

DRIVERS-555 Unified test format changes for CSOT

d4aa44e

divjotarora force-pushed the csot-test-format-changes branch from dd50486 to d4aa44e Compare April 30, 2021 00:24

rozza removed their request for review September 8, 2021 13:52

benjirewis mentioned this pull request Apr 15, 2022

GODRIVER-2384 Unified test runner changes for CSOT. mongodb/mongo-go-driver#912

Merged

benjirewis reviewed Apr 19, 2022

View reviewed changes

source/unified-test-format/tests/valid-pass/initialCollectionData-collectionOptions.yml Outdated Show resolved Hide resolved

ShaneHarvey added 2 commits April 22, 2022 12:43

DRIVERS-555 rename schema-1.8.json

c2ca520

Merge branch 'master' into divjotarora-csot-test-format-changes

716f9c4

divjotarora requested a review from a team as a code owner April 22, 2022 20:05

DRIVERS-555 Restore schema-1.4.json

48136ee

mongodb deleted a comment from benjirewis Apr 22, 2022

ShaneHarvey added 3 commits April 22, 2022 14:14

DRIVERS-555 Require 3.6 for collStats test

c80874d

DRIVERS-555 Add valid-pass test for createEntities operation

ff0b3c9

DRIVERS-555 Fix createEntities test

8ff24ee

ShaneHarvey requested review from benjirewis and jmikola April 22, 2022 23:22

benjirewis approved these changes Apr 26, 2022

View reviewed changes

source/unified-test-format/tests/valid-pass/createEntities-operation.yml Outdated Show resolved Hide resolved

jmikola mentioned this pull request Apr 26, 2022

DRIVERS-2017 Add clientEncryption entity to unified test format #1188

Merged

jmikola reviewed Apr 26, 2022

View reviewed changes

ShaneHarvey added 3 commits April 26, 2022 15:40

DRIVERS-555 Merge outcomeCollectionData and initialCollectionData, ma…

f60cc1a

…ke timeoutMS an integer, other fixes

DRIVERS-555 bump 1.9

39d3f34

Merge branch 'master' into csot-test-format-changes

103f6c1

ShaneHarvey reviewed Apr 26, 2022

View reviewed changes

ShaneHarvey requested a review from jmikola April 26, 2022 23:20

jmikola reviewed Apr 27, 2022

View reviewed changes

source/unified-test-format/schema-1.9.json Show resolved Hide resolved

ShaneHarvey added 2 commits April 27, 2022 14:10

DRIVERS-555 rename initialCollectionData -> collectionData

a6b5745

Merge branch 'master' into csot-test-format-changes

92e26af

ShaneHarvey requested a review from jmikola April 27, 2022 21:15

DRIVERS-555 Use anchors

f5ef38a

jmikola approved these changes Apr 27, 2022

View reviewed changes

DRIVERS-555 Remove redundant createEntities and initialData

3f4d7ca

ShaneHarvey merged commit a8a7d01 into mongodb:master Apr 27, 2022

prestonvasquez mentioned this pull request Jun 1, 2023

GODRIVER-2736 Deprecate currentOp/collStats commands mongodb/mongo-go-driver#1280

Merged

W-A-James mentioned this pull request May 24, 2024

test(NODE-6120): Implement Unified test runner changes for CSOT mongodb/node-mongodb-native#4121

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRIVERS-555 Unified test format changes for CSOT #959

DRIVERS-555 Unified test format changes for CSOT #959

divjotarora commented Apr 13, 2021 •

edited by ShaneHarvey

Loading

jmikola left a comment

jmikola Apr 14, 2021

divjotarora Apr 14, 2021

jmikola Apr 26, 2022

ShaneHarvey Apr 26, 2022

jmikola Apr 27, 2022

DmitryLukyanov left a comment

benjirewis commented Apr 15, 2022

ShaneHarvey commented Apr 22, 2022 •

edited

Loading

benjirewis left a comment

jmikola Apr 26, 2022

jmikola Apr 26, 2022

ShaneHarvey Apr 26, 2022

jmikola Apr 27, 2022 •

edited

Loading

ShaneHarvey Apr 27, 2022

jmikola Apr 27, 2022

ShaneHarvey Apr 27, 2022

ShaneHarvey left a comment

ShaneHarvey Apr 26, 2022

ShaneHarvey Apr 26, 2022

ShaneHarvey commented Apr 27, 2022

jmikola left a comment

jmikola Apr 27, 2022

ShaneHarvey Apr 27, 2022

DRIVERS-555 Unified test format changes for CSOT #959

DRIVERS-555 Unified test format changes for CSOT #959

Conversation

divjotarora commented Apr 13, 2021 • edited by ShaneHarvey Loading

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryLukyanov left a comment

Choose a reason for hiding this comment

benjirewis commented Apr 15, 2022

ShaneHarvey commented Apr 22, 2022 • edited Loading

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey commented Apr 27, 2022

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divjotarora commented Apr 13, 2021 •

edited by ShaneHarvey

Loading

ShaneHarvey commented Apr 22, 2022 •

edited

Loading

jmikola Apr 27, 2022 •

edited

Loading