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

sebastianbergmann commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

localheinz commented 5 years ago

@sebastianbergmann

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

gmponos commented 5 years ago

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

sebastianbergmann commented 5 years ago

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

gmponos commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

Hi @gmponos!

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

do a first attempt to reset the password
assert that the maximum number of attempts has not been reached
do a second attempt to reset the password
assert that the maximum number of attempts has not been reached
do a third attempt to reset the password
assert that the maximum number of attempts has not been reached
do a fourth attempt to reset the password
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 commented 5 years ago

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 commented 5 years ago

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

gmponos commented 5 years ago

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 commented 5 years ago

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

sebastianbergmann commented 5 years ago

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

gmponos commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

epdenouden commented 5 years ago

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

gmponos commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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.

gmponos commented 5 years ago

Thank you for your responses and taking the time to explain things.. Time and maybe acceptance from community will tell.. :)

I will try to be constructive and although I disagree I might even contribute down this road of strictness

see https://github.com/sebastianbergmann/phpunit/issues/3458

vekien commented 5 years ago

Hello,

After this deprecation, Is it best practice to use reflection to modify the scope in order to unit test private/protected methods?

kubawerlos commented 5 years ago

No,

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

maksimovic commented 5 years ago

Sorry for the activity spam, I totally forgot that referencing an issue in commits will also propagate here. Deleting the branch and creating a new one with all changes merged into single commit didn't help :(

Also didn't realize that there's an assignee here, too. And probably it's too early for v9 pull requests.

Regardless, there's this: https://github.com/sebastianbergmann/phpunit/compare/master...maksimovic:9.0-assert-cleanups?expand=1

It includes #3342 because there are dependencies between the functions/methods being removed in v9, so both of the issues will need be solved in the same PR.

gilbertomangones commented 4 years ago

Excuse me, this function is replace by? assertattributeequals in https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Assert.php

Thank's

filakhtov commented 4 years ago

I apologize for writing on the closed issues, but I missed this one and have a question. How do we test factories after this change? Assuming I have a factory that produces objects with certain configured state. None of this state is publicly accessible, however it will be influencing how an object behaves. What I have been historically doing was asserting that factories put an object into a particular state. Moreover, because factories by their nature are designed to instantiate objects, I can't have expectations on what is passed into the constructor due to static coupling. Any clarification is highly appreciated!

deepeloper commented 9 months ago

Wrong way, let people to test everything (

sebastianbergmann / phpunit

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