# Test Quality: Fix Pest P1 Quirks (#62 + #72) Implementation Plan

> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.

**Goal:** Fix 4 failing Pest P1 tests — quirk #62 (3 sequential failures: `LookupsTest` + `ProjectExtensionsTest` see accumulated DB state) and quirk #72 (1 parallel failure: `CsvReconcileJobTest` Redis session race).

**Architecture:** Two independent fixes, two commits. **Quirk #62:** one-line addition to `app/composer.json` scripts — `@php artisan migrate:fresh --env=testing` before `@php artisan test`, ensuring `liderra_testing` starts clean on every sequential run. **Quirk #72:** narrow `Bus::fake([RouteSupplierLeadJob::class])` in `CsvReconcileJobTest.php` (was `Bus::fake()` — ALL jobs) + bind an anonymous `RefreshSupplierSessionJob` mock that calls `putSupplierSession()` in its `handle()` — makes the race-window DB recovery deterministic instead of silently swallowing the session-null exception.

**Tech Stack:** PHP 8.3 + Laravel 13, Pest 4, Redis (Memurai localhost:6379), PostgreSQL 16 (`liderra_testing` DB), `DatabaseTransactions` trait (all Feature tests).

**Reference:** Audit #2 findings Phase 2 P1-01 (quirk #62) + P1-02 (quirk #72). Root-cause notes in `memory/feedback_environment.md` quirks #62 and #72.

---

## Files

- **Modify:** `app/composer.json` — `scripts.test` array, add `migrate:fresh` step
- **Modify:** `app/tests/Feature/Supplier/CsvReconcileJobTest.php` — `beforeEach` block + use-import

---

## Root Cause Summary (read before touching code)

### Quirk #62 — stale DB across `composer test` sessions

`composer test` runs `php artisan test` (sequential) with **no** `migrate:fresh`. `DatabaseTransactions` rolls back within each test but not if a session crashes mid-test or leaves orphaned data. After multiple sessions, tables like `users`, `projects` accumulate rows that persist between runs.

`LookupsTest.php:31` expects `2` active managers for a fresh tenant but reads 1465 (prior runs). `ProjectExtensionsTest.php:89` expects `Project::activeOnDay(1)->count() === 1` but RLS sees rows from prior tenants whose context leaked via `SET app.current_tenant_id` (not `SET LOCAL`).

**Fix:** add `@php artisan migrate:fresh --env=testing @no_additional_args` before `@php artisan test` in `composer.json`. This is identical in spirit to what `composer test:parallel` already does (`--recreate-databases`). Adds ~3s per run — acceptable.

### Quirk #72 — Redis session wiped between `putSupplierSession()` and SUT read

In `--parallel` mode, all workers share Redis DB+prefix. `CsvReconcileJobTest::beforeEach` calls `putSupplierSession()` (sets `supplier:session`). But `SyncSupplierProjectsJobTest::afterEach:36`, `CleanupInactiveSupplierProjectsJobTest::afterEach:30`, and `SupplierSessionRefreshCommandTest::afterEach:18` all call `Cache::store('redis')->forget('supplier:session')`. A concurrent `afterEach` can wipe the key between our `put` and the SUT's `loadSession()` read.

When the key is missing, `SupplierPortalClient::loadSession()` calls `dispatch_sync(app(RefreshSupplierSessionJob::class))`. Under current `Bus::fake()` (all jobs), the `dispatch_sync` is silently swallowed — no `handle()` called, session stays null → `SupplierAuthException("Session refresh failed: cache still empty")`.

**Fix:** switch to `Bus::fake([RouteSupplierLeadJob::class])` (only fake the job we assert on) + bind an anonymous mock for `RefreshSupplierSessionJob` that calls `putSupplierSession()` in `handle()`. Now if the race fires, `dispatch_sync($mock)` is NOT intercepted by the partial fake, the real dispatcher calls `$mock->handle()` → session re-seeded → `loadSession()` succeeds.

---

## Task A: Fix quirk #62 — `migrate:fresh` in `composer test`

**Files:**

- Modify: `app/composer.json` (lines 58–61)

- [ ] **A.1: Apply the fix**

In `app/composer.json`, change:

```json
"test": [
    "@php artisan config:clear --ansi @no_additional_args",
    "@php artisan test"
],
```

to:

```json
"test": [
    "@php artisan config:clear --ansi @no_additional_args",
    "@php artisan migrate:fresh --env=testing @no_additional_args",
    "@php artisan test"
],
```

- [ ] **A.2: Run `composer test` — first pass**

Run (cwd = `app/`):

```bash
cd "c:/моя/проекты/портал crm/Документация/app" && composer test
```

