Remove Validator\TypeTestCase and add validator logic to base TypeTestCase #21960

pierredup · 2017-03-10T12:18:19Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no/possibly
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	symfony/symfony-docs#7587

Based on a discussion in the docs, users should not really extend classes in the Tests namespace. This means that if you want to unit test forms, you need to extend the Test\TypeTestCase class which doesn't have the validation logic (Not adding the validator extension gives an error if you use the constraints option in your forms).
So I propose to remove the Validator\TypeTestCase class (or it can possibly be deprecated if it is wrongly used by someone), and add the validator extension logic to the base TypTestCase class.

The benefit is that there is only one class to extend (both for internal or userland tests), and it makes it easy to add more core extensions if necessary.

The one part that I don't like too much at the moment, is keeping the extension in the getExtensions method. This means that you extend the class and need to register custom extensions, that you would need to do return array_merge(parent::getExtensions(), ... (only in the case when you want core extensions enabled). So I don't know if we rather want to add a private method like getCoreExtensions?

nicolas-grekas · 2017-03-10T13:19:18Z

Tests are broken

pierredup · 2017-03-10T13:27:09Z

The FormTypeValidatorExtensionTest class depends on the validator property, which is now removed. I just need to figure out a way around this, will push an update later to fix it

HeahDude · 2017-03-10T13:29:36Z

src/Symfony/Component/Form/Test/TypeTestCase.php

@@ -34,6 +35,22 @@ protected function setUp()
        $this->builder = new FormBuilder(null, null, $this->dispatcher, $this->factory);
    }

+    protected function getExtensions()


I'm still against this, as said in #21780 we should not add the extension overhead on all base tests, so this should live in a specific type test case like ValidationTypeTestCase.

The issue with a separate class, is that you can't add more core extensions, as each one will need a separate class then, which makes extending the classes or using multiple core extensions difficult.

In terms of the overhead, I don't think that is really an issue. The overhead is so small per test that the impact would be negligible

I understand your concerns, let me try again to explain why I suggest another way to solve this.

The issue with a separate class, is that you can't add more core extensions, as each one will need a separate class then, which makes extending the classes or using multiple core extensions difficult.

It should not be that difficult to call the parent, is it? Or maybe an ExtendedTypeTestCase with all core extensions preloaded could be an idea?

In terms of the overhead, I don't think that is really an issue. The overhead is so small per test that the impact would be negligible.

The overhead is not only about performances even if it impacts every field of every form, which indeed could be negligible in tests, but about the context, all core type tests extend this class, that means we cannot test them with the minimum of code necessary to run them anymore, and IMO we miss then the purpose of unit testing. Testing the core extension should not imply to load the validation one, it should not be tied to it, to avoid side effects, that's what we need in unit testing.

Let's imagine we follow your idea, and now want to add the CsrfExtension because in most applications this extension is enabled too, almost all core tests would fail because the token field is not submitted unless defining the related option to false.

I agree that the validation extension has the singularity to deeply test form errors, but still moving it to another base type test makes much more sense to me, that's why I'm suggesting to do it instead.

Anyway thank you for your contributions, I'm not "against" solving your issue, I'm just worrying about how we should do it.

Thanks for this info, it makes much more sense now.

It should not be that difficult to call the parent, is it

The problem is not calling the parent. The problem is if you have a ValidatorExtension, DoctrineExtension, CsrfExtension etc, and your form uses options provided by all the extensions, but they all live in separate classes. So you will need to extend one class, while manually registering the other extensions for your forms not to break.

Or maybe an ExtendedTypeTestCase with all core extensions preloaded could be an idea?

That could also work.

I played a bit with a different approach, to use traits instead of classes.
The trait would expose a single method (E.G getValidatorExtension), then in the base TypeTestCase class we can check if the trait is added to the current class, and if it is we can register the extension just by calling the method.

The benefit in this is that you don't have to load all the extensions in the base test case, and if someone wants the validator extension in their test, they only have to add a use ValidatorExtensionTrait to their test class which then automatically registers the extension.
This can then be done for all core extensions, where you only need to add the traits you are interested in.

What are your thoughts on this approach? I have a working POC available that I can push to this PR if you want to have a look at the implementation.

It may be a good idea, of course you can push it so every one can review :).

Changes pushed. It's just a POC, so I just want to get feedback on the idea. If everyone is happy with the approach then I'll finish the implementation

pierredup · 2017-03-15T09:55:55Z

Tests back to green

HeahDude · 2017-03-15T13:20:52Z

src/Symfony/Component/Form/Test/Traits/ValidatorExtensionTrait.php

