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 T246674#5933045, @thiemowmde wrote:

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.

In T246674#6326093, @thiemowmde wrote:

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.

Here is four reasons:

1. Use of empty() has long been identified as an anti-pattern in source code (code conventions). To my knowledge, all the same reasons apply to tests as well, e.g. increased review/comprehension time, decreased confidence in author's intent or understanding of the code, decrease confidence in it working correctly, decreased confidence during refactoring due to perhaps keeping things "just in case" or having to deal with something being construed as a "possibly" breaking change — it lacks intent.

2. In my view, a good test suite, essentially doubles as documentation showing how the code should be called, what it supports, and what its contract/expected result is to the outside. When I read assertEmpty() I know very little. The previous comment by @thiemowmde cites 6 possible interpretations of what a (confident) author may have intended with assertEmpty(). That seems quite clear evidence to me that it is a rather poor practice. Plus, while I haven't tried to come up with additional high-confidence intepretations, I can think of plenty more possible intentions that someone less familair with the codebase may have intended. E.g. It could be trying to cover both false or null when a function can do both and during refactoring may flip and the difference would be unnoticed. Similarly, it may flip from null (bottom/disabled value) to an empty string (mock/enabled value) and it would not be noticed. Tests are the last place where we want to tolerate doubt.

3. In the same sense that code like assert.ok (blog post) can easily mask broken code when e.g. something is an empty object instead of a number/string/callable; so can assertEmpty easily mask broken tests or broken code where null masks when something should be 0/false/empty string. Given that PHPUnit will often involve null values from mocked methods, it basically means for any method meant to return something other than null, assertEmpty will not tell you the difference between the test doing nothing vs the test executing your code and verifying the intended bottom value.

4. nullable methods are not uncommon, e.g. ?string, ?array, ?int, or their |null variants. assertEmpty likewise is ambigous for these. Of course, you could look up every time you see this whether that's the case. This is overhead, as result of doubt, and further increases the gap between experienced and inexperienced reviewers for no gain.

My suggestion is we enable this rule immediately, but as indicated by Danny, let LibUp disable it automatically in repositories that currently do make use of it. Most extensions have 0 tests that use this method, which means we immediately prevent addition there, and also have one less thing to look for during code review, and give developers faster feedback.

I recognise that it isn't urgent to change existing tests.

I do not recognise valid reasons for a maintainer to accept this in new tests. A reviewer should therefore look out and amend or refuse to merge new tests that use this method, every day. A lint rule improves the manner and time it takes to get that signal, by getting it from CI or IDE, thus increasing productivity, and also improving on-boarding by having this discovered, declared, and enforced consistently.

Change 591242 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Create PHPUnitAssertEmpty sniff to warn against using assertEmpty

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