-
Notifications
You must be signed in to change notification settings - Fork 265
Support 1.4 schema in unified test runner and sync tests for various 5.0-compat features #834
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
Conversation
f96c8e0
to
d342943
Compare
5f40a08
to
3c47b43
Compare
This updates individual tests pertaining to dots/dollars. Synced with mongodb/specifications@44f8310 Removes skips previously added in a66f31f and ea9853a
Remove automatic result iteration in Operation::execute(), since ChangeStreams will no longer be the lone exception once createFindCursor is implemented. Additionally, filter/pipeline args need not default since they are required by the CRUD and change stream specs and present in all valid spec tests. The previous behavior would conflict with a forthcoming "createFindCursor fails if filter is not specified" test.
A newly created ChangeStream will have a null key, so we can rely on iterateUntilDocumentOrError to initially rewind the ChangeStream.
$args['pipeline'], | ||
array_diff_key($args, ['pipeline' => 1]) | ||
); | ||
$changeStream->rewind(); |
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.
It's not clear to me why we were ever rewinding here. When a ChangeStream is first instantiated, its key is null
. This means that iteratorUntilDocumentOrError
is going to make an initial rewind()
call on its own before attempting to check for valid()
and either return the first result or loop with next()
until we hit a valid()
element.
Removing this didn't break any of the existing change stream tests, so I think it's safe to conclude that this was redundant.
$cachedIsSatisfiedArgs[0], | ||
$cachedIsSatisfiedArgs[1], | ||
$cachedIsSatisfiedArgs[3] ? 'yes' : 'no' | ||
)); |
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 didn't bother to add serverless here, which I think is fine because it's not really supported yet. We can consider doing so when the functionality is actually implemented.
That said, I don't see a nice way to print the server parameters in this message -- since there could be tons of them. One approach might be trying to log the environment details when we first compute/cache them, and then keeping the skip message short and avoiding redundant output. I'm not familiar with what, if any, logging mechanisms PHPUnit makes available.
private function isAuthenticated() : bool | ||
{ | ||
$database = $this->internalClient->selectDatabase('admin'); | ||
$connectionStatus = $database->command(['connectionStatus' => 1])->toArray()[0]; |
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.
Supported since MongoDB 2.4: https://docs.mongodb.com/v3.0/reference/command/connectionStatus/
I look forward to finding out this isn't implemented properly by the Atlas proxies 😄
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.
FWIW, libmongoc checks whether the connection string contains authentication info, or if the MONGOC_TEST_USER
env variable is set. If the proxy gives us trouble, I suggest we default to something like this as a fallback.
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'll add a comment to this method to note that we might want to consider that.
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.
LGTM, thanks for taking care of the new test runner features 🎉
Sync valid-pass and valid-fail tests with mongodb/specifications@ed4e62b Moves caching of server environment to checkRunOnRequirements Detection of load balancers will be addressed after driver support is implemented (PHPLIB-671)
Explicit assertions can avoid differing behavior for undefined array keys (notice/warning on PHP 7/8, respectively).
This is presently only used for test runner ops, but the spec does define it as a common option that should always be resolved (like "session").
Sync unified transaction tests with mongodb/specifications@ba8a3f7 Detection of serverless will be addressed after driver support is implemented (PHPC-1755)
This PR updates all unified spec tests to their latest version (across individual commits). This includes more recent changes to the unified transaction tests for DRIVERS-1539.
Detection of load balancer and serverless is intentional omitted (pending the respective PHP tickets to implement support for those features). The test runner currently assumes that neither is being used when resolving RunOnRequirements.