Expected: migration output (`Dropped all tables...`, `Running migrations...`, `0 errors`) then test run `742 passed` (0 failed).

If failures appear: read the failure output carefully — they must be unrelated to quirk #62 (which would be `LookupsTest` / `ProjectExtensionsTest`). Fix before continuing.

- [ ] **A.3: Run `composer test` — second pass (regression guard)**

Run immediately after A.2 with no other DB changes:

```bash
cd "c:/моя/проекты/портал crm/Документация/app" && composer test
```

Expected: same `742 passed` — proves `migrate:fresh` prevents accumulation on repeated runs.

- [ ] **A.4: Commit**

```bash
git add "c:/моя/проекты/портал crm/Документация/app/composer.json"
git commit -m "$(cat <<'EOF'
fix(tests): add migrate:fresh to composer test — close quirk #62

Sequential `composer test` had no DB reset between sessions; data accumulated
in liderra_testing across runs, causing LookupsTest (expected 2 managers, got
1465) and ProjectExtensionsTest (activeOnDay count inflated) to fail.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
EOF
)"
```

Expected: lefthook pre-commit passes (gitleaks 0 / markdownlint 0 / cspell 0). Show full pre-commit output.

---

## Task B: Fix quirk #72 — partial `Bus::fake` + session mock in `CsvReconcileJobTest`

**Files:**

- Modify: `app/tests/Feature/Supplier/CsvReconcileJobTest.php` (lines 1–57)

- [ ] **B.1: Add the use-import for `RefreshSupplierSessionJob`**

Current import block (lines 1–22) ends at:

```php
use Tests\Concerns\SharesSupplierPdo;
```

Add one line after it:

```php
use App\Jobs\Supplier\RefreshSupplierSessionJob;
```

File head after edit:

```php
<?php

declare(strict_types=1);

use App\Exceptions\Supplier\SupplierTransientException;
use App\Jobs\RouteSupplierLeadJob;
use App\Jobs\Supplier\CsvReconcileJob;
use App\Jobs\Supplier\RefreshSupplierSessionJob;
use App\Mail\CsvDriftAlertMail;
use App\Models\SupplierLead;
use App\Models\SupplierProject;
use App\Services\Supplier\SupplierCsvParser;
use App\Services\Supplier\SupplierPortalClient;
use Illuminate\Console\Scheduling\Schedule;
use Illuminate\Contracts\Mail\Mailer;
use Illuminate\Foundation\Testing\DatabaseTransactions;
use Illuminate\Support\Facades\Bus;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Mail;
use Tests\Concerns\SharesSupplierPdo;
```

- [ ] **B.2: Replace `beforeEach` block**

Current `beforeEach` (lines 48–57):

```php
beforeEach(function () {
    Mail::fake();
    Bus::fake();
    // NB: NOT Cache::store('redis')->flush() — flush wipes session keys belonging to
    // OTHER parallel tests (cross-pollution). Just forget our reserved keys + re-put.
    Cache::store('redis')->forget('supplier:csv_reconcile');
    putSupplierSession();
    config(['services.supplier.portal_url' => 'https://crm.bp-gr.ru']);
    config(['services.supplier.alert_email' => 'ops@liderra.ru']);
});
```

Replace with:

```php
beforeEach(function () {
    Mail::fake();
    // Partial fake: only RouteSupplierLeadJob is intercepted (what we assert on).
    // RefreshSupplierSessionJob must NOT be faked — it must run our mock below
    // so that loadSession() can recover if a concurrent afterEach wipes the session.
    Bus::fake([RouteSupplierLeadJob::class]);
    // Bind a mock that re-puts the session when dispatch_sync triggers it during a race.
    // handle() has no params — no DI injection needed.
    app()->bind(RefreshSupplierSessionJob::class, fn () => new class {
        public function handle(): void
        {
            putSupplierSession();
        }
    });
    // NB: NOT Cache::store('redis')->flush() — flush wipes session keys belonging to
    // OTHER parallel tests (cross-pollution). Just forget our reserved keys + re-put.
    Cache::store('redis')->forget('supplier:csv_reconcile');
    putSupplierSession();
    config(['services.supplier.portal_url' => 'https://crm.bp-gr.ru']);
    config(['services.supplier.alert_email' => 'ops@liderra.ru']);
});
```

- [ ] **B.3: Verify assertions are still correct**

The two assertions in the file must still work:

- Line 116 (test "matches existing leads"): `Bus::assertNothingDispatched();`
  With `Bus::fake([RouteSupplierLeadJob::class])`, `assertNothingDispatched()` checks only faked jobs (RouteSupplierLeadJob). If no drift → no RouteJob dispatched → ✅.

