Refactor the bundle to be more stable and faster

[Aerendir]

Hi @alexislefebvre and @lsmith77 , how are you?

Excuse me for my long absence, but I were in the process of closing the first version of my app and I had to concentrate on it.

But now that it is online, I'd like to start again contributing here.

We leave ourselves speaking about the fastness of this bundle (#218).

Then the the discourse was left to itself.

I'm opening this issue to try to make a recap of what we said and to draft a first (small) plan (so I can afford it given the time I have to dedicate to LiipFunctionTestBundle).

So, here are the things I think are priorities to create a better and faster bundle:

• Fix Move all files to src and tests #323: Move library files to src and tests files to tests;
• Ordering methods following this order: public, protected and private (this will make for developer easier to understand the API (s)he can use;
• Split the big WebTestCase.php in smaller files: AbstractTestCase from which inherit WebTestCase and CommandTestCase. This will make us able to move to dedicated files methods that are relevant only for one kind of test (for example, getVerbosityLevel() that is relevant only for commands or see also Getting exit code from runCommand() #329 )
• Fix Use private properties to store sensible and big variables to improve memory usage (Enhancement) #218: Start using properties (also not private to not BC) to store objects created by the bundle: this makes us able to do the next task...
• Start using setUp() and tearDown() methods to unset properties to free up memory during execution and make tests faster
• Start using getters and setter, deprecating the usage of direct access to properties (that should be private: extending classes should use getters and setters)
• Improve fetchContent() and fetchCrawler(): they MUST set (or use the already created - for this reason is useful using getters and setters: we can check if a client already exists and avoid creating a new one) the $client property, and not creating each time the client. There are use cases (mostly after a form is submitted) where it is required to reuse the same exact client created to load some subsequent pages: in those cases it is impossible to use the same $client and the only option is to create it directly in the test method using makeClient(), forcing the developer to write more code.
• Maybe also using PHPCSFixer is a good thing...

All this will be a good preparation also for the #6 , making easier the transition process.

This is a first list: what do you think about it?

[alexislefebvre]

Thanks for taking the time to merge all these ideas.

Splitting codes in several bricks would be nice.

I recently started at the upper level: currently this bundle is a collection of tools for tests that perform different taks. Can we refactor this bundle in several specialized bundles?

• loading fixtures and check number of queries
• assertions: fetchContent(), assertStatusCode(),etc. and user authentication
• command tests ?
• etc.

On the other hand, having only one bundle with these ready-to-use tools is also a nice point.

---

Anyway, I agree with most of your suggestions but it looks like they are all breaking changes, so we may start by adding a 2.0 branch and prioritize these tasks in order to build some solid bases for the future. I don't know how to do this though. :)

[Aerendir]

As this is a really big transition, we should make it as painless as possible.

So my suggested roadmap is something like this (all the subsequent points aren't BC):

STEP 1: Prepare the code for the 1.9.0 version

0. Reorder all methods in WebTestCase to make simpler to inspect and evaluate them (public, then protected and then private)
1. Create AbstractTestCase
2. Make WebTestCase extend AbstractTestCase
3. Find in WebTestCase all methods that are common to both CommandTestCase and WebTestCase and mark them with a @todo Move to AbstractTestCase
4. Move to AbstractTestCase the methods in WebTestCase marked with @todo Move to AbstractTestCase

STEP 2: Prepare code for CommandTestCase

1. Create CommandTestCase and make it extend AbstractTestCase (where at this point there are all methods it requires to work as those are inherited from AbstractTestCase)
2. COPY (NOT move) from WebTestCase to CommandTestCase all methods that are specifically used to test commands (I think, now, at WebTestCase::isDecorated() and setVerbosityLevel().
   If they make use of protected properties currently in WebTestCase, take this into account.
   So, for example, the procedure to move WebTestCase::isDecorated() should be this:
   1. Create the private property CommandTestCase::decorated (as a private property! No more protected properties, please!)
   2. Create the protected method CommandTestCase::isDecorated()
   3. Mark the property WebTestCase::$decorated as @deprecated Since 1.9.0 will be removed in 2.0. Use CommandTestCase::isDecorated() method instead
   4. Mark the method WebTestCase::isDecorated() as @deprecated Since 1.9.0 will be removed in 2.0. Use CommandTestCase::isDecorated() method instead
   5. Include in WebTestCase::isDecorated() an error with @trigger_error('isDecorated() is deprecated since 1.9.0 and will be removed in 2.0. Use CommandTestCase::isDecorated() instead')

STEP 3: Deprecate other protected properties of WebTestCase

At this point the other protected properties can be deprecated following this procedure:

1. Add to all protected properties `@deprecated Since 1.9.0 will be removed in 2.0. Use "TheTestCase::theMethod()" method instead
2. Create the TheTestCase::theMethod() method (in one of AbstractTestCase, WebTestCase or CommandTestCase (it depends and should be discussed for each property).

STEP 4: Create the version 1.9.0 and then decide how to proceed (toward a 1.10.0 version or directly toward the 2.0.0 version

The next steps should be to implement tearDown() method and improve other methods (like fetchCrawler() and fetchContent(). But this can be discussed after the 4 steps outlined above will have been discussed.

What do you think of this roadmap?

[Jean85]

Checking in to say that I would like to help here! I use this bundle in a few cases, so I would like to give back!

[Aerendir]

Hi @Jean85 , glad to get your help! :)

In the mean time, do you have any feedback?

[Aerendir]

@alexislefebvre Reading again your comments, I saw that you spoke about the splitting in several bundles.

Currently I don't have a precise idea about this, but I think that the first step is to start splitting the big WebTestCase file.

Then, another thing to start considering is to create more services (as also suggested by @lsmith77 in #6 ).

Once the code is split, then we will also have a better view of which can become a dedicated bundle and which, instead, should remain in this one.

[Jean85]

I like the roadmap very much, the only concern that I have is the fetchCrawler() and fetchContent() stuff, it's not very clear to me. You want to hide the client inside the testcase to avoid multiple instantiation? What if I want to reproduce a scenario where two different users interact?

[Aerendir]

Yes and no... The goal is not to hide the client, just the opposite.

I want to be able to directly access the client, the same client, using both fetchCrawler and fetchContent.

Regarding you question, I really never thought at the possibility of creating two clients for two different users as I never needed it.
But for sure it will be taken into account when it will be time to refact the code deeper.

For the moment we should understand what we want to move where to split the code and reorganize it in a more efficient and logical way without break the old code.

[lsmith77]

I am not a huge fan of traits but I wonder if they are not a perfect fit here to be able to split out the different features without requiring limitations of inheritance and to reduce complexity with composition.

[Aerendir]

@lsmith77 , why not use services instead? I like traits, but not for complex features like the ones offered by this bundle.

Yes, using services maybe the complexity increases, but I think the flexibility will benefits more than using traits.

Anyway, is there somewhere a plan or a roadmap about what will be extrapolated to be a separate service/trait?

Just to take it into account planning the refactoring...

[lsmith77]

I am not sure if using DI in the test setup is the way to go anymore. I mean it makes sense to give access to services in the app, but I am not sure if providing functionality via services is the way to go: 1) reduce complexity 2) improve performance

[Aerendir]

Mmm... which factor should we use to decide the best approach? How can we try to figure before the benefits and drawbacks of each design choice?

Anyway we need to take a decision... on which factors and considerations we will base such decision?

[lsmith77]

honestly who ever does the work, or at leasy gets the ball rolling, gets to decide :)

maybe I should clartify what I mean with "complexity"
if we add lots of test specific services to the DI, we are effectively risking heisenbugs.
another option could be of course to have our own DI container just for test services but this I suspect is overkill for our needs.

[Aerendir]

About your last suggestion, I'm using in one of my projects a custom DI container that is nothing more than a plain PHP ibject with getters and setters and I use it to some commonly used services around without the needs to move the entire Symfony DI container.

And I've also implemented it as a trait, so I can use it wherever I like.

Anyway, whichever is the solution we will adopt, I think we should first have a clear picture of what we want to extrapolate (as a service or as a trait is a secondary decision).

@alexislefebvre mentioned fixtures loading and some other functionalities: is there a list of these functionalities that may be rewrote as standalone classes (traits or services).

[alexislefebvre]

@Aerendir I'm reading all the conversation and there's one part that bothered me:

COPY (NOT move) from WebTestCase to CommandTestCase all methods that are specifically used to test commands

Copying code is an antipattern, we need a strong argument to accept duplicated code.

@lsmith77 You suggested to use traits, were you thinking about moving existing code in traits dedicated to different features (fixtures, assertions, etc.) and importing them in WebTestCase?

It looks like it would be a good way to split the huge class WebTestCase while providing BC. And in the same time it would allow users to import the traits instead of extending WebTestCase.

That would ease transition from the huge WebTestCase to smaller and specific traits.

[lsmith77]

@alexislefebvre yeah .. my thought was to group functionality into various traits.

[Aerendir]

@alexislefebvre , copying and not moving because of BC: if we immediately remove the code from webtestcase, for sure current tests will break. So in the transition we should deprecate the methods in webtestcase, Copy the code to commandtestcase and then, in a successive versione definitively remove it from webtestcase.

About traits, I still not have a strong opinion about it... I deserve more thoughts about.

Anyway we need a list of possible traits/services we need, just to better understand how much code and for doing what we need to put in them. I think that this will also help use better decide about traits vs services.

Ps Excuse me for the poor formatting: I’m on a mobile phone.

[alexislefebvre]

We could split the methods from WebTestCase in several groups:

• general utilities: getKernelClass(), getServiceMockBuilder(), getContainer()
• loading fixtures: loadFixtures(), loadFixtureFiles(), setExcludedDoctrineTables() and all the private methods that are needed by these methods
• running commands: runCommand(), setVerbosityLevel()
• authentication: createUserToken(), loginAs()
• request helpers and HTTP assertions: makeClient(), isSuccessful()¹, fetchContent(), fetchCrawler(), assertStatusCode()¹, assertValidationErrors()¹

Methods marked with ¹ were moved in the HttpAssertions one year ago.

[Jean85]

Someone else opened #338, can we move this forward? In my opinion that attack plan is a lot more practical and fast to implement, can we start with that to start the ball rolling?

[soullivaneuh]

I'm very excited about @alexislefebvre (#332 (comment)) proposition to split WebTestCase onto traits.

We indeed may use only fixture loading for a non web test case testing services.

[alexislefebvre]

@soullivaneuh in fact, it was an idea from Lukas 😉: #332 (comment)

I'll try to split the WebTestCase class in traits this week. Ideally, I'll be able to do this on the current branch without breaking anything. Then we could use a PSR-4 structure (see #324) and work on the future major release (see #359).

[lsmith77]

awesome!

[soullivaneuh]

Some samples I use to inspire somebody before having it on this bundle:

trait FixturesTestTrait
{
    use KernelTestTrait;

    /**
     * @var mixed[]
     */
    private $fixtures;

    /**
     * @before
     */
    public function loadFixtures(): void
    {
        if (null === $this->container) {
            $this->setUpSymfonyKernel();
        }

        $this->fixtures = $this->container->get('fidry_alice_data_fixtures.loader.doctrine')->load(
            $this->getFixturesFiles()
        );
    }

    /**
     * @return mixed
     */
    protected function getReference(string $reference)
    {
        return $this->fixtures[$reference];
    }

    /**
     * @return string[]
     */
    abstract protected function getFixturesFiles(): array;
}

trait KernelTestTrait
{
    /**
     * @var Kernel
     */
    private $kernel;

    /**
     * @var ContainerInterface
     */
    private $container;

    /**
     * @var Registry
     */
    private $doctrine;

    /**
     * @before
     */
    protected function setUpSymfonyKernel(): void
    {
        if (null === $this->kernel) {
            $this->kernel = new Kernel('test', false);
            $this->kernel->boot();
            $this->container = $this->kernel->getContainer();
            $this->doctrine = $this->kernel->getContainer()->get('doctrine');
        }
    }

    protected function getContainer(): ContainerInterface
    {
        return $this->container;
    }

    protected function getDoctrine(): Registry
    {
        return $this->doctrine;
    }

    /**
     * @after
     */
    protected function tearDownSymfonyKernel(): void
    {
        if ($this->kernel) {
            $this->kernel->shutdown();
        }
    }
}

Just use the first trait on your test case and write your getFixturesFiles method, and you are good to go! :-)

EDIT: I have some property conflict with the two trait, so I made only one.