Remove assertions (and helper methods) that operate on (non-public) attributes

[sebastianbergmann]

The following methods should be removed:

• assertAttributeContains()
• assertAttributeNotContains()
• assertAttributeContainsOnly()
• assertAttributeNotContainsOnly()
• assertAttributeCount()
• assertAttributeNotCount()
• assertAttributeEquals()
• assertAttributeNotEquals()
• assertAttributeEmpty()
• assertAttributeNotEmpty()
• assertAttributeGreaterThan()
• assertAttributeGreaterThanOrEqual()
• assertAttributeLessThan()
• assertAttributeLessThanOrEqual()
• assertAttributeSame()
• assertAttributeNotSame()
• assertAttributeInstanceOf()
• assertAttributeNotInstanceOf()
• assertAttributeInternalType()
• assertAttributeNotInternalType()
• attribute()
• attributeEqualTo()
• readAttribute()
• getStaticAttribute()
• getObjectAttribute

[localheinz]

@sebastianbergmann

Interesting!

When you have the time, can you share what the plans are? Should these be replaced by using non-specialized (or domain-specific) assertions in userland code?

[sebastianbergmann]

These assertions make it too easy to test implementation details (private and protected attributes) that should not be tested directly.

[localheinz]

@sebastianbergmann

Ha, I missed the Attribute part. Let’s kill them with 🔥!

[gmponos]

Is there any related discussion that explain the thought behind this decision?

[sebastianbergmann]

A test should not depend on private implementation details. It was a bad idea to make a bad testing practice this convenient.

[gmponos]

These assertions make it too easy to test implementation details (private and protected attributes) that should not be tested directly.
A test should not depend on private implementation details. It was a bad idea to make a bad testing practice this convenient.

I know that this is true but on the other hand this is just a misuse of a very helpful feature let me give you one of my use cases:

namespace Whatever;

class ResetPasswordHandler
{
    /** @var ResetPasswordRepository */
    private $resetPasswordRepository;

    public function __construct(ResetPasswordRepository $resetPasswordRepository)
    {
        $this->resetPasswordRepository = $resetPasswordRepository;
    }

    public function handle(string $oneTimeToken, string $email)
    {
        /** @var ResetPassword $resetPassword */
        $resetPassword = $this->resetPasswordRepository->findOneBy(['email' => $email]);

        if (!$resetPassword->hasReachedAttemptLimit() && $resetPassword->getToken() !== $oneTimeToken) {
            $resetPassword->increaseAttempts();
            $this->resetPasswordRepository->save($resetPassword);
            return false;
        } else {
            return true;
        }
    }
}

/** My entity class */
class ResetPassword
{
    private CONST MAX_ATTEMPTS = 3;

    private $uuid;
    private $attempts;
    private $lastAttempt;
    private $email;
    private $token;

    public function __construct($uuid, $email, $token, $attempts, \DateTimeInterface $lastAttempt)
    {
        $this->uuid = $uuid;
        $this->attempts = $attempts;
        $this->lastAttempt = $lastAttempt;
        $this->email = $email;
        $this->token = $token;
    }

    public function hasReachedAttemptLimit()
    {
        return $this->attempts >= self::MAX_ATTEMPTS;
    }

    public function increaseAttempts()
    {
        $this->attempts++;
        $this->lastAttempt = new \DateTime();
    }

    /** @return string */
    public function getToken()
    {
        return $this->token;
    }
}

class ResetPasswordHandlerUnitTest extends TestCase
{
    private $repository;
    private $handler;

    public function setUp()
    {
        $this->repository = $this->getMockBuilder(ResetPasswordHandler::class)->disableOriginalConstructor()->getMock();
        $this->handler = new ResetPasswordHandler($this->repository);
    }

    public function testThatItIncreasedTheCount()
    {
        $entity = new ResetPassword('12234', 'test@test.com', 'whatevertoken', 0, new \DateTime());
        $this->repository->expects($this->once())->method('findOneBy')->willReturn($entity);
        $this->handler->handle('notmatch', 'test@test.com');
        $this->assertAttributeSame(1, 'attempts', $entity);
    }
}

If you remove these functions in order for me to test that the developer has not made anything stupid like commenting a line like this:

    public function increaseAttempts()
    {
	    // $this->attempts++;
        $this->lastAttempt = new \DateTime();
    }

