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-943 Convert retryable writes command construction tests to unified format #1697

Merged
38 changes: 4 additions & 34 deletions source/retryable-writes/tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,40 +65,8 @@ allow the fail point to trigger on the second command.

## Command Construction Tests

Drivers should also assert that command documents are properly constructed with or without a transaction ID, depending
on whether the write operation is supported.
[Command Logging and Monitoring](../../command-logging-and-monitoring/command-logging-and-monitoring.md) may be used to
check for the presence of a `txnNumber` field in the command document. Note that command documents may always include an
`lsid` field per the [Driver Session](../../sessions/driver-sessions.md) specification.

These tests may be run against both a replica set and shard cluster.

Drivers should test that transaction IDs are never included in commands for unsupported write operations:

- Write commands with unacknowledged write concerns (e.g. `{w: 0}`)
- Unsupported single-statement write operations
- `updateMany()`
- `deleteMany()`
- Unsupported multi-statement write operations
- `bulkWrite()` that includes `UpdateMany` or `DeleteMany`
- Unsupported write commands
- `aggregate` with write stage (e.g. `$out`, `$merge`)

Drivers should test that transactions IDs are always included in commands for supported write operations:

- Supported single-statement write operations
- `insertOne()`
- `updateOne()`
- `replaceOne()`
- `deleteOne()`
- `findOneAndDelete()`
- `findOneAndReplace()`
- `findOneAndUpdate()`
- Supported multi-statement write operations
- `insertMany()` with `ordered=true`
- `insertMany()` with `ordered=false`
- `bulkWrite()` with `ordered=true` (no `UpdateMany` or `DeleteMany`)
- `bulkWrite()` with `ordered=false` (no `UpdateMany` or `DeleteMany`)
Command construction tests asserting on the absence or presence of the `txnNumber` field have been replaced by command
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this wording is a little confusing - originally it made me think we are no longer asserting on txnNumber.

I think it'd be better to remove this section, and if we want to include this information somewhere, we can expand in the changelog instead. Something like the following:

Convert command construction tests to unified format. In the retryable writes unified tests, the unified spec runner now asserts on the absence or presence of the txnNumber field using 'Command Monitoring'.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The specs README states:

Outdated tests must not be removed completely, but may be marked as such (e.g. by striking through or replacing the entire test with a note (e.g. Removed).

I think it's useful to maintain this section as a reference for drivers that have not yet removed these tests. WDYT about adding a strikethrough to the section title and rephrasing this as:

These prose tests have been removed in favor of unified format tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, we can also rephrase it to something like.

In the retryable writes unified tests, the unified spec runner now asserts on the absence or presence of the txnNumber field using 'Command Monitoring'.

This or the strikethrough works!

event assertions in the retryable writes unified tests.

## Prose Tests

Expand Down Expand Up @@ -297,6 +265,8 @@ debugger, code coverage tool, etc.

## Changelog

- 2024-10-29: Convert command construction tests to unified format.

- 2024-05-30: Migrated from reStructuredText to Markdown.

- 2024-02-27: Convert legacy retryable writes tests to unified format.
Expand Down
144 changes: 144 additions & 0 deletions source/retryable-writes/tests/unified/aggregate-out-merge.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

65 changes: 65 additions & 0 deletions source/retryable-writes/tests/unified/aggregate-out-merge.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
description: "aggregate with $out/$merge does not set txnNumber"

schemaVersion: "1.3"

runOnRequirements:
- minServerVersion: "3.6"
topologies:
- replicaset
- sharded
- load-balanced

createEntities:
- client:
id: &client0 client0
observeEvents: [ commandStartedEvent ]
- database:
id: &database0 database0
client: *client0
databaseName: &database0Name retryable-writes-tests
- collection:
id: &collection0 collection0
database: *database0
collectionName: &collection0Name coll0

initialData:
# The output collection must already exist for $merge on a sharded cluster
- collectionName: &mergeCollection mergeCollection
databaseName: *database0Name
documents: []

tests:
- description: "aggregate with $out does not set txnNumber"
operations:
- object: *collection0
name: aggregate
arguments:
pipeline:
- { $sort: { x: 1 } }
- { $match: { _id: { $gt: 1 } } }
- { $out: outCollection }
expectEvents:
- client: client0
events:
- commandStartedEvent:
commandName: aggregate
command:
txnNumber: { $$exists: false }
- description: "aggregate with $merge does not set txnNumber"
runOnRequirements:
- minServerVersion: "4.1.11"
operations:
- object: *collection0
name: aggregate
arguments:
pipeline:
- { $sort: { x: 1 } }
- { $match: { _id: { $gt: 1 } } }
- { $merge: { into: *mergeCollection } }
expectEvents:
- client: client0
events:
- commandStartedEvent:
commandName: aggregate
command:
txnNumber: { $$exists: false }
Loading
Loading