Page MenuHomePhabricator

Treat assertEmpty() as problematic in PHPUnitAssertEquals
Open, Needs TriagePublic

Description

This method promotes doubts and uncertaintity and masks most types of failure, in favour of syntactal brevity over assertCount( 0, …) or assertSame( [], …) which should be used instead (for asserting an empty array), or for other bottom values, the strict assertion should be used instead, e.g. assert strictly equal to null, 0, "", "0", false etc. - whichever is the expected value.

Event Timeline

In the past, I liked PHP's empty() exactly because it is so relaxed. Whenever I saw something like if ( !empty( $var ) ), I was sure no unwanted empty or empty-ish value can make it to my code.

But we have a type system now. Code that expects an array should say so. Code that returns an array should say so. We can use $var !== [] and be sure there is no edge-case we forgot about.

Same applies to PHPUnit assertions. A test should know both what type and what value it expects. assertSame( [], $array ) does both in one assertion, and so does assertCount( 0, $array ). I can not think of a use case for assertEmpty() that can not be replaced with something better.

Problem is: It's impossible to auto-fix assertEmpty() precisely because it is so relaxed and the value could be just anything, an array, a string, null, who knows. Is this really worth the manual (!) effort?

I'm fine with adding a sniff for this, but please make it a separate sniff.

Change 591242 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/tools/codesniffer@master] Create PHPUnitAssertEmpty sniff to warn against using assertEmpty

https://gerrit.wikimedia.org/r/591242

In the past, I liked PHP's empty() exactly because it is so relaxed. Whenever I saw something like if ( !empty( $var ) ), I was sure no unwanted empty or empty-ish value can make it to my code.

But we have a type system now. Code that expects an array should say so. Code that returns an array should say so. We can use $var !== [] and be sure there is no edge-case we forgot about.

Same applies to PHPUnit assertions. A test should know both what type and what value it expects. assertSame( [], $array ) does both in one assertion, and so does assertCount( 0, $array ). I can not think of a use case for assertEmpty() that can not be replaced with something better.

Problem is: It's impossible to auto-fix assertEmpty() precisely because it is so relaxed and the value could be just anything, an array, a string, null, who knows. Is this really worth the manual (!) effort?

I'm fine with adding a sniff for this, but please make it a separate sniff.

No autofixing included, just a warning; done in a separate (new) sniff

I'm still not really sure if it's a good idea to block people from using assertEmpty. I mean, it's still doing an assertion, isn't it? It will work as expected in many situations:

  • When an empty array is expected, but a non-empty one is returned.
  • When null is expected, but an object returned.
  • When 0 is expected, but a number greater than zero returned.
  • When false is expected, but true returned.
  • When an empty string is expected, but a non-empty one returned. Ok, it will miss the ugly "0" edge case. But it will still catch everything else.

Sure, we could rewrite them all. But do we really want to manually update all 280 usages that are currently found by CodeSearch? I feel this is very expensive for (in my opinion) rarely any value.

I'm still not really sure if it's a good idea to block people from using assertEmpty. I mean, it's still doing an assertion, isn't it? It will work as expected in many situations:

  • When an empty array is expected, but a non-empty one is returned.
  • When null is expected, but an object returned.
  • When 0 is expected, but a number greater than zero returned.
  • When false is expected, but true returned.
  • When an empty string is expected, but a non-empty one returned. Ok, it will miss the ugly "0" edge case. But it will still catch everything else.

Sure, we could rewrite them all. But do we really want to manually update all 280 usages that are currently found by CodeSearch? I feel this is very expensive for (in my opinion) rarely any value.

Libup will disable it for all of the current uses, but going forward new uses in repos that don't currently have any will be warning that something more specific can be used

See, I'm not sure about this "can". Do we need to force people to be more specific, when assertEmpty works just fine in so many situations? What do we win?

We just run into a similar discussion in T266823#6672755. The sniffs we have in the MediaWiki CodeSniffer rule set don't work as "it would be nice if you could consider this" suggestions. They work as strict rules. I.e. what we do here is effectively bannig assertEmpty. I just started to realize that vague suggestions that have so many possible exceptions can possibly create so much hassle, it might not be worth it.