Literal Boolean Or Null In Assertion
ID |
php.no_literal_bool_in_assertion |
Severity |
high |
Remediation Complexity |
trivial |
Remediation Risk |
low |
Remediation Effort |
low |
Resource |
Reliability |
Language |
Php |
Tags |
assertion, reliability, testing |
Description
Reports a PHPUnit assertion whose expected value is the literal true, false or null, and the
mirror-image case of a boolean or null assertion applied to a literal.
Rationale
A literal in the expected position. assertEquals(true, $x) runs, but it compares loosely against
a boolean, so 1, 'a' and [0] all satisfy it — the assertion is far weaker than it looks. Its
failure message reports a value mismatch instead of saying that the flag was not set. PHPUnit has a
dedicated assertion for each of the three literals, and it is both stricter and clearer.
A literal as the value under test. assertTrue(true) is fixed at parse time: it passes whatever
the code under test does, so it says nothing while hiding a check that was meant to be there. The
usual causes are a condition replaced by a constant while debugging and never restored, and a
generated test whose condition was never filled in.
<?php
$this->assertEquals(true, $this->subject->isActive()); // FLAW — use assertTrue
$this->assertSame(false, $this->subject->isFrozen()); // FLAW — use assertFalse
$this->assertSame(null, $this->subject->closedAt()); // FLAW — use assertNull
$this->assertNotSame(null, $this->subject->openedAt()); // FLAW — use assertNotNull
$this->assertTrue(true); // FLAW — always passes
$this->assertNull(null); // FLAW — always passes
$this->assertSame('active', $this->subject->label()); // OK — a string is not this smell
$this->assertTrue($this->subject->isActive() === true); // OK — the literal is inside an expressionThe opposite literal — assertTrue(false), assertFalse(true), assertNull(true) — always
fails instead, and is deliberately not reported: it is the idiomatic stand-in for fail(…) inside
a catch block or an otherwise-unreachable branch, usually carrying a message that says why
execution should never get there.
Only the documented expected-value position — the first argument — is inspected. A literal in the
actual position, assertEquals($x, true), is the arguments written the wrong way round: a
different defect with a different fix, left alone rather than reported with advice that would not
apply to it.
Remediation
Use the assertion PHPUnit provides for the value being checked. Each one is strict, so it also rules out the loosely-equal values the comparison form would have accepted.
<?php
$this->assertTrue($this->subject->isActive());
$this->assertFalse($this->subject->isFrozen());
$this->assertNull($this->subject->closedAt());
$this->assertNotNull($this->subject->openedAt());When the assertion was standing in for a check that is still missing, write that check instead.