PHPLIB-481: Add the ability to specify a pipeline to an update within bulk write #684
Conversation
alcaeus
force-pushed
the
fix-bulkwrite-pipeline
branch
from
1a26ae2 to
97249da
Compare
There was a problem hiding this comment.
I've noted that this depends on mongodb/specifications#651 being merged.
| @@ -263,7 +263,7 @@ public function provideOpsWithMissingArguments() | |||
| public function testUpdateOneRequiresUpdateOperators() | |||
| { | |||
| $this->expectException(InvalidArgumentException::class); | |||
| $this->expectExceptionMessage('First key in $operations[0]["updateOne"][1] is not an update operator'); | |||
| $this->expectExceptionMessage('First key in $operations[0]["updateOne"][1] is neither an update operator nor a pipeline'); | |||
There was a problem hiding this comment.
The test title should probably be changed to "RequiresUpdateOperatorsOrPipeline".
That said, are these test redundant in light of the other two tests we have in BulkWriteTest.php (later in this PR)? They look very similar, except the ones in the other file use constants for the operation name.
If so, I think the correct place for these is the non-functional BulkWriteTest.php class, in which case you can delete the two methods in BulkWriteFunctionalTest.php. May want to check for other duplicated tests while you're in here.
There was a problem hiding this comment.
Good catch. I've removed these and a few other tests that were duplicated and didn't run the bulk writes.
| @@ -272,7 +272,7 @@ public function testUpdateOneRequiresUpdateOperators() | |||
| public function testUpdateManyRequiresUpdateOperators() | |||
| { | |||
| $this->expectException(InvalidArgumentException::class); | |||
| $this->expectExceptionMessage('First key in $operations[0]["updateMany"][1] is not an update operator'); | |||
| $this->expectExceptionMessage('First key in $operations[0]["updateMany"][1] is neither an update operator nor a pipeline'); | |||
There was a problem hiding this comment.
Likewise with this test title, although this may be irrelevant given the rest of my previous comment about test duplication.
alcaeus
force-pushed
the
fix-bulkwrite-pipeline
branch
from
97249da to
fe3278a
Compare
|
Note: mongodb/specifications#651 has been merged. |
alcaeus
force-pushed
the
fix-bulkwrite-pipeline
branch
from
fe3278a to
383f90a
Compare
There was a problem hiding this comment.
I assume the JSON tests are already in sync with whatever was merged in mongodb/specifications#651. I'm not sure if they actually had to change in this PR or not.
Changes LGTM!
I used this PR to validate the new tests and ensure that everything was working as expected 👍 |
alcaeus
force-pushed
the
fix-bulkwrite-pipeline
branch
from
383f90a to
d44bd16
Compare
https://jira.mongodb.org/browse/PHPLIB-481