Skip to content
This repository was archived by the owner on Feb 28, 2025. It is now read-only.

Conversation

OskarStark
Copy link

@OskarStark OskarStark commented Oct 20, 2023

I am using it this way in my pipeline and when I use the assertSamePipeline method from this repository (copy/pasted), I receive the following error:
CleanShot 2023-10-20 at 21 56 07

With this test in your CI I want to achieve, that it is not a problem of the assertSamePipeline method.

cc @alcaeus @GromNaN

@OskarStark
Copy link
Author

OskarStark commented Oct 21, 2023

I rewrote all my pipelines, and my integration tests are all passing, except the ones, where I compare the pipeline itself. That means, the pipeline is correct, the fetched data is correct, but the assertion method seems not to be able to handle $unwind and my complex $group stage

@OskarStark OskarStark changed the title Add test for $unwind Add test for $unwind and a complex group stage Oct 21, 2023
@OskarStark OskarStark changed the title Add test for $unwind and a complex group stage Add test for $unwind and a complex $group stage Oct 21, 2023
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Thanks for your feedbacks.
MongoDB and the PHP driver are fairly permissive about the types they accept. We need to choose the most reliable for the encoder to cover all use cases.

);

$expected = [
['$unwind' => '$foo'],
Copy link
Member

Choose a reason for hiding this comment

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

This is the short syntax of $unwind. In order to cover all use cases while keeping the code as simple as possible, I've chosen not to detect when only the field path is provided. So it's always an object with at least one property "path" that's returned.

Suggested change
['$unwind' => '$foo'],
['$unwind' => ['path' => '$foo']],

Copy link
Author

Choose a reason for hiding this comment

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

Ok it would be helpful if the diff would show this when failing

Copy link
Member

Choose a reason for hiding this comment

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

Using JSON comparison, the error of looks like this:

1) MongoDB\Tests\Builder\BuilderEncoderTest::testUnwind
'{ "root" : [ { "$unwind" : { "path" : "$foo" } }, { "$unwind" : { "path" : "$foo.bar" } }, { "$unwind" : { "path" : "$foo.bar.baz" } } ] }'
Failed asserting that '{ "root" : [ { "$unwind" : { "path" : "$foo" } }, { "$unwind" : { "path" : "$foo.bar" } }, { "$unwind" : { "path" : "$foo.bar.baz" } } ] }' matches JSON string "{ "root" : [ { "$unwind" : "$foo" }, { "$unwind" : "$foo.bar" }, { "$unwind" : "$foo.bar.baz" } ] }".
--- Expected
+++ Actual
@@ @@
 {
     "root": [
         {
-            "$unwind": "$foo"
+            "$unwind": {
+                "path": "$foo"
+            }
         },
         {
-            "$unwind": "$foo.bar"
+            "$unwind": {
+                "path": "$foo.bar"
+            }
         },
         {
-            "$unwind": "$foo.bar.baz"
+            "$unwind": {
+                "path": "$foo.bar.baz"
+            }
         }
     ]
 }

Comment on lines +305 to +308
Expression::arrayToObject([[
'k' => Expression::toString(Expression::fieldPath('key')),
'v' => Expression::fieldPath('value'),
]]),
Copy link
Member

Choose a reason for hiding this comment

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

It's strictly equivalent. But in order to match the expected pipeline, it have to be an object. Maybe assertSamePipeline need to be improved to accept array or object equally.

Suggested change
Expression::arrayToObject([[
'k' => Expression::toString(Expression::fieldPath('key')),
'v' => Expression::fieldPath('value'),
]]),
Expression::arrayToObject([
object(
k: Expression::toString(Expression::fieldPath('key')),
v: Expression::fieldPath('value'),
),
]),

Copy link
Author

Choose a reason for hiding this comment

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

Yes it should not be necessary to change the pipeline itself to make the assert method happy 👌🏻

Copy link
Member

Choose a reason for hiding this comment

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

This is solved by using a BSON round-trip. See #13

@OskarStark
Copy link
Author

I changed my pipelines accordingly and my tests pass 👍 Thanks for the help

@GromNaN
Copy link
Member

GromNaN commented Oct 30, 2023

I'm closing your PR for now. The requested changes are tracked in PHPLIB-1291.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants