Sniff for wrong order of PHPUnit assert parameters
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Krinkle
	May 28 2020, 8:08 PM

Description

assertEquals( expected, actual, … );
assertSame( expected, actual, … );

This Yoda-conditional order is confusing and very frequently done incorrectly.

This is hard to catch in code review, as no matter how many times I fix it, it still "feels" right everytime I see it. (Selective memory loss?)

While it's hard to catch 100% of the cases where this is done incorrectly (e.g. if both are a variable), it is fairly easy to detect the majority of cases, which is where one of them is a literal.

assertSame( 42, $something ); # right
assertSame( $something, 42 ); # wrong

We already do this for QUnit assertions (which btw, take their arguments in the other way around) in our ESLint configuration. Would be nice to do this for PHPUnit/PHPCS as well.

If the test is passing, it works both ways, given $foo == $bar and $bar == $foo produce the same result. Where it falls apart is when the test is failing. There will be a diff of Expected/Actual which would be the wrong way around. And, especially when that diff is optimised to only show the parts that are wrong, this could also omit information you might want to see.

Details

	Subject	Repo	Branch	Lines +/-
	Add AssertionOrderSniff	mediawiki/tools/codesniffer	master	+191 -0

Customize query in gerrit

Related Objects

Mentioned In: T242762: Expand PHPUnitAssertEqualsSniff to better auto-fix comparisons with count()

Event Timeline

Krinkle created this task.May 28 2020, 8:08 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 28 2020, 8:08 PM

DannyS712 subscribed.May 28 2020, 8:09 PM

Should be fairly straightforward for the majority of cases
Given that this would be adding a fifth phpunit sniff, I've submitted https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/codesniffer/+/599482/ to move the phpunit sniffs out of /Usage/ and into a new group/folder, /PHPUnit/. Will submit this on top of that change

Restricted Application added a project: User-DannyS712. · View Herald TranscriptMay 29 2020, 2:49 AM

DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.May 29 2020, 2:49 AM

A while ago I did a few manual cleanups and tried to find flipped "expected" and "actual" parameters.

If the second parameter of an assertEquals or assertSame is a literal, that's guaranteed to be the wrong way around and fine to auto-fix. Literals to look for include int, float, string, null, true, false.
- I think it's worth checking if the first argument is a literal as well. If both are literals, that's something else, and the sniff should just skip this assertion.
- Note that arrays are not necessarily literals. For example, assertEquals( $foo, [ call() ] ) is fine. The sniff would need to check if the array is entirely made of literals. Then an auto-fix would be fine. However, I'm not sure if this is worth the effort.
It might be worth looking for variable names that start with $actual… or $expect…, but are in the wrong spot. While this is not guaranteed to be wrong, it's confusing and should be marked with a warning, in my opinion. An auto-fix should not touch these.
I was thinking about something like assertEquals( call(), $var ), but realized reporting this would cause to many false positives. See, assertEquals is also used to check if the result of two code paths is the same. A sniff can't know if the $var in my example is a static value or the result of a function call as well. Tracking this would be something for Phan – but probably not worth it.