Data Fixtures Example Unjustified

[acleon]

This is a great resource, but I don't think your Data Fixtures section is right.

The information says that it's better to avoid shared state between tests and the "Bad" example given is using PHPUnit's setUp() method. But I don't think this example provides any shared state. The setUp() method should be called before each test. PHPUnit should also, as far as I'm aware, be running each test in a class in a new instance of the class.

From the manual...

The setUp() and tearDown() template methods are run once for each test method (and on fresh instances) of the test case class.

https://phpunit.readthedocs.io/en/9.5/fixtures.html#fixtures-examples-stacktest-php

[sarven]

Good point, I know how work setUp and tearDown, but I meant that we should place there only common things for all tests in particular test class, so I'll change this example and use there setUp method, because indeed these examples are confusing.

[sargath]

I've had the same impression. #13

We can also consider adding an example without usage of PHPUnit TestCase, although I don't know if this actually relevant

[sarven]

I was thinking about this problem and I'm a bit confused because when we're using the setUp method to prepare the SUT object our tests are coupled together. Changing the setUp method has an impact on all tests.

Moreover, not using the setUp section makes our test more readable, because every test is independent and we can find all necessary information in the body of the method.

public function suspending_a_new_subscription_with_cannot_suspend_new_policy_is_not_possible(): void
{
    $sut = $this->createANewSubscription();

    $result = $sut->suspend(new CannotSuspendNewSubscriptionPolicy(), new \DateTimeImmutable());

    self::assertFalse($result);
}

public function suspending_a_new_subscription_with_cannot_suspend_new_policy_is_not_possible(): void
{
    $result = $this->sut->suspend(new CannotSuspendNewSubscriptionPolicy(), new \DateTimeImmutable());

    self::assertFalse($result);
}

The first example is longer, but IMHO it's more readable because I can see all necessary information related to the arrange section.

What do you think?

[sargath]

I agree with this as well. This also gets more interesting once you use @dataProvider, then this approach is more suitable.

I found some of my colleagues scratching their heads why I haven't used setUp as it is a preferable way using testing framework (PHPUnit) 😅 It also gets tedious when you have more complex testing class.

That is why, perhaps, we should think about having a couple of examples.

[sargath]

Any further considerations?

[sarven]

I am not sure that we should change these examples. Perhaps we can just add a comment to the BAD section in Test Fixtures, with an explanation of why it's bad:

• worse readability
• all tests coupled together

Then we can just a new example with dataProvider, and also show better usage of the setUp method than the one I created for the Bad example, with a proper explanation that not every usage of setUp is bad, and the author of tests should consider pros and cons. What do you think? Do you want to adjust your PR after our discussion here?

[sargath]

Something like this? Please, take a look #13

[sarven]

@acleon @sargath has fixed this issue 👍 Changes have been merged.