-
Notifications
You must be signed in to change notification settings - Fork 888
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
GODRIVER-3217 Use the manually-specified maxTimeMS on Find and Aggregate if it would be ommitted by CSOT #1644
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,6 +455,46 @@ func TestCSOT(t *testing.T) { | |
bsoncore.ErrElementNotFound, | ||
"expected maxTimeMS BSON value to be missing, but is present") | ||
}) | ||
|
||
// TODO(GODRIVER-2944): Remove this test once the "timeoutMode" option is | ||
// supported. | ||
mt.RunOpts("Find always sends manually-specified maxTimeMS", csotOpts, func(mt *mtest.T) { | ||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
// Manually set a maxTimeMS on the Find and assert that it's sent with | ||
// the command, even when CSOT is enabled. | ||
cursor, err := mt.Coll.Find( | ||
ctx, | ||
bson.D{}, | ||
options.Find().SetMaxTime(5*time.Second)) | ||
require.NoError(mt, err, "Find error") | ||
err = cursor.Close(context.Background()) | ||
require.NoError(mt, err, "Cursor.Close error") | ||
|
||
evt := getStartedEvent(mt, "find") | ||
assertMaxTimeMSIsSet(mt, evt.Command) | ||
}) | ||
|
||
// TODO(GODRIVER-2944): Remove this test once the "timeoutMode" option is | ||
// supported. | ||
mt.RunOpts("Aggregate always sends manually-specified maxTimeMS", csotOpts, func(mt *mtest.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same considerations for "Find always sends manually-specified maxTimeMS" apply to the aggregate counterpart. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added test cases for the permutations described in the other comment. |
||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
// Manually set a maxTimeMS on the Aggregate and assert that it's sent with | ||
// the command, even when CSOT is enabled. | ||
cursor, err := mt.Coll.Aggregate( | ||
ctx, | ||
bson.D{}, | ||
options.Aggregate().SetMaxTime(5*time.Second)) | ||
require.NoError(mt, err, "Aggregate error") | ||
err = cursor.Close(context.Background()) | ||
require.NoError(mt, err, "Cursor.Close error") | ||
|
||
evt := getStartedEvent(mt, "aggregate") | ||
assertMaxTimeMSIsSet(mt, evt.Command) | ||
}) | ||
} | ||
|
||
func TestCSOT_errors(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is being run with a client-level timeout of 10 seconds, set by previous subtests. The csotOpts should be reset before executing to test that the deprecated operation-level maxTimeMS value is being propagated to command message when omitting CSOT.
Suggest adding the following function to test that a 5s timeout is being used:
Also suggest creating an additional test ensuring behavior defined in the Validation and Overrides section of the CSOT specifications:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test matrix should look something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current test case is basically
so setting
timeoutMS
is intentional. However, the test doesn't cover the rest of the cases, which could be confusing (especially because the test description is actually incorrect, since a manually-setmaxTimeMS
is ignored when a client-level timeout is set but no operation-level timeout is set).I'll update the test to cover the additional cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added test cases for the permutations you described.