Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

How to achieve test isolation with Symfony forms and data transformers?

Tags:

Note: This is Symfony < 2.6 but I believe the same overall issue applies regardless of version

To start, consider this form type that is designed to represent one-or-more entities as a hidden field (namespace stuff omitted for brevity)

class HiddenEntityType extends AbstractType
{
    /**
     * @var EntityManager
     */
    protected $em;

    public function __construct(EntityManager $em)
    {
        $this->em = $em;
    }

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        if ($options['multiple']) {
            $builder->addViewTransformer(
                new EntitiesToPrimaryKeysTransformer(
                    $this->em->getRepository($options['class']),
                    $options['get_pk_callback'],
                    $options['identifier']
                )
            );
        } else {
            $builder->addViewTransformer(
                new EntityToPrimaryKeyTransformer(
                    $this->em->getRepository($options['class']),
                    $options['get_pk_callback']
                )
            );
        }
    }

    /**
     * See class docblock for description of options
     *
     * {@inheritdoc}
     */
    public function setDefaultOptions(OptionsResolverInterface $resolver)
    {
        $resolver->setDefaults(array(
            'get_pk_callback' => function($entity) {
                return $entity->getId();
            },
            'multiple' => false,
            'identifier' => 'id',
            'data_class' => null,
        ));

        $resolver->setRequired(array('class'));
    }

    public function getName()
    {
        return 'hidden_entity';
    }

    /**
     * {@inheritdoc}
     */
    public function getParent()
    {
        return 'hidden';
    }
}

This works, it's straightforward, and for the most part looks like like all the examples you see for adding data transformers to a form type. Until you get to unit testing. See the problem? The transformers can't be mocked. "But wait!" you say, "Unit tests for Symfony forms are integration tests, they're supposed to make sure the transformers don't fail. Even says so in the documentation!"

This test checks that none of your data transformers used by the form failed. The isSynchronized() method is only set to false if a data transformer throws an exception

Ok, so then you live with the fact you can't isolate the transformers. No big deal?

Now consider what happens when unit testing a form that has a field of this type (assume that HiddenEntityType has been defined & tagged in the service container)

class SomeOtherFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('field', 'hidden_entity', array(
                'class' => 'AppBundle:EntityName',
                'multiple' => true,
            ));
    }

    /* ... */
}

Now enters the problem. The unit test for SomeOtherFormType now needs to implement getExtensions() in order for the hidden_entity type to function. So how does that look?

protected function getExtensions()
{
    $mockEntityManager = $this
        ->getMockBuilder('Doctrine\ORM\EntityManager')
        ->disableOriginalConstructor()
        ->getMock();

    /* Expectations go here */

    return array(
        new PreloadedExtension(
            array('hidden_entity' => new HiddenEntityType($mockEntityManager)),
            array()
        )
    );
}

See where that comment is in the middle? Yeah, so for this to work correctly, all of the mocks and expectations that are in the unit test class for the HiddenEntityType now effectively need to be duplicated here. I'm not OK with this, so what are my options?

  1. Inject the transformer as one of the options

    This would be very straightforward and would make mocking simpler, but ultimately just kicks the can down the road. Because in this scenario, new EntityToPrimaryKeyTransformer() would just move from one form type class to another. Not to mention that I feel form types should hide their internal complexity from the rest of the system. This option means pushing that complexity outside the boundary of the form type.

  2. Inject a transformer factory of sorts into the form type

    This is a more typical approach to removing "newables" from within a method, but I can't shake the feeling that this is being done just to make the code testable, and is not actually making the code better. But if that was done, it would look something like this

    class HiddenEntityType extends AbstractType
    {
        /**
         * @var DataTransformerFactory 
         */
        protected $transformerFactory;
    
        public function __construct(DataTransformerFactory $transformerFactory)
        {
            $this->transformerFactory = $transformerFactory;
        }
    
        public function buildForm(FormBuilderInterface $builder, array $options)
        {
            $builder->addViewTransformer(
                $this->transformerFactory->createTransfomerForType($this, $options);
            );
        }
    
        /* Rest of type unchanged */
    }
    

    This feels ok until I consider what the factory will actually look like. It will need the entity manager injected, for starters. But what then? If I look further down the road, this supposedly-generic factory could need all sorts of dependencies for creating data transformers of different kinds. That is clearly not a good long-term design decision. So then what? Re-label this as an EntityManagerAwareDataTransformerFactory? It's starting to feel messy in here.

  3. Stuff I'm not thinking of...

Thoughts? Experiences? Solid advice?

like image 247
Peter Bailey Avatar asked Oct 11 '16 16:10

Peter Bailey


1 Answers

First of all, I have next to no experience with Symfony. However, I think you missed a third option there. In Working Effectively with Legacy Code, Michael Feathers outlines a way to isolate dependencies by using inheritance (he calls it "Extract and Override").

It goes like this:

class HiddenEntityType extends AbstractType
{
    /* stuff */

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        if ($options['multiple']) {
            $builder->addViewTransformer(
                $this->createEntitiesToPrimaryKeysTransformer($options)
            );
        }
    }

    protected function createEntitiesToPrimaryKeysTransformer(array $options)
    {
        return new EntitiesToPrimaryKeysTransformer(
            $this->em->getRepository($options['class']),
            $options['get_pk_callback'],
            $options['identifier']
        );
    }
}

Now to test, you create a new class, FakeHiddenEntityType, that extends HiddenEntityType.

class FakeHiddenEntityType extends HiddenEntityType {

    protected function createEntitiesToPrimaryKeysTransformer(array $options) {
        return $this->mock;
    }    

}

Where $this->mock obviously is whatever you need it to be.

The two most prominent advantages are that there are no factories involved, thus complexity is still encapsulated, and there is virtually no chance that this change breaks existing code.

The disadvantage being that this technique requires an extra class. More importantly, it requires a class that knows about the internals of the class under test.


To avoid the extra class, or rather hide the extra class, one could encapsulate it in a function, creating an anonymous class instead (support for anonymous classes was added in PHP 7).

class HiddenEntityTypeTest extends TestCase
{

    private function createHiddenEntityType()
    {
        $mock = ...;  // Or pass as an argument

        return new class extends HiddenEntityType {

            protected function createEntitiesToPrimaryKeysTransformer(array $options)
            {
                return $mock;
            }    

        }
    }

    public function testABC()
    {
        $type = $this->createHiddenEntityType();
        /* ... */
    }

}
like image 101
Pieter van den Ham Avatar answered Oct 06 '22 21:10

Pieter van den Ham