Page MenuHomePhabricator

Add PHPCS sniff to encourage assertSame() when comparing to literal values
Open, Needs TriagePublic

Description

Follows https://gerrit.wikimedia.org/r/53744/ by @thiemowmde.

assertEquals tolerates type casting and can easily lead to mistakes or bugs slipping into the code base, e.g. cases where tests were doing assertEquals(0, …) or assertEquals('', …) when in actuality the return value was null, and the tests still pass.

This is an anti-pattern I think, and would be nice to prevent regression from this in the future.

Note that we have similar rules for our QUnit code base already.

Event Timeline

Krinkle created this task.Fri, Sep 20, 3:33 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFri, Sep 20, 3:33 PM

Please note: Even if assertEquals() relies on the magic type casting behavior of PHP's == operator, it's not entirely identical. There is one special case: if one side is a string, both are forcefully cast to strings before comparison. This means that assertEquals() behaves identical to assertSame() in all situations where one value is a string, and this string is not a "falsy" one ('0' or the empty string). In other words: A sniff should only look for assertEquals( '', … ) and assertEquals( '0', … ), but no other strings.

// PHP would consider these equal, but PHPUnit does not:
$this->assertNotEquals( '', 0 );
$this->assertNotEquals( '9a', 9 );

I'm not sure it it makes sense to forbid the usage of assertEquals( 0, … ), assertEquals( false, … ), assertEquals( null, … ), and such. There might be tests that intentionally check for any falsy value.