acc-detect-test-smells

name	acc-detect-test-smells
description	Detects test antipatterns and code smells in PHP test suites. Identifies 15 smells (Logic in Test, Mock Overuse, Fragile Tests, Mystery Guest, etc.) with fix recommendations and refactoring patterns for testability.

Test Smell Detection

Identifies antipatterns and code smells in PHP test suites.

15 Test Smells

1. Logic in Test

Problem: Test contains conditional logic (if/for/while).

Detection:

Grep: "if \(" --glob "tests/**/*Test.php"
Grep: "for \(" --glob "tests/**/*Test.php"
Grep: "while \(" --glob "tests/**/*Test.php"
Grep: "foreach \(" --glob "tests/**/*Test.php"

Example (Bad):

public function test_calculates_total(): void
{
    $items = [10, 20, 30];
    $total = 0;
    foreach ($items as $item) {  // ❌ Logic in test
        $total += $item;
    }
    self::assertEquals($total, $this->calculator->sum($items));
}

Fix: Use data providers or inline expected values.

public function test_calculates_total(): void
{
    $items = [10, 20, 30];

    self::assertEquals(60, $this->calculator->sum($items));  // ✅
}

---

2. Mock Overuse

Problem: More than 3 mocks in a single test.

Detection:

Grep: "createMock\|createStub" --glob "tests/**/*Test.php" -C 20
# Count mocks per test method

Example (Bad):

public function test_process_order(): void
{
    $repository = $this->createMock(OrderRepository::class);      // 1
    $mailer = $this->createMock(MailerInterface::class);          // 2
    $logger = $this->createMock(LoggerInterface::class);          // 3
    $eventDispatcher = $this->createMock(EventDispatcher::class); // 4
    $validator = $this->createMock(ValidatorInterface::class);    // 5 ❌
    // ...
}

Fix: Use Fakes, refactor design, or split test.

public function test_process_order(): void
{
    $repository = new InMemoryOrderRepository();  // Fake
    $mailer = new CollectingMailer();             // Fake
    $eventDispatcher = new CollectingEventDispatcher(); // Fake
    // ...
}

---

3. Test Interdependence

Problem: Tests depend on execution order or shared state.

Detection:

Grep: "static \$" --glob "tests/**/*Test.php"
Grep: "self::\$[a-z]" --glob "tests/**/*Test.php"
# Check for @depends annotation
Grep: "@depends" --glob "tests/**/*Test.php"

Example (Bad):

private static array $createdUsers = [];  // ❌ Shared state

public function test_creates_user(): void
{
    $user = $this->service->create('[e-mail]');
    self::$createdUsers[] = $user;
}

public function test_finds_created_user(): void
{
    $user = $this->service->find(self::$createdUsers[0]->id);  // ❌ Depends on previous
}

Fix: Each test creates its own data.

public function test_finds_user(): void
{
    $user = UserMother::default();
    $this->repository->save($user);

    $found = $this->service->find($user->id);

    self::assertEquals($user->id, $found->id);
}

---

4. Fragile Test

Problem: Test breaks when implementation changes (not behavior).

Detection:

# Method call order verification
Grep: "expects\(.*exactly\|expects\(.*at\(" --glob "tests/**/*Test.php"
# Internal method testing
Grep: "->method\('_" --glob "tests/**/*Test.php"

Example (Bad):

$mock->expects($this->exactly(3))->method('process');  // ❌ Verifies HOW, not WHAT
$mock->expects($this->at(0))->method('first');
$mock->expects($this->at(1))->method('second');

Fix: Test outcomes, not call sequences.

$this->service->processAll($items);

self::assertCount(3, $this->repository->findProcessed());  // ✅ Verifies WHAT

---

5. Mystery Guest

Problem: Test uses external files or hidden data sources.

Detection:

Grep: "file_get_contents\|fopen\|include\|require" --glob "tests/**/*Test.php"
Grep: "getenv\|_ENV\|_SERVER" --glob "tests/**/*Test.php"

Example (Bad):

public function test_imports_products(): void
{
    $data = json_decode(file_get_contents('fixtures/products.json'));  // ❌ Hidden
    // Where does this file come from? What's in it?
}

Fix: Inline test data or use explicit builders.

