OptionsResolver test coverage #16446
Conversation
|
|
I introduced the changes you wanted. I have very mixed feelings about the principle number 2. I know that it's good for most cases but sometimes some parts of code are harder to test. I don't see anything wrong with using reflections in such cases. Can this be discussed? Thanks for review. |
|
I agree with @Tobion, private methods encapsulate logic as black boxes. We need to test them via the regular public API developers are going to use. |
|
@eventhorizonpl I don't see the |
|
true case is covered by testResolveFailsIfValueAllowedCallbackReturnsFalse according to a report.
|
|
@eventhorizonpl |
| $this->resolver->setDefined('option'); | ||
| $this->resolver->setAllowedTypes('option', 'string'); | ||
|
|
||
| $this->resolver->resolve(array('option' => [])); |
There was a problem hiding this comment.
please keep the code compatible with PHP 5.3 (this is why Travis is failing)
|
@eventhorizonpl can you please rewrite the tests to use a dataProvider. This provider (actual type, allowed type, exception message) would then also cover all the other types (from |
|
I added dataProvider for my tests and testResolveFailsIfInvalidType, testResolveFailsIfInvalidTypeIsNull, testResolveFailsIfNotInstanceOfClass. I didn't changed testResolveSucceedsIfValidType* because my current goal is not to fix all tests. Right now i 8000 t's just about covering code. |
| * @dataProvider getFormatValueData | ||
| * @expectedException \Symfony\Component\OptionsResolver\Exception\InvalidOptionsException | ||
| */ | ||
| public function testFormatValue($actualType, $allowedType, $exceptionMessage) |
There was a problem hiding this comment.
please name this test testResolveFailsIfInvalidType and at the same lines of the old tests (line 505). this makes merging easier.
| @@ -499,27 +499,29 @@ public function testFailIfSetAllowedTypesFromLazyOption() | |||
| } | |||
|
|
|||
| /** | |||
| * @dataProvider provideInvalidTypes | |||
| * @expectedException \Symfony\Component\OptionsResolver\Exception\InvalidOptionsException | |||
|
Thank you @eventhorizonpl. |
This PR was submitted for the 2.8 branch but it was merged into the 2.7 branch instead (closes #16446). Discussion ---------- OptionsResolver test coverage Hi, This PR adds 100% test code coverage to OptionsResolver component. Best regards, Michal | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Commits ------- 185950c OptionsResolver test coverage
Hi,
This PR adds 100% test code coverage to OptionsResolver component.
Best regards,
Michal