Make abstract test case for user

[soullivaneuh]

Lot of Sonata classes are abstract classes and the goal is to be extended on your projects.

I'm thinking about AbstractAdmin or BlockService.

The test case

The goal of a test case is to provide facilities to the user to test project classes extending Sonata classes.

A test case can:

• Init needed properties on setUp and setUpBeforeClass
• Add some advanced assert methods
• Have some test* method that will be launched on each children test class.

The name

I suggest to name those kind of class Abstract<TargetClass>TestCase.

• Abstract prefix because, well, it's an abstract class. :-)
• TestCase to differentiate internal abstract test class that should not be used by ending user and abstract class dedicated for this

BC compliance

By definition, test classes should not be affected by the BC rule.

*TestCase classes must do, because they will be used.

Tag all other test classes with the @internal label can do the trick, but will be painful to apply.

So I'm thinking about the @api tag.

The @api tag represents those Structural Elements with a public visibility which are intended to be the public API components for a library or framework. Other Structural Elements with a public visibility serve to support the internal structure and are not recommended to be used by the consumer.

The exact meaning of Structural Elements tagged with @api MAY differ per project. It is however RECOMMENDED that all tagged Structural Elements SHOULD NOT change after publication unless the new version is tagged as breaking Backwards Compatibility.

This is exactly the goal here.

With that, we can quickly see which test class can be broke and which should not.

A documentation about that should be added somewhere for ending users.

Sources

Class example from Symfony: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTestCase.php

A class from one of my projects: https://github.com/Soullivaneuh/IsoCodesValidator/blob/3.x/tests/Constraints/AbstractConstraintValidatorTest.php

This is not a test case class, but you can see the principle of "default test" like testNullIsValid. This avoid test code duplication.

What is your opinion about this?

[greg0ire]

I think there can be two sorts of abstract tests :

1. internal tests
2. abstract tests the user can extend to tests their classes

I think both should be separated, because the first should not be autoloadable

Also, I think tests of type 2 should be integration tests (meaning some things are not mocked), because I can't imagine an abstract unit test that would be useful for users, and because I also think we should try to avoid inheritance as much as we can (main reason I prefer data mapper to active record).

[soullivaneuh]

because I can't imagine an abstract unit test that would be useful for users

Why?

It could be useful to simplify testing. For example, instead of let the user do all the mock logic, we can make an abstract method like getExpectedResult that the user extending the test case will simply have to implement to have tests running.

I think both should be separated, because the first should not be autoloadable

What do you mean?

[greg0ire]

What do you mean?

If some tests are not meant to be run by the user, or used, they should not be autoloadable (they should be only in dev, thanks to autoload-dev)

[greg0ire]

It could be useful to simplify testing. For example, instead of let the user do all the mock logic, we can make an abstract method like getExpectedResult that the user extending the test case will simply have to implement to have tests running.

Do you have a concrete example ?

[soullivaneuh]

If some tests are not meant to be run by the user, or used, they should not be autoloadable (they should be only in dev, thanks to autoload-dev)

This is not currently the case: https://github.com/sonata-project/SonataAdminBundle/blob/daa65de93c11737481ab99475bb5b551234082ce/composer.json#L63-L65

This is weird because Symfony does this: https://github.com/symfony/symfony/blob/2.8/composer.json#L107-L109

And I'm pretty sur I'm using one of their test class for my project...

Do you have a concrete example ?

This test method: https://github.com/Soullivaneuh/IsoCodesValidator/blob/6ef053d7131ce701aba2f1e1f5fdb84aa5bec2c2/tests/Constraints/AbstractGenericConstraintValidatorTest.php#L10-L17

Use getValidValues data provider. So I defined an abstract method of this here: https://github.com/Soullivaneuh/IsoCodesValidator/blob/6ef053d7131ce701aba2f1e1f5fdb84aa5bec2c2/tests/Constraints/AbstractConstraintValidatorTest.php#L81

With that, you only have to supply the valid values on test classes, like this: https://github.com/Soullivaneuh/IsoCodesValidator/blob/6ef053d7131ce701aba2f1e1f5fdb84aa5bec2c2/tests/Constraints/BbanValidatorTest.php#L13-L19

At the end, nearly no test logic have to be written on child classes, except for special cases.

And this does not stop only at data providers: https://github.com/Soullivaneuh/IsoCodesValidator/blob/6ef053d7131ce701aba2f1e1f5fdb84aa5bec2c2/tests/Constraints/AbstractConstraintValidatorTest.php#L60

[greg0ire]

This is not currently the case: https://github.com/sonata-project/SonataAdminBundle/blob/daa65de93c11737481ab99475bb5b551234082ce/composer.json#L63-L65

This is a bundle. What about libraries? And what about the future, what if someday Symfony recommends to blacklist tests from the autoload in bundles ?

I must go, but I'll have a look at your examples.

[greg0ire]

Thanks for the example, it is valid indeed!

[greg0ire]

So to sum up my point of view :

In a library, tests useful for end-users would go insrc, so that they can be autoloadable, abstract tests not useful for end-users would go in test?

In a bundle… well I think it would be Tests vs Test, like symfony does : https://github.com/symfony/symfony/tree/master/src/Symfony/Bundle/FrameworkBundle

[soullivaneuh]

In a library, tests useful for end-users would go insrc, so that they can be autoloadable

So you mean src/Test/AbstractDummyTestCase.php for example?

In a bundle… well I think it would be Tests vs Test, like symfony does

Sound good to me. 👍

[greg0ire]

So you mean src/Test/AbstractDummyTestCase.php for example?

Yes

[greg0ire]

@sonata-project/contributors is everyone ok with that?

[soullivaneuh]

Yes for me. 👍

It would be great to add a note about this on the contributing guide.

[core23]

23 days have passed... Only 3 reactions :(

[soullivaneuh]

@core23 Testing is not the priority of lot of people... 😉

Let's write this on the contributing guide soon.

[greg0ire]

3 positive reactions though. I think we can merge sonata-project/SonataCoreBundle#275 , ok?

[soullivaneuh]

It would be great to have proper guideline about this before merging anything IMHO.

[sstok]

FYI, usually a TestCase is an abstract, so placing Abstract before it seems a bit redundant.

[greg0ire]

FYI, usually a TestCase is an abstract, so placing Abstract before it seems a bit redundant.

Plus, Symfony does it this way ™.

[soullivaneuh]

Symfony did it historically.

But each abstract class name must be prefixed by Abstract according to Symfony conventions.

[greg0ire]

But each abstract class name must be prefixed by Abstract according to Symfony conventions.

You mean they are not following their own conventions?

[soullivaneuh]

You mean they are not following their own conventions?

Historically.

Prefix abstract classes with Abstract. Please note some early Symfony classes do not follow this convention and have not been renamed for backward compatibility reasons. However all new abstract classes must follow this naming convention;

http://symfony.com/doc/current/contributing/code/standards.html#naming-conventions

[jordisala1991]

If the need comes and someone wants to implement it , it will be welcomed