Hotfix #279 #280
Hotfix #279 #280
Conversation
test/ServiceManagerTest.php
Outdated
| 'class_map' => [ | ||
| 'Foo' => 'Foo', | ||
| ], | ||
| ] |
test/ServiceManagerTest.php
Outdated
| $lazyServicesProperty->setAccessible(true); | ||
|
|
||
| self::assertSame($config['delegators'], $delegatorsProperty->getValue($serviceManager)); | ||
| self::assertSame($config['lazy_services'], $lazyServicesProperty->getValue($serviceManager)); |
There was a problem hiding this comment.
I am not convinced to that test, as we are using reflection to check internal state of the object. Is there something what we can notice "outside" that it's wrong configured? I'm afraid that test like that is not future-proof.
There was a problem hiding this comment.
@thomasvargiu I agree with @webimpress here. What is the side effect that occurs currently? The test should be checking to ensure that side effect does not occur with your patch.
With delegators, that's generally pretty easy - you can check that a class is not double-decorated after retrieval. With lazy services, I'm assuming that the fact that you get an array value in a class map will likely throw an error on retrieval — if that's the case, test that you do not get that exception.
There was a problem hiding this comment.
@webimpress @weierophinney I changed it, configuration used in test was wrong.
But I also changed the implementation to merge delegators. Let me know.
|
Run |
|
@weierophinney fixed it |
|
@thomasvargiu It looks like a hotfix, so I think you should target master here. |
|
@webimpress I rewrite it on master base |
| $instance = $serviceManager->get('Foo'); | ||
|
|
||
| self::assertInstanceOf(InvokableObject::class, $instance); | ||
| self::assertSame(1, $instance->getOptions()['inc']); |
There was a problem hiding this comment.
I think a single test should have only one assertion.
There was a problem hiding this comment.
IMHO, I think it can be a valid assertion. Instead to throw an error in case of failure I'm asserting that the second assertion depends on the first assertion. If it's enough the phpunit error catch for you, I can remove the first assertion. Different point of view :)
Someonelse opinion?
There was a problem hiding this comment.
It's fine to have multiple assertions as long as they are connected, imho. Kinda "one logic assertion". I was checking what people say about it, and I see some approach to write test with single assertion. I see the advantages of it, but I think this approach would be more messy. You can see that in other tests usually we have more than one assertion, but these are connected - as you are saying, @thomasvargiu.
any my favourite comment there is:
A single assert per unit test is a great way to test the reader's ability to scroll up and down.
🤣
|
This repository has been closed and moved to laminas/laminas-servicemanager; a new issue has been opened at laminas/laminas-servicemanager#3. |
|
This repository has been moved to laminas/laminas-servicemanager. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:
|
Fix issue #279