I will need to create a getter function for attempts which honestly I hate doing. I mean you shouldn't add functions just in order to test your code. Another solution would be to extend this entity under a Test namespace and add the getter there.

So again if a developer wants to actually test the private details of an object by removing this we are forcing them follow even worse paths.

Note that I guess that most probably you will disagree with how the example is structured but similar scenarios happen regularly in everyday life and hence these are some pretty useful functions

[localheinz]

@gmponos

How about testing it like this instead?

final class ResetPasswordTest extends TestCase
{
    public function testHasReachedAttemptLimitReturnsFalseWhenAttemptLimitHasNotBeenReached(): void
    {
        $attempts = 0;

        $resetPassword = new ResetPassword(
            '12234',
            'test@test.com',
            'whatevertoken',
            $attempts,
            new \DateTimeImmutable()
        );

        $resetPassword->increaseAttempts();

        $this->assertFalse($resetPassword->hasReachedAttemptLimit());
    }

    public function testHasReachedAttemptLimitReturnsTrueWhenAttemptLimitHasBeenReached(): void
    {
        $attempts = ResetPassword::MAX_ATTEMPTS - 1;

        $resetPassword = new ResetPassword(
            '12234',
            'test@test.com',
            'whatevertoken',
            $attempts,
            new \DateTimeImmutable()
        );

        $resetPassword->increaseAttempts();

        $this->assertTrue($resetPassword->hasReachedAttemptLimit());
    }
}

There are always alternatives! 👍

[gmponos]

Yea.. ok.. in this case you got me.. but the example was just for the sake of the argument... For instance what if he comments the $this->lastAttempt = new \DateTime();...?

There will be cases that this will make testing much harder driving developers to harder and worse paths... :(

[epdenouden]

Hi @gmponos!

Step away from the PHP for a minute. What behaviour do you want to test? How about this:

1. do a first attempt to reset the password
2. assert that the maximum number of attempts has not been reached
3. do a second attempt to reset the password
4. assert that the maximum number of attempts has not been reached
5. do a third attempt to reset the password
6. assert that the maximum number of attempts has not been reached
7. do a fourth attempt to reset the password
8. assert that the maximum number of attempts has been reached

I know that this is true but on the other hand this is just a misuse of a very helpful feature let me give you one of my use cases:
[...]
I will need to create a getter function for attempts which honestly I hate doing. I mean you shouldn't add functions just in order to test your code.

The testing problem is quickly solved by looking at the difference between testing implementation versus testing behaviour of a system. When I started working on the test execution reordering and its cache I ran into this question. The TestSuiteSorter and ResultCacheExtension are designed to work without any user output and to have as little effect as possible.

My first tests relied heavily on --debug output and even reflection. This caused me a lot of grief later on, when refactoring the TestSuiteSorter forced me to rewrite a lot of tests. Apart from all the extra work the testing confidence went out the window, as I had a lot fewer stable tests during the refactoring! Not fun.

The cleanest solution turned out to be to only test the observable external behaviour:

• the TestSuiteSorter should give me: TestSuites that are sorted in a different order. I can observe this by looking at the order the tests are actually run. I can observe this with --debug (not great) or by watching a streaming log format like TeamCity (much better!)
• the ResultCacheExtension should give me: a result cache file that matches the results of the previous run

[gmponos]

Step away from the PHP for a minute. What behaviour do you want to test? How about this:

The steps mentioned here do give a different perspective... 👍

Thanks for the explanation guys.. I will have them in mind and see how I can start removing these functions from my projects... and see how this goes.. Never the less at the moment as I see it as an overall I still insist on my 👎 hopefully I will be proven wrong at the future.

[TomasVotruba]

@gmponos Hi, I'm working on instant migration tool @rectorphp. Would any automatization help you? E.g. use getter over private property?

[gmponos]

Unfortunatelly no.. IMHO open an issue if you wish at your repo and I will explain you my arguments there... no need to start a discussion about your tool here and add extra noise :)

[TomasVotruba]

I think this is the best place for upgrade paths most people would look for. If there is none, no troubes :)

[sebastianbergmann]

The upgrade path is to refactor your (test) code to not require the assertions in question.

[gmponos]

@TomasVotruba look.. most probably in the places that I used one of these assertions I did not had a getter.. Otherwise I wouldn't use them in first place...

if you create a tool just for the cases that the getters exists then I would say that there will be a lot of developers that will misinterpret the intention and they will think that the correct solution is to add a getter just for the tests.

