One Invocation Per Expected Exception
ID |
php.one_method_invocation_expected_testing |
Severity |
high |
Remediation Complexity |
medium |
Remediation Risk |
low |
Remediation Effort |
medium |
Resource |
Reliability |
Language |
Php |
Tags |
reliability, testing |
Description
Reports a test that expects an exception while invoking the code under test more than once. Only one of those calls is the one being tested, and nothing in the test says which.
Rationale
When such a test passes, it proves only that something in the block raised the declared exception. The day the wrong call starts raising it — a changed precondition, a stricter setter, a mock that now throws — the test keeps passing while the behaviour it was written for is no longer covered. Splitting the block so the expectation covers exactly one call makes the test say what it means, and makes a future failure point at the right line.
Both ways a PHP test expresses the expectation are covered.
<?php
use PHPUnit\Framework\TestCase;
final class CheckoutTest extends TestCase
{
private Checkout $subject;
public function testCheckoutRejectsAnEmptyBasket(): void
{
$this->expectException(EmptyBasket::class); // FLAW — two calls can each raise it
$this->subject->lockBasket();
$this->subject->complete();
}
public function testCheckoutRejectsAnExpiredCard(): void
{
try { // FLAW — two calls can each raise it
$this->subject->authorise('4111');
$this->subject->capture();
$this->fail('an expired card must be rejected');
} catch (CardExpired $e) {
$this->assertSame('expired', $e->getCode());
}
}
}
What counts as invoking the subject is deliberately narrow, so that a finding names a genuine ambiguity:
-
an assignment keeps its value and is therefore setup — which also stops the everyday
$s = new Service(); $s→run();pair from reading as two candidates; -
a statement carrying an assertion, an
expect…declaration, a skip marker orfail(…)states an expectation rather than exercising anything; -
a line of double wiring,
$gateway→method('charge')→willReturn(false);, configures a mock; -
only statements after the
expectException(…)declaration count, and only at the top level of the method — a call before it is setup, and one nested in a loop or a condition is not plainly one of a sequence; -
the manual idiom needs its
fail(…); without it atrythat happens to hold two calls is ordinary error handling, not an expected-exception block.
A PHP finding does not claim that each of the calls can raise the declared type — PHP has neither checked exceptions nor a throws clause, and in a PHP test the throwing call is nearly always a method of the subject declared in another file. What it says is that the expectation covers more than one invocation, so a failure cannot say which one raised.
Remediation
Move everything that is setup out from under the expectation, leaving the one call being tested.
<?php
use PHPUnit\Framework\TestCase;
final class CheckoutExpectationTest extends TestCase
{
private Checkout $subject;
public function testCheckoutRejectsAnEmptyBasket(): void
{
$this->subject->lockBasket();
$this->expectException(EmptyBasket::class);
$this->subject->complete();
}
}
If two calls genuinely need covering, write two tests, so each names the behaviour it proves.