+{
+    protected $validator;
+
+    protected function getValidatorExtension()


The trait is now missing the tearDown (or maybe a tearDownAfterClass?) to set the validator property to null.

tearDownAfterClass added to the TypeTestCase class

Can't use tearDownAfterClass since it's static, so I added tearDown instead

nicolas-grekas · 2017-09-26T12:57:18Z

@pierredup looks like this one just needs fixing one small comment. Would you be up for fixing it+rebasing/retargeting to 3.4?

…tCase

…s testing

fabpot · 2017-09-27T16:12:54Z

Thank you @pierredup.

…to base TypeTestCase (pierredup) This PR was squashed before being merged into the 3.4 branch (closes #21960). Discussion ---------- Remove Validator\TypeTestCase and add validator logic to base TypeTestCase | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no/possibly | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | symfony/symfony-docs#7587 Based on a discussion in the docs, users should not really extend classes in the `Tests` namespace. This means that if you want to unit test forms, you need to extend the `Test\TypeTestCase` class which doesn't have the validation logic (Not adding the validator extension gives an error if you use the `constraints` option in your forms). So I propose to remove the `Validator\TypeTestCase` class (or it can possibly be deprecated if it is wrongly used by someone), and add the validator extension logic to the base `TypTestCase` class. The benefit is that there is only one class to extend (both for internal or userland tests), and it makes it easy to add more core extensions if necessary. The one part that I don't like too much at the moment, is keeping the extension in the `getExtensions` method. This means that you extend the class and need to register custom extensions, that you would need to do `return array_merge(parent::getExtensions(), ... ` (only in the case when you want core extensions enabled). So I don't know if we rather want to add a private method like `getCoreExtensions`? Commits ------- 5ab5010 Remove Validator\TypeTestCase and add validator logic to base TypeTestCase

carsonbot added Status: Needs Review Feature labels Mar 10, 2017

HeahDude suggested changes Mar 10, 2017

View reviewed changes

nicolas-grekas added this to the 3.x milestone Mar 14, 2017

HeahDude reviewed Mar 15, 2017

View reviewed changes

ogizanagi mentioned this pull request May 1, 2017

[Form] RepeatedType does not allow invalid_options #22593

Closed

pierredup changed the base branch from master to 3.4 September 26, 2017 19:17

pierredup added 8 commits September 26, 2017 21:19

Remove Validator\TypeTestCase and add validator logic to base TypeTes…

777107d

…tCase

Keep class references consistent

06b4b83

Add ValidatorExtensionTrait to register validation extension for form…

6180189

…s testing

Add ValidatorExtensionTrait to FormTypeValidatorExtensionTest

9374094

Add ValidatorExtensionTrait to SubmitTypeValidatorExtensionTest

f74a53c

Fixed CS

b887c3c

Add tearDownAfterClass method to clear validator property

45e518a

Change tearDownAfterClass to tearDown in TypeTestCase

c6ae491

pierredup force-pushed the forms-testing branch from cefde58 to c6ae491 Compare September 26, 2017 19:27

fabpot approved these changes Sep 27, 2017

View reviewed changes

fabpot closed this Sep 27, 2017

pierredup deleted the forms-testing branch September 27, 2017 16:28

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

kobelobster mentioned this pull request Dec 6, 2017

Potential BC Break by removing Symfony\Component\Form\Tests\Extension\Validator\Type\TypeTestCase after updating from Symfony 3.3 to 3.4 #25358

Closed

javiereguiluz mentioned this pull request May 28, 2018

[Form] Missing note about validation and form type tests symfony/symfony-docs#7566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Validator\TypeTestCase and add validator logic to base TypeTestCase #21960

Remove Validator\TypeTestCase and add validator logic to base TypeTestCase #21960

pierredup commented Mar 10, 2017

nicolas-grekas commented Mar 10, 2017

pierredup commented Mar 10, 2017

HeahDude Mar 10, 2017 •

edited

Loading

pierredup Mar 10, 2017

HeahDude Mar 11, 2017

pierredup Mar 11, 2017 •

edited

Loading

HeahDude Mar 11, 2017

pierredup Mar 11, 2017

nicolas-grekas Mar 14, 2017

pierredup commented Mar 15, 2017 •

edited

Loading

HeahDude Mar 15, 2017

pierredup Sep 26, 2017

pierredup Sep 26, 2017

nicolas-grekas commented Sep 26, 2017

fabpot commented Sep 27, 2017

Remove Validator\TypeTestCase and add validator logic to base TypeTestCase #21960

Remove Validator\TypeTestCase and add validator logic to base TypeTestCase #21960

Conversation

pierredup commented Mar 10, 2017

nicolas-grekas commented Mar 10, 2017

pierredup commented Mar 10, 2017

HeahDude Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierredup Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierredup commented Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 26, 2017

fabpot commented Sep 27, 2017

HeahDude Mar 10, 2017 •

edited

Loading

pierredup Mar 11, 2017 •

edited

Loading

pierredup commented Mar 15, 2017 •

edited

Loading