public function test_imports_products(): void
{
    $data = [
        ['name' => 'Book', 'price' => 1000],
        ['name' => 'Pen', 'price' => 100],
    ];

    $this->importer->import($data);

    self::assertCount(2, $this->repository->findAll());
}

---

6. Eager Test

Problem: Single test verifies multiple unrelated behaviors.

Detection:

# Multiple assert groups with different subjects
Grep: "self::assert" --glob "tests/**/*Test.php" -C 5
# Count assertions per test method

Example (Bad):

public function test_user_operations(): void
{
    // Testing creation
    $user = $this->service->create('[e-mail]');
    self::assertNotNull($user);

    // Testing update (different behavior!)
    $user->setName('John');
    $this->service->update($user);
    self::assertEquals('John', $user->getName());

    // Testing deletion (another behavior!)
    $this->service->delete($user);
    self::assertNull($this->repository->find($user->id));
}

Fix: One test per behavior.

public function test_creates_user(): void { ... }
public function test_updates_user_name(): void { ... }
public function test_deletes_user(): void { ... }

---

7. Assertion Roulette

Problem: Multiple assertions without messages, unclear which failed.

Detection:

# Count assertions per test method
Grep: "self::assert" --glob "tests/**/*Test.php"
# More than 5 assertions without context

Example (Bad):

public function test_order_properties(): void
{
    self::assertEquals('pending', $order->status);
    self::assertEquals(100, $order->total);
    self::assertEquals(3, count($order->items));
    self::assertEquals('[e-mail]', $order->customer->email);
    self::assertEquals('2024-01-01', $order->createdAt->format('Y-m-d'));
    // Which one failed?
}

Fix: Group related assertions or add messages.

public function test_order_has_correct_status(): void
{
    self::assertEquals('pending', $order->status);
}

public function test_order_calculates_total(): void
{
    self::assertEquals(100, $order->total);
}

---

8. Obscure Test

Problem: Test purpose unclear from name or structure.

Detection:

# Generic test names
Grep: "test_it_works\|test_test\|test_foo" --glob "tests/**/*Test.php"
# Missing assertions
Grep: "function test_" --glob "tests/**/*Test.php" -A 10

Example (Bad):

public function test_it_works(): void  // ❌ What works?
{
    $x = $this->service->doSomething($this->data);
    self::assertTrue($x);
}

Fix: Descriptive name following convention.

public function test_calculate_total_with_discount_returns_reduced_price(): void
{
    // Clear intent
}

---

9. Test Code Duplication

Problem: Same setup/assertion code repeated across tests.

Detection:

# Repeated patterns across test methods
# Manual review or static analysis tools

Example (Bad):

public function test_confirm_order(): void
{
    $order = new Order(OrderId::generate(), CustomerId::generate());
    $order->addItem(new Product('Book', Money::EUR(100)));
    // ... same setup in 10 tests
}

Fix: Extract to setUp, Builder, or Mother.

protected function setUp(): void
{
    $this->order = OrderBuilder::anOrder()->withItem($this->book)->build();
}

---

10. Conditional Test Logic

Problem: Different assertions based on conditions.

Detection:

Grep: "if.*assert\|assert.*if" --glob "tests/**/*Test.php"

Example (Bad):

public function test_process(): void
{
    $result = $this->service->process($input);
    if ($result !== null) {  // ❌
        self::assertInstanceOf(Order::class, $result);
    } else {
        self::fail('Should not be null');
    }
}

Fix: Explicit assertions.

public function test_process_returns_order(): void
{
    $result = $this->service->process($input);

    self::assertNotNull($result);
    self::assertInstanceOf(Order::class, $result);
}

---

11. Hard-Coded Test Data

Problem: Magic values without meaning.

Detection:

Grep: "'[a-z0-9]{8,}'" --glob "tests/**/*Test.php"
Grep: "12345\|999\|100" --glob "tests/**/*Test.php"

Example (Bad):

$order = new Order('550e8400-e29b-41d4-a716-446655440000');  // ❌ Magic UUID
$money = Money::EUR(12345);  // ❌ Magic number

Fix: Named constants or builders.

private const KNOWN_ORDER_ID = 'order-123';