- Line 157 (test "drift 10%"): `Bus::assertDispatched(RouteSupplierLeadJob::class, 1);`
  RouteSupplierLeadJob IS faked → intercepted + recorded → `assertDispatched` passes ✅.

No changes needed to assertions. If you see `BusFake::assertNothingDispatched()` failing, it means a `RouteSupplierLeadJob` was unexpectedly dispatched — investigate the SUT, not the test setup.

- [ ] **B.4: Run `composer test:parallel` — first pass**

Run (cwd = project root Документация/):

```bash
cd "c:/моя/проекты/портал crm/Документация" && composer --working-dir=app test:parallel
```

Expected: `742 passed, 3 skipped` (0 failed). The 3 skipped are the pre-existing `@skip` marks unrelated to this fix.

If `CsvReconcileJobTest` still fails: run in isolation to diagnose:

```bash
cd "c:/моя/проекты/портал crm/Документация/app" && php vendor/bin/pest tests/Feature/Supplier/CsvReconcileJobTest.php --parallel -v
```

Check: (1) Is the session key wiped? (2) Is `RefreshSupplierSessionJob::handle()` being called? Add temporary `\Log::debug('mock handle called');` and check `storage/logs/laravel.log`.

- [ ] **B.5: Run `composer test:parallel` — two more times (race guard)**

Run twice more in quick succession:

```bash
cd "c:/моя/проекты/портал crm/Документация/app" && composer test:parallel && composer test:parallel
```

Expected: both pass `742/739/3sk`. If either fails on `CsvReconcileJobTest`, the race window is still open — escalate to user with timing details.

- [ ] **B.6: Commit**

```bash
git add "c:/моя/проекты/портал crm/Документация/app/tests/Feature/Supplier/CsvReconcileJobTest.php"
git commit -m "$(cat <<'EOF'
fix(tests): Bus::fake partial + session mock — close quirk #72

CsvReconcileJobTest used Bus::fake() (all jobs), silencing dispatch_sync of
RefreshSupplierSessionJob when a parallel afterEach wiped supplier:session.
Now: Bus::fake([RouteSupplierLeadJob::class]) + anonymous mock that re-puts
the session in handle(), making race-window recovery deterministic.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
EOF
)"
```

Expected: lefthook pre-commit passes. Show full output.

---

## Task C: Pre-push + Push

- [ ] **C.1: Run lefthook pre-push**

```bash
cd "c:/моя/проекты/портал crm/Документация" && npx lefthook run pre-push
```

Expected: gitleaks 0 leaks / lychee 0 broken. Show full output.

If gitleaks fails → inspect with `./bin/gitleaks.exe detect` to identify the offending commit. Do NOT bypass with `--no-verify`.

If lychee fails → fix the broken link in the commit that introduced it, make a new fixup commit.

- [ ] **C.2: Push**

```bash
git push origin main
```

Expected: exit 0. Show full output.

- [ ] **C.3: Verify remote HEAD**

```bash
git log origin/main --oneline -5
```

Expected: top 2 commits are Task A commit (`fix(tests): add migrate:fresh`) and Task B commit (`fix(tests): Bus::fake partial`).

---

## Self-Review

| Requirement | Coverage |
|---|---|
| Quirk #62: 3 sequential P1 tests fixed | Task A — `migrate:fresh` added to `composer test` ✅ |
| Quirk #72: 1 parallel P1 test fixed | Task B — partial fake + session mock ✅ |
| `composer test` passes (first + second run) | A.2 + A.3 ✅ |
| `composer test:parallel` passes (×3 runs) | B.4 + B.5 ✅ |
| `Bus::assertNothingDispatched()` still works | B.3 analysis ✅ |
| `Bus::assertDispatched(RouteSupplierLeadJob)` still works | B.3 analysis ✅ |
| No `Bus::fake()` silently hiding `RefreshSupplierSessionJob` | B.2 partial fake ✅ |
| pre-commit clean | A.4 + B.6 (show output) ✅ |
| pre-push clean | C.1 (show output) ✅ |
| No `--no-verify` / hook bypass | rule enforced throughout ✅ |
| No PD / tokens in commits | composer.json + test file — no sensitive data ✅ |
| Two atomic commits (one per quirk) | A.4 + B.6 ✅ |

**Placeholder scan:** no TBD/TODO/«implement later». All steps contain concrete code.

**Type consistency:** `RefreshSupplierSessionJob` import added at B.1 before use at B.2. `RouteSupplierLeadJob` already imported at line 6. Anonymous mock `handle(): void` matches `RefreshSupplierSessionJob::handle(PlaywrightBridge $bridge): void` signature — DI injection not triggered because `handle()` has no params in mock.
