New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage of asserts is incorrect? #1914

Closed
sergekukharev opened this Issue Oct 15, 2015 · 16 comments

Comments

@sergekukharev

sergekukharev commented Oct 15, 2015

It's not a secret that all asserts in PHPUnit are in fact static methods, but everyone are using them as they are not, even official documentation.

I think I understand the reasons, why they were made static in first place, probably it is related to performance (btw, do other reasons exists for that?).

But in my own opinion, if they are static, we should use them as we use other static methods to make our code cleaner.

This means using:

self::assertTrue(...);

instead of:

$this->assertTrue(...);

Can anyone give me a good reason why we shouldn't?

@fabiocarneiro

This comment has been minimized.

fabiocarneiro commented Oct 15, 2015

👯

@sebastianbergmann

This comment has been minimized.

Owner

sebastianbergmann commented Oct 15, 2015

  • 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::
@VasekPurchart

This comment has been minimized.

Contributor

VasekPurchart commented Jul 15, 2016

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?

@briedis

This comment has been minimized.

briedis commented Jul 22, 2016

...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...

@eclipxe13

This comment has been minimized.

Contributor

eclipxe13 commented Aug 9, 2016

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.

@mcfedr

This comment has been minimized.

mcfedr commented Aug 11, 2016

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?

@rtek

This comment has been minimized.

rtek commented Aug 23, 2016

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
@mcfedr

This comment has been minimized.

mcfedr commented Aug 24, 2016

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.

@BenMorel

This comment has been minimized.

BenMorel commented Feb 26, 2017

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?

mhujer added a commit to mhujer/elasticsearch-php that referenced this issue Aug 11, 2017

mhujer added a commit to mhujer/elasticsearch-php that referenced this issue Aug 11, 2017

mhujer added a commit to mhujer/elasticsearch-php that referenced this issue Aug 20, 2017

polyfractal added a commit to elastic/elasticsearch-php that referenced this issue Aug 21, 2017

[TEST] tests code cleanup (#618)
* [TEST] use ::class instead of classname string

Reason - when the class is renamed or removed, it will be detected much faster.
It helps with the static analysis of the code (also used by IDEs)

* [TEST] use assertSame() instead of assertEquals()

assertEquals() means ==
assertSame() is ===

Can cause tests not detecting issues, because different instances of
same class with same data are equal, but not the same.

* [TEST] convert array() to [] syntax

* [TEST] $this->assert* should be used

See
sebastianbergmann/phpunit#1914 (comment)

p365labs added a commit to p365labs/elasticsearch-php that referenced this issue Sep 10, 2017

[TEST] tests code cleanup (elastic#618)
* [TEST] use ::class instead of classname string

Reason - when the class is renamed or removed, it will be detected much faster.
It helps with the static analysis of the code (also used by IDEs)

* [TEST] use assertSame() instead of assertEquals()

assertEquals() means ==
assertSame() is ===

Can cause tests not detecting issues, because different instances of
same class with same data are equal, but not the same.

* [TEST] convert array() to [] syntax

* [TEST] $this->assert* should be used

See
sebastianbergmann/phpunit#1914 (comment)

p365labs added a commit to p365labs/elasticsearch-php that referenced this issue Sep 10, 2017

[TEST] tests code cleanup (elastic#618)
* [TEST] use ::class instead of classname string

Reason - when the class is renamed or removed, it will be detected much faster.
It helps with the static analysis of the code (also used by IDEs)

* [TEST] use assertSame() instead of assertEquals()

assertEquals() means ==
assertSame() is ===

Can cause tests not detecting issues, because different instances of
same class with same data are equal, but not the same.

* [TEST] convert array() to [] syntax

* [TEST] $this->assert* should be used

See
sebastianbergmann/phpunit#1914 (comment)

PowerKiKi referenced this issue in doctrine/dbal Sep 20, 2017

@tomasfejfar tomasfejfar referenced this issue Jan 31, 2018

Merged

HTTP extractor #1

6 of 6 tasks complete

o5 referenced this issue in phpstan/phpstan-strict-rules Jun 27, 2018

carakas added a commit to justcarakas/forkcms that referenced this issue Jul 23, 2018

@dereuromark

This comment has been minimized.

dereuromark commented Sep 8, 2018

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.

@sebastianfeldmann

This comment has been minimized.

sebastianfeldmann commented Sep 8, 2018

It is a code smell - totally.

No it's not! It's OO. There is no rule, that you can't call static methods within your instance context.
Static methods are callable either way. Only none static methods are not callable statically because they need the object context.
JUnit does the same thing by the way ;)

@dereuromark

This comment has been minimized.

dereuromark commented Sep 8, 2018

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?

@sebastianfeldmann

This comment has been minimized.

sebastianfeldmann commented Sep 8, 2018

Fix the tools!

@sergekukharev

This comment has been minimized.

sergekukharev commented Sep 11, 2018

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

@keradus

This comment has been minimized.

Contributor

keradus commented Sep 11, 2018

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::

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment