Added note about using the Validator TypeTestCase in form unit testing #7587

pierredup · 2017-03-07T10:04:16Z

This update ads the usage of the validator TypeTestCase class to use for testing when validation is required.

HeahDude

Thanks!

xabbuh · 2017-03-07T20:49:49Z

Actually, after thinking about this again I am 👎 on merging the PR as is. We usually don't want to guarantee BC for classes that live inside the Tests namespace (that's why there is a Test namespace that contains classes that are meant to be used as base classes for userland code).

pierredup · 2017-03-07T21:02:25Z

What would you suggest needs to change? Should the class be moved to the Test namespace under a different name (Something like ValidatorTestCase)?

xabbuh · 2017-03-07T21:06:18Z

I am not completely settled on that yet. The main issue is: What if you need another extension? Which base class do you extended then (supposed there is a base class for the other extension as well)?

xabbuh · 2017-03-07T21:06:44Z

I think the best is to open an issue on the code repository and discuss the matter over there first.

wouterj · 2017-03-07T21:07:23Z

While I like your suggestion, I don't think we should document classes that live in *\Tests\*. These classes aren't part of the BC promise and thus shouldn't be used by other packages.

Instead, we should update our example to fix the code. This probably means finishing #6463 .

However, if it turns out to be a very nice addition to unit testing, the class might need to be moved to Symfony\Component\Form\Test\ValidatorTypeTestCase so it can be used by other packages. As I've seen this class used by quite a few packages, I suggest submitting a PR doing so against the code repository. Are you maybe willing to do that?

pierredup · 2017-03-07T21:08:24Z

Hence my original PR that added the validator to the main base class with an extension point to register custom extensions (and other core extensions can be added in the same place).
I'll open another PR following that approach again and see what discussions come out of it

wouterj · 2017-03-07T21:10:40Z

Sorry if I duplicated something that's already done. I lost overview with all these referenced PRs. 😇

pierredup · 2017-03-07T21:11:12Z

I would suggest updating the standard base class to include the validator extension, and remove (deprecate?) the one under Tests so that there is one class to extend, both for userland and internal tests (also checking that the validator is installed before registering the extension).
I'll open a PR on the code repository for this and wait for feedback

wouterj · 2017-03-07T21:13:08Z

I would suggest updating the standard base class to include the validator extension

I don't think this will be accepted by the core team. The form component can be used standalone without the validator, so it should at least be optional. You may create a trait with a getValidatorExtension() method that can be easily called when extending the TypeTestCase?

pierredup · 2017-03-07T21:15:28Z

also checking that the validator is installed before registering the extension

This should cover the case where the form component is used in standalone mode and the validator is not installed.

xabbuh · 2017-06-05T06:22:26Z

form/unit_testing.rst

@@ -157,40 +157,49 @@ before creating the parent form using the ``PreloadedExtension`` class::
    be getting errors that are not related to the form you are currently
    testing but to its children.

+Forms Using Validation


@pierredup Can you remove this section? We can then still merge your fixes to the class namespaces which is still a legit fix.

pierredup · 2017-06-24T13:54:25Z

form/unit_testing.rst

            return array(
-                new ValidatorExtension($validator),


Should these changes also be reverted?

…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

javiereguiluz · 2018-01-10T08:23:02Z

What can we do to make this PR mergeable? Thanks!

HeahDude · 2018-01-10T08:45:29Z

@javiereguiluz Revert everything excepted namespaces modifications like https://github.com/symfony/symfony-docs/pull/7587/files#diff-d9a34263773b4fe7719e18118a10e628R204.

javiereguiluz · 2018-01-10T09:13:21Z

@pierredup we had to revert part of your proposed changes, but merged others. Thanks for your contribution!

… unit testing (pierredup, javiereguiluz) This PR was merged into the 2.7 branch. Discussion ---------- Added note about using the Validator TypeTestCase in form unit testing This update ads the usage of the validator TypeTestCase class to use for testing when validation is required. Commits ------- c495b92 Readded a blank line ff1bd8e Reverted some changes f0887ae Remove section about forms using validation 6fda2df Removed extra class cf0f9e6 Update fomr testing sentence 070b2c0 Renamed TestedTypeTests to TestedTypeTest d17821a Fix heading underlining d4979eb Fix typos 3e6dc19 Add docs about the validator TypeTestCase class

carsonbot added the Status: Needs Review label Mar 7, 2017

pierredup mentioned this pull request Mar 7, 2017

Add docs to register custom form extensions and types for unit testing #7578

Closed

HeahDude approved these changes Mar 7, 2017

View reviewed changes

HeahDude added Form Status: Reviewed and removed Status: Needs Review labels Mar 7, 2017

pierredup mentioned this pull request Mar 10, 2017

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

Closed

HeahDude mentioned this pull request Mar 28, 2017

Update unit_testing.rst #7725

Closed

xabbuh reviewed Jun 5, 2017

View reviewed changes

pierredup added 6 commits June 24, 2017 15:49

Add docs about the validator TypeTestCase class

3e6dc19

Fix typos

d4979eb

Fix heading underlining

d17821a

Renamed TestedTypeTests to TestedTypeTest

070b2c0

Update fomr testing sentence

cf0f9e6

Removed extra class

6fda2df

pierredup force-pushed the forms-unit-testing branch from 66a009b to 6fda2df Compare June 24, 2017 13:50

Remove section about forms using validation

f0887ae

pierredup commented Jun 24, 2017

View reviewed changes

HeahDude added Status: Needs Work and removed Status: Reviewed labels Jul 29, 2017

HeahDude added this to the 2.7 milestone Jul 29, 2017

javiereguiluz added the Waiting feedback label Jan 10, 2018

Reverted some changes

ff1bd8e

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 10, 2018

Readded a blank line

c495b92

javiereguiluz added Status: Reviewed and removed Status: Needs Review Waiting feedback labels Jan 10, 2018

javiereguiluz merged commit c495b92 into symfony:2.7 Jan 10, 2018

pierredup deleted the forms-unit-testing branch January 10, 2018 09:14

Uh oh!

Added note about using the Validator TypeTestCase in form unit testing #7587

Added note about using the Validator TypeTestCase in form unit testing #7587

Uh oh!

Conversation

pierredup commented Mar 7, 2017

Uh oh!

HeahDude left a comment

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Mar 7, 2017

Uh oh!

pierredup commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xabbuh commented Mar 7, 2017

Uh oh!

xabbuh commented Mar 7, 2017

Uh oh!

wouterj commented Mar 7, 2017

Uh oh!

pierredup commented Mar 7, 2017

Uh oh!

wouterj commented Mar 7, 2017

Uh oh!

pierredup commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wouterj commented Mar 7, 2017

Uh oh!

pierredup commented Mar 7, 2017

Uh oh!

xabbuh Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

pierredup Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

xabbuh Jun 29, 2017

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Jan 10, 2018

Uh oh!

HeahDude commented Jan 10, 2018

Uh oh!

javiereguiluz commented Jan 10, 2018

Uh oh!

Uh oh!

pierredup commented Mar 7, 2017 •

edited

Loading

pierredup commented Mar 7, 2017 •

edited

Loading