[2.0] Reduce size of WebTestCase?

[alexislefebvre]

Problem

As suggested by @soullivaneuh, we can discuss here about the size problem of WebTestCase.

The last version of WebTestCase contains 888 lines, including 28 use declarations: https://github.com/liip/LiipFunctionalTestBundle/blob/3bca1f2349c694f16670d6de63020c28c55df9fa/src/Test/WebTestCase.php

This class has accumulated features since 7 years, this is nice but it looks more and more like a Big Ball Of Mud. We have the opportunity to clean it up.

---

I identified some groups of related methods (methods marked with ¹ were moved in the HttpAssertions one year ago):

• 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()¹

We're very far from Single responsibility principle. 😅

Interestingly, about one half of the class code is used to load fixtures. So I think that it should be the first thing to move.

Solution

@lsmith77 has suggested several possibilities:

• traits: Refactor the bundle to be more stable and faster #332 (comment)
• services: refactor Bundle to provide all features as services refactor Bundle to provide all features as services #6 (this one is really old 😃)

Traits

I was in favour of traits but @Jean85 added a strong argument againt it:

I'm personally against traits. They are simple a dirty way to avoid copy and paste, and I don't think we have many places were we should use it.

With traits, complexity would persist since WebTestCase and its traits would still be linked together.

Services

With services, we could add something like the liip.functional_test.fixtures_loader (or the class name) and call $this->getContainer('liip.functional_test.fixtures_loader')->loadFixtures(…); $this->getContainer()->get('liip.functional_test.fixtures_loader')->loadFixtures(…); instead of $this->loadFixtures(…);`. This is longer to type but it will be easier to maintain a service with its own class than a long class.

We may also inject this liip.functional_test.fixtures_loader service in WebTestCase in order to provide a shortcut to this service.

[Your solution here]

Please share your thoughts on this subject!

[lsmith77]

I agree that traits are the less elegant solution. Essentially they are a quick fix band aid. So to me the choice boils down to how much time we want to invest. The service route is the cleaner route but more work.

[Jean85]

It may not be so long, but the main issue is that we cannot do $this->getContainer()->get('...') of many stuff, because with Symfony 4 we would hit the issue of private services. Maybe defining a single, fixture-dedicated service with injected dependecies would be better?

[server-may-cry]

Could we deprecate runCommand or create additional method which will return CommandTester? https://symfony.com/doc/current/console.html#testing-commands
We still can use fixtures.

related to #219

[Jean85]

I agree, using the CommandTester is surely better. Maybe we could add a protected method in place to easily create it with a single call, using the command name as the only required argument.

[alexislefebvre]

Most of the code has been moved in several classes, thanks to #398 🎉