Relax some mocks #22394
Relax some mocks #22394
Conversation
9bfa0a6 to
c58c42b
Compare
| @@ -33,7 +33,6 @@ public function testThatCacheWarmersAreProcessedInPriorityOrder() | ||
| ->will($this->returnValue($services)); | ||
| $container->expects($this->atLeastOnce()) | ||
| ->method('getDefinition') | ||
| ->with('cache_warmer') |
There was a problem hiding this comment.
this removal does not look right to me. We don't want to return the same Definition mock for unrelated services
There was a problem hiding this comment.
does this losen the assertion of the test to you?
imho it's a "we shouldn't care" internal detail - dealing with it is not related to the business of this test case (same for other changes)
There was a problem hiding this comment.
Not sure what's your goal here Nicolas, but I'd rather replace ->expects($this->atLeastOnce()) with ->expects($this->any()) (use a stub instead of a mock). Removing the with() call seems wrong, as the test really should expect this particular service definition to be fetched from the container (as opposed to any definiton).
| @@ -31,12 +32,16 @@ public function testServicesAreOrderedAccordingToPriority() | ||
| new Reference('n3'), | ||
| ); | ||
|
|
||
| $container = $this->getMockBuilder('Symfony\Component\DependencyInjection\ContainerBuilder')->setMethods(array('findTaggedServiceIds'))->getMock(); | ||
| $container = $this->getMockBuilder('Symfony\Component\DependencyInjection\ContainerBuilder')->setMethods(array('findTaggedServiceIds', 'getDefinition'))->getMock(); |
There was a problem hiding this comment.
Do we really need to mock the ContainerBuilder class in the first place? IMO, most of the times we can simply use a "real" ContainerBuilder object when writing tests for compiler passes.
There was a problem hiding this comment.
We don't, these tests use reflection to check that the private findAndSortTaggedServices is called, they could just register some services with the good tag in a concrete builder and check the services arguments/method calls after processing, which is the right thing to assert IMO as these passes just set arguments/calls on the collected services.
c58c42b to
1dd7fe8
Compare
1dd7fe8 to
61be733
Compare
|
PR ready |
|
Thank you @nicolas-grekas. |
This PR was merged into the 3.2 branch. Discussion ---------- Relax some mocks | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - We should not use mocks when it's not required - they too often run assertions on internal behavior. These changes will unlock #22388 Commits ------- 61be733 Relax some mocks
We should not use mocks when it's not required - they too often run assertions on internal behavior.
These changes will unlock #22388