DRIVERS-555 Add spec and prose tests for CSOT

[divjotarora]

This PR is a rebased and updated version of #873 and contains the spec/prose tests for CSOT as well as an update to the Unified Test Format Makefile to validate the tests in the client-side-operations-timeout/tests directory. Implementing the new CSOT tests also requires the unified test format changes described in #959.

One of the linked GitHub checks fail because the Unified Test Format Makefile is validating using schema-1.3.json, which does not contain the new features from #959, so the CSOT tests fail validation. I have verified locally that schema validation succeeds if the changes from #959 are incorporated.

Note: Some of the new tests may need an additional serverless requirement if we find that they fail due to known limitations of serverless clusters.

[jmikola]

Noticed missing newline at end of a file (perhaps from template generation), but LGTM.

[DmitryLukyanov]

LGTM

[ShaneHarvey]

Updated to fix merge conflicts. No action needed yet. I'm working on POCing these tests.

[benjirewis]

I'm slowly POC'ing these tests in the Go driver. Left a few comments I had after syncing global-timeoutMS, override-collection-timeoutMS, override-database-timeoutMS and override-operation-timeoutMS.

[benjirewis]

The Go driver expects a saveResultAsEntity field for all the createChangeStream operations in these new tests and errors in its absence. The unified test format does not seem to imply that there must be a saveResultAsEntity field for createChangeStream, so I could just throw away the result of the watch command, but wanted to point out that this deviates from existing createChangeStream operations (by nature of the template used to create the tests).

[ShaneHarvey]

Good catch. The unified test format says (emphasis mine):

... before the resulting change stream might be saved with operation.saveResultAsEntity.

So I think saveResultAsEntity can be optional. I tried adding saveResultAsEntity but it's tedious and error prone because it's mutually exclusive with expectError. Can we leave it optional for now?

[jmikola]

Not my intention to create additional work for anyone, but there may be value in introduce a new valid-pass test in the unified spec to demonstrate createChangeStream being used w/o saveResultAsEntity so that the only example of that usage isn't confined to the CSOT spec.

Feel free to ignore if this seems superfluous.

[ShaneHarvey]

Done: 73a01d1

[benjirewis]

There is no count command in the Go driver (nor in a couple other, new drivers), so I think we'll skip tests with count.

[ShaneHarvey]

Yep drivers without the deprecated count helper will skip tests for count. Note there are already tests for count:

git grep ' name: count$'
source/client-side-encryption/etc/test-templates/count.yml.template:19:      - name: count
source/client-side-encryption/etc/test-templates/count.yml.template:50:      - name: count
source/client-side-encryption/tests/count.yml:19:      - name: count
source/client-side-encryption/tests/count.yml:50:      - name: count
source/crud/tests/v1/read/count-collation.yml:20:            name: count
source/crud/tests/v1/read/count-empty.yml:24:            name: count
source/crud/tests/v1/read/count.yml:48:            name: count
source/crud/tests/v1/read/count.yml:57:            name: count
source/crud/tests/v1/read/count.yml:67:            name: count
source/retryable-reads/tests/legacy/count-serverErrors.yml:26:                    name: count
source/retryable-reads/tests/legacy/count.yml:22:                    name: count
source/transactions/tests/legacy/count.yml:25:      - name: count

[jmikola]

cc: @kmahar re: #1206 (comment)

[benjirewis]

As described in that linked thread, I'll add the logic in Go to automatically skip tests with unsupported operations. Other drivers that decode entire test files and the contained operations into native types will likely find issue with the count and listIndexNames tests.

[benjirewis]

Tests for estimatedDocumentCount expect a count commandStartedEvent in this PR. Depending on the wire version/whether drivers have implemented DRIVERS-2228, an aggregate may be started for estimatedDocumentCount. I think tests with estimatedDocumentCount will need some min/maxServerVersion logic.

[ShaneHarvey]

We can't use wireVersions because the count/aggregate behavior depends on the driver, not the server. Drivers will just need to implement DRIVERS-2228 first.

[benjirewis]

The Go driver has no listIndexNames operation, so we'll skip tests with this operation. We have ListIndexSpecifications, but it's not quite the same.

[ShaneHarvey]

Yep I believe that's expected. Python also does not have a listIndexNames helper.

[jmikola]

FWIW, listIndexNames has existed in the legacy retryable reads spec tests for some time. This is likely the first instance of that operation being used in a unified spec test, but I concur that drivers can/should just skip this as needed.

[benjirewis]

I don't think we'll run any of the tests in this file in the Go driver. Users can pass a Context with a deadline to any operation to specify an operation-level timeout. Allowing users to set timeoutMS on an operation and pass a Context with a deadline would be redundant and confusing.

[jmikola]

On an unrelated note, I realized there's a typo in the test name: overridden

[ShaneHarvey]

What do you think about running these tests by having the test runner translate the timeoutMS argument into a Context with an equivalent deadline? This is what I've done in Python. And I've fixed the typo.

[jmikola]

This might require more discussion in the unified spec, but would the proposal be that drivers implementing timeoutMS via a context modify their execution such that each operation with a timeoutMS option runs within its own context?

[ShaneHarvey]

Yes, but I think the description of that behavior should live in the CSOT test readme, not the unified spec.

[benjirewis]

I can try to do that in Go and report back. @jmikola yep that's my understanding of the proposal.

[jmikola]

but I think the description of that behavior should live in the CSOT test readme, not the unified spec.

No objections to hosting that documentation in the CSOT spec, but I think it'd be prudent to reference the CSOT spec's explanation from Executing an Operation in the unified spec. In the event timeoutMS ever shows up in non-CSOT spec tests, it'd be helpful to have this mentioned somewhere in the test runner implementation directions.

[ShaneHarvey]

My idea doesn't work (at least for Go) so drivers won't be using this approach. Given that I don't think there's anything special to document in the unified format regarding the timeoutMS option.