integreat/todos/008-pending-p1-refactor-tests-behavior-not-implementation.md

---
status: pending
priority: p1
issue_id: "008"
tags: [testing, architecture, refactoring, vendors, best-practices]
dependencies: []
---

# Refactor Tests to Focus on Behavior vs Implementation Details

## Problem Statement

The vendors test file tests internal implementation functions (`fetch-ids`, `hydrate-results`) rather than the public interface (`fetch-page`). This creates:
1. Brittle tests that break during refactoring
2. Tight coupling to internal structure
3. Violation of "Test user-observable behavior" principle

## Findings

**From architecture-strategist and kieran-rails-reviewer:**

**Tests exercising internal functions (lines 47, 80, 170):**
```clojure
;; Testing internal implementation
(sut/fetch-ids db {:query-params ...})      ; Internal query helper
(sut/hydrate-results [vendor-id] db {})     ; Internal hydration helper
```

**Problem:** These are intermediate functions used by `fetch-page`. Testing them directly:
- Couples tests to implementation details
- Makes refactoring harder (changing internal structure breaks tests)
- Violates testing conventions from AGENTS.md

**accounts_test.clj pattern - tests public interface:**
```clojure
;; Tests public behavior through account-save and fetch-page
(let [result (sut/account-save {:form-params ...})]
  (is (= 200 (:status result))))

(let [[accounts matching-count] (sut/fetch-page {:query-params ...})]
  (is (number? matching-count)))
```

## Proposed Solutions

### Option A: Remove Implementation Detail Tests (Recommended)
**Effort:** Small (15 minutes)
**Risk:** Low

Remove or refactor tests that directly test `fetch-ids` and `hydrate-results`:

1. **Remove:** `vendor-fetch-ids-returns-correct-structure` (lines 40-53)
   - Already covered by `vendor-fetch-page-returns-vendors`
   - Tests same functionality through public interface

2. **Remove:** `vendor-hydrate-results-works` (lines 71-86)
   - Merge into `vendor-fetch-page-returns-vendors`
   - Test hydration through `fetch-page` output

3. **Keep:** `vendor-hydration-includes-all-fields` (lines 158-177)
   - Rename to `vendor-fetch-page-returns-complete-vendor-data`
   - Test through `fetch-page` instead of `hydrate-results`

**Pros:**
- Follows accounts_test.clj pattern
- Tests are more resilient to refactoring
- Focuses on user-observable behavior
- Reduces test file size (~40 lines)

**Cons:**
- Slightly less granular test coverage
- May miss edge cases in internal functions

### Option B: Make Functions Private
**Effort:** Small (10 minutes)
**Risk:** Medium

Change `fetch-ids` and `hydrate-results` to private (`defn-`):
```clojure
(defn- fetch-ids [db request] ...)
(defn- hydrate-results [ids db _] ...)
```

Then test through `fetch-page`:
```clojure
(deftest vendor-fetch-page-works
  (let [[vendors count] (sut/fetch-page request)]
    ;; This implicitly tests fetch-ids and hydrate-results
    ...))
```

**Pros:**
- Clearly signals these are implementation details
- Forces tests to use public interface

**Cons:**
- Requires modifying source file (vendors.clj)
- May break other code using these functions
- Risk of breaking changes

### Option C: Extract to Query Namespace
**Effort:** Medium (1 hour)
**Risk:** Medium

Create `auto-ap.queries.vendor` namespace:
```clojure
(ns auto-ap.queries.vendor
  (:require [datomic.api :as dc]))

(defn fetch-ids [db query-params] ...)
(defn hydrate-results [ids db] ...)
```

Test in separate file with explicit contract tests.

**Pros:**
- Clear separation of concerns
- Query functions become public API of new namespace
- Follows single responsibility principle

**Cons:**
- Significant refactoring
- Requires updating all callers
- Overkill for current scope

## Recommended Action

**Go with Option A** - remove implementation detail tests and focus on behavior:

1. Delete `vendor-fetch-ids-returns-correct-structure` (covered by fetch-page test)
2. Delete `vendor-hydrate-results-works` (merge into fetch-page test)
3. Refactor `vendor-hydration-includes-all-fields` to use `fetch-page`

This aligns with:
- accounts_test.clj pattern
- testing-conventions skill guidelines
- "Test behavior, not implementation" principle

## Technical Details

**Affected Files:**
- `test/clj/auto_ap/ssr/admin/vendors_test.clj`

**Tests to Remove/Refactor:**
- Lines 40-53: `vendor-fetch-ids-returns-correct-structure` → REMOVE
- Lines 71-86: `vendor-hydrate-results-works` → REMOVE (merge assertions into fetch-page test)
- Lines 158-177: `vendor-hydration-includes-all-fields` → REFACTOR to use fetch-page

**Verification:**
```bash
lein test auto-ap.ssr.admin.vendors-test  # All tests pass
lein cljfmt check                         # Formatting correct
```

## Acceptance Criteria

- [ ] `vendor-fetch-ids-returns-correct-structure` removed
- [ ] `vendor-hydrate-results-works` removed (assertions merged)
- [ ] `vendor-hydration-includes-all-fields` refactored to test through `fetch-page`
- [ ] Remaining tests focus on public interface (`fetch-page`, `merge-submit`)
- [ ] Tests still pass
- [ ] Code formatted with lein cljfmt

## Work Log

### 2026-02-07 - Initial Creation

**By:** Architecture Strategist Agent

**Actions:**
- Identified implementation detail testing pattern
- Compared with accounts_test.clj public interface approach
- Created todo with multiple solution options

**Learnings:**
- testing-conventions skill emphasizes testing behavior not implementation
- accounts_test.clj only tests public functions (account-save, fetch-page)
- Internal function testing creates maintenance burden