[TomasVotruba]

@gmponos I just wasn't sure how people use it, since I have no experience with that. It's clear to me now there is no way to automate this. I look for A -> B changes

[yaquawa]

This means we have no way to test private properties after this PR ??

[epdenouden]

If you see no way around inspecting private properties you can still use reflection.

[gmponos]

If you see no way around inspecting private properties you can still use reflection.

As far as I know PHPUnit uses reflection in order to assert these properties with these functions. So in the end the developers instead of using the wrapper of PHPUnit will end to do reflection on their own...

Also I have been thinking about your comment before...

What behavior do you want to test?

Yes this is true... but I am trying to do unit test... I would accept this argument from a library that does behavior testing like behat... not from PHPUnit.. which is as the name suggests for Unit tests...

[TomasVotruba]

@gmponos

Yes this is true... but I am trying to do unit test... I would accept this argument from a library that does behavior testing like behat... not from PHPUnit.. which is as the name suggests for Unit tests...

I see what you mean. I think unit testing doesn't mean to test all private properties/metods, but rather public api of the object.

If we have big class with 50 private methdos, we shouldn't use reflection to test private elements. We should decouple to standalone responsible objects. And then, test their public api.

[gmponos]

@TomasVotruba I was not talking about testing actual private methods... but about testing public methods that do some alteration...

Check the following example for instance:

class MyLogger {
   private $formatter;
   private $client;

   public function __constructor(HttpClient $client){
        $this->client = $client;
   }

   public function setFormatter($formatter){
        $this->formatter = $formatter;
   }

   public function log($level, $message, array $context){
          $message = $this->formatter($message);
          $this->client->send($level, $message, $context);
   }

my only way to test this now is

• call setFormatter once
• call Log
• see in my test how the client has sent the log
• call set formatter second time setting a different formatter
• call log again
• see that the format actually changed...

While before I was:

• Check property of the class that it is X
• set formatter
• Check property of the class that it is Y

I do agree that first way may seem more "sophisticated" but isn't this getting out of the scope of actual unit testing and moving to another level or is it just me thinking this way?

Edit also the same thing applies for my previous example.. Yes I can test it with the way that it is mentioned.. executing the test with four attempts etc etc... but again this reminds mostly practices from testing suites related with behavior...

[epdenouden]

As far as I know PHPUnit uses reflection in order to assert these properties with these functions. So in the end the developers instead of using the wrapper of PHPUnit will end to do reflection on their own...

Well yes and water is wet. If you cannot implicitly use PHPUnit's reflection-based wrapper, you will have to write your own. Which I am sure some people will do and even create a nice thing on Packagist for it.

What behavior do you want to test?

Yes this is true... but I am trying to do unit test... I would accept this argument from a library that does behavior testing like behat... not from PHPUnit.. which is as the name suggests for Unit tests...

How does a 'unit' test not test the behaviour of the system under test? If you want get creative with the language surrounding testing, sure! Here is one: we cannot even unit test, it is impossible. We are always testing integration: your code with PHP, a filesystem, an OS (with multitasking and IO interrupts, no less!)

Your tests lock down the implementation of a thing by inspecting the inside. That is not 'wrong' as such; sometimes you have to do that, mostly when it comes to licensing/auditing/legal reasons. You want to make sure people do 1+1=2 the way you expect them to. In the software communities I am part of that is not considered unit testing because I'd have to change my test if the implementation changes. The major downside is that I cannot use the tests to improve my refactoring confidence this way: code and tests change together.

Some references from current PHPUnit development work:

• a new fast, minimal health-check test collection for project/extension developers
• example of making the insides of a thingamajig testable
• a lot of our own tests/end-to-end is this way because testing a system requires us to jump through some hoops

Hope this provides you with a bit of a scratching pole for your thoughts about testing. Wanting to look inside is not wrong but it has a shitload of side-effects.

[epdenouden]

I do agree that first way may seem more "sophisticated" but isn't this getting out of the scope of actual unit testing and moving to another level or is it just me thinking this way?

Thank you for providing so much detail on your thought process and example code. That's really great. :)

The logger example is a classic and you could tests this a bit 'quicker' by using dependency injection (e.g. a "log output stream thingy" that you can check the buffer of) or using a Mock with expectations on the calls. Which is turn out to be very much alike once you draw it out on paper.

[deepeloper]

Wrong way, let people to test everything (