phpunit: Usage of asserts is incorrect?

• This has nothing to do with performance
• This is due to how JUnit 3.8, which served as the initial inspiration for PHPUnit, did things ~ 15 years ago
• It allows developers to reuse PHPUnit’s assertions and constraints outside the context of PHPUnit
• A regular user of PHPUnit should use $this-> instead of self::

I still fail to see the motivation behind keeping things as they are.

This has nothing to do with performance

Great then.

This is due to how JUnit 3.8, which served as the initial inspiration for PHPUnit, did things ~ 15 years ago

Fair enough. But still, we’re15 years later, 6 major versions of PHPUnit have been released since then, slowly forgetting this heritage doesn’t look like a big deal.

It allows developers to reuse PHPUnit’s assertions and constraints outside the context of PHPUnit

Now that’s a good point.

A regular user of PHPUnit should use $this-> instead of self::

This is where I get lost. You’re giving all your reasons to keep the methods static, yet you advise users to call them dynamically, with no explanation whatsoever.

Why on earth should we call them dynamically?

What is the reasoning behind the statement?

A regular user of PHPUnit should use $this-> instead of self::

Its what I do, but why is it considered the ‘right’ choice?

https://phpunit.de/manual/current/en/appendixes.assertions.html#appendixes.assertions.static-vs-non-static-usage-of-assertion-methods

I understand the reasons above (and I find them reasonable), but maybe there can be a slight change? With growth of static analysis tools for PHP this could provide unnecessary warnings, because in most other cases if you call a static method non-statically that might be an error.

I think that it is certainly useful to allow reuse of asserts and doing it by static calls is perfectly fine in this case, but maybe there could be different behavior directly on TestCase? I mean instead of extending PHPUnit_Framework_Assert why not just call the static method in a new non-static method with the same name?

That means that the API would stay the same - obviously other than the static change, but maybe that is not a big BC break, because everywhere is written to use $this-> with these methods?

I would advocate to call methods with respect to their declarations as well.

For all who want’s to automate choosing his way of usage of assertions, PHP CS Fixer can do it for you with php_unit_test_case_static_method_calls rule - it can be configured to ensure assertions are called in unified way - $this->, self:: or static::

Fair enough. But still, we’re15 years later, 6 major versions of PHPUnit have been released since then, slowly forgetting this heritage doesn’t look like a big deal.

I feel with you. With higher phpstan levels you face the same thing in the default behavior of reporting. It is a code smell - totally. And I think it should be fixed up in one direction, both for usage and docs.

…and I almost went to PhpStorm bug tracker to submit a bug, that $this->assert… IDE suggestions don’t offer anything, but the code still works. I thought I was going insane…

I have thoroughly analyzed PHPUnit’s TestCase and Assert code, and unless we are going to declare willfully disregarding the static method calls are OK, then such an important code quality tool as PHPUnit should have code written for it using self::assertEquals() vs. the contextually ambiguous $this->assertEquals().

Or, PHPUnit needs to have a third class that does something like

// Expose static assert methods for people who need them.
class Assert
{
    public static function assertEquals($expected, $actual, $memo='')
    {
        return (new AssertLogic())->assertEquals($expected, $actual, $memo);
    }
}

Then extend TestCase against AssertLogic. Then literally everyone can be happy, at probably minimal overhead. (if the news are too much, then private static $asserter.

For anyone that is interested we have since moved to using Assert::assert*() methods directly. Since these are basically functions (no state) and do not need to be connected to TestCase, it makes more sense.

The only downside is that you have to add use for Assert (and potentially other classes), but I don’t mind that at all. Plus it becomes clearer if you use some custom asserts where they come from and you don’t have to “register” them in TestCase either.

Static anlyzers, IDEs, everything is warning and complaining about this. Why is this then an issue for those to report if it was totally “acceptable”? Sure, technically it is fine, but conceptually?

I like this idea 👍

But to better comply with PHPUnit standards, I won’t be calling Assert::assert*() statically, I’ll prefer:

(new class extends Assert {})->assert*();

…

</joke_mode>

I can see value in the assertions being declared statically, this probably allows for better code reuse.

But I cannot see the value in calling them via $this - It would be impossible to change the assertions to need object context because it would contradict the reason of code reuse, it would be a very breaking change.

If anything it just hide the fact that they are static and reusability.

I think the reason for static assertions is:

• They do not use object context, so you can reuse them anywhere
• Calling them via $this-> is valid and safe since it assumes object context (which is not required)
• If an assertion needs object context in the future, your tests will be safe
• If you use self:: then your tests will break

What’s the point of fixing tools if we can fix the documentation?

Fix the tools!

I thinks is great that the methods are defined as static when they are not using $this.

Please also notice:

In other languages like C# its event a hint that if some method don’t access instance data (don’t use this) can be declared static https://msdn.microsoft.com/en-us/library/ms245046.aspx

Calling a non-method method statically generates on PHP 5 E_STRICT & on PHP 7 E_DEPRECATED. But not the other way around. Call a static method non-statically is fine.

👯