$order = OrderBuilder::anOrder()
    ->withId(OrderId::fromString(self::KNOWN_ORDER_ID))
    ->withTotal(Money::EUR(100))  // Meaningful amount
    ->build();

---

12. Testing Private Methods

Problem: Tests access private/protected methods directly.

Detection:

Grep: "setAccessible\(true\)" --glob "tests/**/*Test.php"
Grep: "ReflectionMethod\|ReflectionProperty" --glob "tests/**/*Test.php"

Example (Bad):

$method = new ReflectionMethod(Order::class, 'calculateDiscount');
$method->setAccessible(true);
$result = $method->invoke($order, $amount);  // ❌ Testing internals

Fix: Test through public API.

$order->applyDiscount($coupon);

self::assertEquals($expectedTotal, $order->total());  // ✅ Public API

---

13. Slow Test

Problem: Unit test takes >100ms.

Detection:

# Run PHPUnit with timing
phpunit --log-junit timing.xml
# Check for sleep, HTTP calls, file I/O
Grep: "sleep\|usleep\|file_\|curl_" --glob "tests/Unit/**/*Test.php"

Fix: Mock external dependencies, use in-memory implementations.

---

14. Mocking Final Classes

Problem: Attempting to mock final classes.

Detection:

# Find final classes
Grep: "final class" --glob "src/**/*.php"
# Cross-reference with mocks in tests
Grep: "createMock\(.*::" --glob "tests/**/*Test.php"

Fix: Mock interfaces, not implementations.

// Bad: $mock = $this->createMock(FinalService::class);
// Good:
$mock = $this->createMock(ServiceInterface::class);

---

15. Mocking Value Objects

Problem: Mocking immutable value objects.

Detection:

# Find readonly classes (likely VOs)
Grep: "readonly class" --glob "src/**/*.php"
# Check if they're mocked

Fix: Use real value objects — they're simple and deterministic.

// Bad: $email = $this->createMock(Email::class);
// Good:
$email = new Email('[e-mail]');

Output Format

# Test Smell Report

## Summary

| Smell | Count | Severity |
|-------|-------|----------|
| Logic in Test | 5 | High |
| Mock Overuse | 3 | High |
| Mystery Guest | 2 | Medium |
| Eager Test | 8 | Medium |

## Findings

### Logic in Test (5 occurrences)

| File | Line | Code |
|------|------|------|
| OrderTest.php | 45 | `foreach ($items as $item)` |
| UserTest.php | 23 | `if ($result !== null)` |

**Recommendation:** Extract to data providers or inline values.

### Mock Overuse (3 occurrences)

| File | Test Method | Mock Count |
|------|-------------|------------|
| PaymentTest.php | test_process | 6 |
| OrderServiceTest.php | test_place | 5 |

**Recommendation:** Use Fakes (InMemory implementations) or split tests.

## Action Items

1. **High Priority**
   - Refactor `PaymentTest::test_process` — 6 mocks indicates design smell
   - Remove loops from `OrderTest` — use DataProvider

2. **Medium Priority**
   - Inline fixture data in `ImporterTest`
   - Split `UserTest::test_user_operations` into 3 tests

## Recommended Skills

| Smell | Fix With |
|-------|----------|
| Mock Overuse | `acc-create-mock-repository` |
| Mystery Guest | `acc-create-test-builder` |
| Test Duplication | `acc-create-test-builder` |

Severity Matrix

Smell	Severity	Impact
Logic in Test	High	Unreliable results
Mock Overuse	High	Design problem indicator
Test Interdependence	High	Flaky tests
Fragile Test	High	Maintenance burden
Mocking Final/VO	High	Runtime errors
Mystery Guest	Medium	Hard to understand
Eager Test	Medium	Hard to diagnose failures
Slow Test	Medium	Development slowdown
Obscure Test	Low	Documentation issue
Hard-Coded Data	Low	Readability

Refactoring for Testability

See references/refactoring-patterns.md for detailed refactoring patterns:

• Extract Interface (Seam)
• Constructor Injection
• Replace Singleton with DI
• Break Temporal Coupling
• Extract Pure Function
• Replace new with Factory
• Testability Score Checklist
• Smell → Refactoring Matrix