acc-detect-test-smells

# 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:**
```bash
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):**
```php
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.
```php
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:**
```bash
Grep: "createMock\|createStub" --glob "tests/**/*Test.php" -C 20
# Count mocks per test method
```

**Example (Bad):**
```php
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.
```php
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:**
```bash
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):**
```php
private static array $createdUsers = [];  // ❌ Shared state

public function test_creates_user(): void
{
    $user = $this->service->create('john@example.com');
    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.
```php
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:**
```bash
# Method call order verification
Grep: "expects\(.*exactly\|expects\(.*at\(" --glob "tests/**/*Test.php"
# Internal method testing
Grep: "->method\('_" --glob "tests/**/*Test.php"
```

**Example (Bad):**
```php
$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.
```php
$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:**
```bash
Grep: "file_get_contents\|fopen\|include\|require" --glob "tests/**/*Test.php"
Grep: "getenv\|_ENV\|_SERVER" --glob "tests/**/*Test.php"
```

**Example (Bad):**
```php
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.
```php
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:**
```bash
# Multiple assert groups with different subjects
Grep: "self::assert" --glob "tests/**/*Test.php" -C 5
# Count assertions per test method
```

**Example (Bad):**
```php
public function test_user_operations(): void
{
    // Testing creation
    $user = $this->service->create('john@example.com');
    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.
```php
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:**
```bash
# Count assertions per test method
Grep: "self::assert" --glob "tests/**/*Test.php"
# More than 5 assertions without context
```

**Example (Bad):**
```php
public function test_order_properties(): void
{
    self::assertEquals('pending', $order->status);
    self::assertEquals(100, $order->total);
    self::assertEquals(3, count($order->items));
    self::assertEquals('john@example.com', $order->customer->email);
    self::assertEquals('2024-01-01', $order->createdAt->format('Y-m-d'));
    // Which one failed?
}
```

**Fix:** Group related assertions or add messages.
```php
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:**
```bash
# 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):**
```php
public function test_it_works(): void  // ❌ What works?
{
    $x = $this->service->doSomething($this->data);
    self::assertTrue($x);
}
```

**Fix:** Descriptive name following convention.
```php
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:**
```bash
# Repeated patterns across test methods
# Manual review or static analysis tools
```

**Example (Bad):**
```php
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.
```php
protected function setUp(): void
{
    $this->order = OrderBuilder::anOrder()->withItem($this->book)->build();
}
```

---

### 10. Conditional Test Logic

**Problem:** Different assertions based on conditions.

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

**Example (Bad):**
```php
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.
```php
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:**
```bash
Grep: "'[a-z0-9]{8,}'" --glob "tests/**/*Test.php"
Grep: "12345\|999\|100" --glob "tests/**/*Test.php"
```

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

**Fix:** Named constants or builders.
```php
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:**
```bash
Grep: "setAccessible\(true\)" --glob "tests/**/*Test.php"
Grep: "ReflectionMethod\|ReflectionProperty" --glob "tests/**/*Test.php"
```

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

**Fix:** Test through public API.
```php
$order->applyDiscount($coupon);

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

---

### 13. Slow Test

**Problem:** Unit test takes >100ms.

**Detection:**
```bash
# 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:**
```bash
# 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.
```php
// Bad: $mock = $this->createMock(FinalService::class);
// Good:
$mock = $this->createMock(ServiceInterface::class);
```

---

### 15. Mocking Value Objects

**Problem:** Mocking immutable value objects.

**Detection:**
```bash
# 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.
```php
// Bad: $email = $this->createMock(Email::class);
// Good:
$email = new Email('test@example.com');
```

## Output Format

```markdown
# 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