feat: use IteratorAggregate interface instead of final class

[soyuka]

Q	A
Type	improvement
BC Break	no

Hi, I have unit tests which are mocking the Builder and as Aggregation is a final class (even instantiating it in a test is hard, and even if I do so the getIterator method uses an assert($cursor instanceof Cursor) where Cursor is a final class that we can not instantiate. Using an interface here seems more appropriate, thoughts?

[IonBazan]

Why are you mocking the Builder in your tests? Aggregation does not take that many arguments - you can grab them from the Builder itself.

I think we are missing some context here - could you provide some code examples and share more information about your use case?

[malarzm]

I have unit tests which are mocking the Builder

This usually is a bad idea and make tests very fragile. You probably should be writing integration tests that do run against the database, or at least checks whether the query generated by your code makes sense/contains specific stuff.

[soyuka]

I agree with you but this means having a database running. We're unit testing an API Platform's abstraction that fetches a collection. No assertion is done on the queries, we just want to make sure that options are propagated to the execution context. For example: https://github.com/api-platform/core/blob/2.6/tests/Bridge/Doctrine/MongoDbOdm/CollectionDataProviderTest.php#L86

In that case, we want to assert that our doctrine_mongodb options are propagated to the execution context of the builder. I think that it makes sense to be able to unit test this kind of things. I must agree that mocking the whole Builder is a bad idea in general but there are still valid use cases.

[malarzm]

Given how the test is mocking everything from the registry down to the builder it should be OK to change it in this way:

1. Don't mock the builder using Prophecy, write your own implementation of a spy

• your spy can extend our Builder, it'll be easier that way and Builder itself is not final
• log what is being passed as an argument to methods you want to inspect (or the fact that methods are called), call parent's method when you want to
• to avoid using or even mocking Aggregation throw an exception instead of calling parent method

2. have your mocked repository return the spy instead of mocked aggregation builder
3. assert on your own that spy was interacted with in an expected way

Or on a second thought you probably should be able to tell Prophecy to throw an exception instead of returning the iterator from the execute method. You can catch the exception in the test (or even set it as expected to be thrown to know the method was indeed called as there's a branch for execute/getAggregation in the code) and still have other assertions tested.

It'll be a bit more work but I don't think that not being able to mock stuff is a valid reason to change a library. Also that'll allow you to work without waiting for a release :)

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.