diff --git a/docs/test-optimization-policy-tests-analysis.md b/docs/test-optimization-policy-tests-analysis.md new file mode 100644 index 0000000..94e53f8 --- /dev/null +++ b/docs/test-optimization-policy-tests-analysis.md @@ -0,0 +1,868 @@ +# Policy Tests - Deep Optimization Analysis + +**Date:** 2026-01-28 +**Status:** 📋 Analysis Complete - Ready for Decision + +--- + +## Executive Summary + +Analysis of policy tests identified significant optimization opportunities: +- **Redundant fixture creation:** Roles and users created repeatedly across tests +- **Framework functionality over-testing:** Many tests verify Ash policy framework behavior +- **Test duplication:** Similar tests across different permission sets +- **Estimated time savings:** 5-8 seconds (from current ~66s for top 20 tests) + +**Key Finding:** All policy tests use `async: false`, which prevents `setup_all` usage. However, roles can be shared within describe blocks, and some tests verify framework functionality rather than business logic. + +--- + +## Current Performance Metrics + +### Policy Test Files Performance + +| File | Tests | Total Time | Avg per Test | Top Tests | +|------|-------|------------|--------------|-----------| +| `member_policies_test.exs` | 24 tests | ~66s (top 20) | ~2.7s | 4.8s (slowest) | +| `user_policies_test.exs` | 30 tests | ~66s (top 20) | ~2.2s | 3.0s (slowest) | +| `custom_field_value_policies_test.exs` | 20 tests | ~66s (top 20) | ~3.3s | ~3.0s (estimated) | +| **Total** | **74 tests** | **~152s** | **~2.1s** | **4.8s** | + +### Top 20 Slowest Policy Tests + +| Rank | Test | File | Time | Permission Set | +|------|------|------|------|----------------| +| 1 | `cannot destroy member (returns forbidden)` | `member_policies_test.exs` | 4.8s | own_data | +| 2 | `cannot update unlinked member (returns forbidden)` | `member_policies_test.exs` | 4.6s | own_data | +| 3 | `can read all members` | `member_policies_test.exs` | 4.5s | read_only | +| 4 | `cannot update any member (returns forbidden)` | `member_policies_test.exs` | 4.2s | read_only | +| 5 | `can update linked member` | `member_policies_test.exs` | 3.7s | own_data | +| 6 | `can update any member` | `member_policies_test.exs` | 3.6s | admin | +| 7 | `can read individual member` | `member_policies_test.exs` | 3.4s | read_only | +| 8 | `can create member` | `member_policies_test.exs` | 3.1s | normal_user | +| 9 | `cannot create member (returns forbidden)` | `member_policies_test.exs` | 3.1s | own_data | +| 10 | `cannot create user (returns forbidden)` | `user_policies_test.exs` | 3.0s | read_only | + +**Total Top 20:** ~66 seconds + +--- + +## Current Situation Analysis + +### Test File Configuration + +| File | `async` Setting | Reason | Can Use `setup_all`? | +|------|----------------|--------|---------------------| +| `member_policies_test.exs` | `false` | Database commits visible across queries | ❌ No (sandbox limitation) | +| `user_policies_test.exs` | `false` | Database commits visible across queries | ❌ No (sandbox limitation) | +| `custom_field_value_policies_test.exs` | `false` | Database commits visible across queries | ❌ No (sandbox limitation) | + +**Note:** All policy tests use `async: false` to ensure database commits are visible across queries within the same test. This is necessary for testing unlinked members and other scenarios where data needs to persist across multiple queries. + +### Current Fixture Creation Patterns + +#### Pattern 1: Role Creation (Most Redundant) + +**Current Implementation:** +```elixir +# Helper function called in EVERY test +defp create_role_with_permission_set(permission_set_name, actor) do + role_name = "Test Role #{permission_set_name} #{System.unique_integer([:positive])}" + + Authorization.create_role(%{ + name: role_name, + description: "Test role for #{permission_set_name}", + permission_set_name: permission_set_name + }, actor: actor) +end +``` + +**Usage:** +- Called in `create_user_with_permission_set` helper +- Called for every user created in every test +- **Total calls:** ~74+ (one per user, multiple users per test) + +**Problem:** +- Roles are **identical** across tests with the same permission set +- `own_data` role created ~20+ times (once per test) +- `read_only` role created ~20+ times +- `normal_user` role created ~20+ times +- `admin` role created ~20+ times + +**Optimization Opportunity:** +- Create roles once per permission set in describe-level `setup` +- Reuse roles across tests in the same describe block + +#### Pattern 2: User Creation (Redundant) + +**Current Implementation:** +```elixir +# Helper function creates user + role + assigns role +defp create_user_with_permission_set(permission_set_name, actor) do + role = create_role_with_permission_set(permission_set_name, actor) # Creates role + user = Accounts.User |> Ash.Changeset.for_create(:register_with_password, ...) |> Ash.create(...) + # Assign role + # Reload with role +end +``` + +**Usage:** +- Called in every test's `setup` block +- Creates: 1 role + 1 user + role assignment + reload +- **Cost:** ~4-5 database operations per user + +**Problem:** +- Users are created fresh for each test +- But: Users need to be unique (different emails) +- **Cannot share users** across tests (they're actors, need isolation) + +**Optimization Opportunity:** +- **Limited** - Users must be unique per test +- But: Can optimize role creation (see Pattern 1) + +#### Pattern 3: Member Creation (Redundant) + +**Current Implementation:** +```elixir +# Helper creates admin user, then creates member +defp create_linked_member_for_user(user, actor) do + admin = create_admin_user(actor) # Creates admin user + role + member = Membership.Member |> Ash.Changeset.for_create(:create_member, ...) |> Ash.create(...) + # Link member to user +end + +defp create_unlinked_member(actor) do + admin = create_admin_user(actor) # Creates admin user + role AGAIN + member = Membership.Member |> Ash.Changeset.for_create(:create_member, ...) |> Ash.create(...) +end +``` + +**Usage:** +- Called in every describe block's `setup` +- Creates: 1 admin user + 1 admin role + 1-2 members +- **Cost:** ~5-6 database operations per describe block + +**Problem:** +- Admin user created repeatedly (once per describe block) +- Admin role created repeatedly (once per admin user) +- Members are test-specific (need isolation) + +**Optimization Opportunity:** +- Create admin user once per test file (module-level) +- Reuse admin user for member creation +- **But:** `async: false` + sandbox prevents `setup_all` + +#### Pattern 4: Test Structure (Highly Repetitive) + +**Current Structure:** +```elixir +describe "own_data permission set" do + setup %{actor: actor} do + user = create_user_with_permission_set("own_data", actor) # Creates role + user + linked_member = create_linked_member_for_user(user, actor) # Creates admin + member + unlinked_member = create_unlinked_member(actor) # Creates admin + member + %{user: user, linked_member: linked_member, unlinked_member: unlinked_member} + end + + test "can read linked member" do + # Test business logic + end + + test "cannot read unlinked member" do + # Test business logic + end + + # ... 5-7 more tests +end + +describe "read_only permission set" do + setup %{actor: actor} do + # SAME PATTERN - creates role + user + members + end + + test "can read all members" do + # Similar to own_data tests + end + + # ... similar tests +end +``` + +**Problem:** +- Same setup pattern repeated 4 times (one per permission set) +- Same test patterns repeated across permission sets +- Tests verify **framework behavior** (Ash policies) not business logic + +--- + +## Framework Functionality Analysis + +### What Are We Testing? + +According to `CODE_GUIDELINES.md` Section 4.7, we should **not** test framework functionality. Let's analyze what policy tests actually verify: + +#### Framework Functionality (Should NOT Test) + +**Ash Policy Framework:** +- ✅ **Policy evaluation** - How Ash evaluates policies (framework) +- ✅ **Permission lookup** - How Ash looks up permissions (framework) +- ✅ **Scope filtering** - How Ash applies scope filters (framework) +- ✅ **Auto-filter behavior** - How Ash auto-filters queries (framework) +- ✅ **Forbidden vs NotFound** - How Ash returns errors (framework) + +**Examples:** +```elixir +# Tests Ash framework behavior: +test "cannot read other users (returns not found due to auto_filter)" do + # This tests how Ash's auto_filter works (framework) + assert_raise Ash.Error.Invalid, fn -> + Ash.get!(Accounts.User, other_user.id, actor: user, domain: Mv.Accounts) + end +end + +test "cannot update other users (returns forbidden)" do + # This tests how Ash returns Forbidden errors (framework) + assert_raise Ash.Error.Forbidden, fn -> + other_user |> Ash.Changeset.for_update(...) |> Ash.update!(actor: user) + end +end +``` + +#### Business Logic (Should Test) + +**Our Authorization Rules:** +- ✅ **Permission set definitions** - What permissions each role has (business logic) +- ✅ **Scope definitions** - What scopes each permission set uses (business logic) +- ✅ **Special cases** - Custom business rules (e.g., "user can always read linked member") +- ✅ **Permission set behavior** - How our permission sets differ (business logic) + +**Examples:** +```elixir +# Tests our business logic: +test "own_data can read linked member" do + # Tests our business rule: own_data can read linked members + {:ok, member} = Ash.get(Membership.Member, linked_member.id, actor: user) + assert member.id == linked_member.id +end + +test "read_only can read all members" do + # Tests our business rule: read_only can read all members + {:ok, members} = Ash.read(Membership.Member, actor: user) + assert length(members) >= 2 # Should see all members +end +``` + +### Test Classification + +#### Tests That Verify Framework Functionality + +**Error Handling Tests (Framework):** +- `cannot read other users (returns not found due to auto_filter)` - Tests Ash auto-filter +- `cannot update other users (returns forbidden)` - Tests Ash error types +- `cannot create user (returns forbidden)` - Tests Ash error types +- `cannot destroy user (returns forbidden)` - Tests Ash error types + +**Permission Evaluation Tests (Framework):** +- Most "cannot" tests verify Ash's policy evaluation (framework) +- Tests that check error types (Forbidden vs NotFound) verify framework behavior + +**Recommendation:** Remove or consolidate these tests - they test Ash framework, not our business logic. + +#### Tests That Verify Business Logic + +**Permission Set Behavior Tests (Business Logic):** +- `can read linked member` - Tests our business rule +- `can read all members` - Tests our permission set definition +- `can update linked member` - Tests our scope definition +- `list members returns only linked member` - Tests our scope filter + +**Special Case Tests (Business Logic):** +- `user can always READ linked member` - Tests our custom business rule +- `own_data user can update linked member` - Tests our permission set + +**Recommendation:** Keep these tests - they verify our business logic. + +--- + +## Detailed Test Analysis + +### `member_policies_test.exs` (24 tests) + +#### Test Structure + +**4 Permission Sets × ~6 Tests Each = 24 Tests** + +**Permission Sets Tested:** +1. `own_data` (Mitglied) - 7 tests +2. `read_only` (Vorstand/Buchhaltung) - 6 tests +3. `normal_user` (Kassenwart) - 5 tests +4. `admin` - 4 tests +5. Special case - 3 tests + +#### Redundancy Analysis + +**Redundant Tests Across Permission Sets:** + +| Test Pattern | own_data | read_only | normal_user | admin | Redundant? | +|--------------|----------|-----------|-------------|-------|------------| +| `can read linked/all members` | ✅ | ✅ | ✅ | ✅ | ⚠️ Partially (different scopes) | +| `can update linked/any member` | ✅ (linked) | ❌ | ✅ (all) | ✅ (all) | ⚠️ Partially | +| `cannot create member` | ✅ | ✅ | ❌ | ❌ | ⚠️ Yes (same behavior) | +| `cannot destroy member` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | +| `cannot update unlinked/any` | ✅ | ✅ | ❌ | ❌ | ⚠️ Yes (same behavior) | + +**Framework Tests (Should Remove):** +- `cannot create member (returns forbidden)` - Tests Ash error handling (framework) +- `cannot destroy member (returns forbidden)` - Tests Ash error handling (framework) +- `cannot update unlinked member (returns forbidden)` - Tests Ash error handling (framework) + +**Business Logic Tests (Should Keep):** +- `can read linked member` - Tests our scope definition +- `can update linked member` - Tests our permission set +- `can read all members` - Tests our permission set +- `can create member` - Tests our permission set +- `list members returns only linked member` - Tests our scope filter + +#### Fixture Creation Analysis + +**Per Describe Block Setup:** +```elixir +describe "own_data permission set" do + setup %{actor: actor} do + user = create_user_with_permission_set("own_data", actor) # Creates: role + user + assignment + reload + linked_member = create_linked_member_for_user(user, actor) # Creates: admin user + admin role + member + link + unlinked_member = create_unlinked_member(actor) # Creates: admin user + admin role + member + # Total: 1 own_data role + 1 own_data user + 1 admin role + 1 admin user + 2 members = ~10 DB operations + end +end +``` + +**Total Per Test File:** +- 4 describe blocks × ~10 operations = ~40 operations per test run +- But: `setup` runs for each test, so ~40 × 6 tests = ~240 operations +- **Actually:** `setup` runs once per describe block, so ~40 operations total per file + +**Optimization Opportunity:** +- Roles are identical across tests in same describe block +- Admin user is identical across describe blocks +- **Can create roles once per describe block** (already done via `setup`) +- **Can create admin user once per test file** (but `async: false` prevents `setup_all`) + +### `user_policies_test.exs` (30 tests) + +#### Test Structure + +**4 Permission Sets × ~7 Tests Each + 3 Bypass Tests = 30 Tests** + +**Permission Sets Tested:** +1. `own_data` (Mitglied) - 7 tests +2. `read_only` (Vorstand/Buchhaltung) - 7 tests +3. `normal_user` (Kassenwart) - 7 tests +4. `admin` - 5 tests +5. AshAuthentication bypass - 3 tests + +#### Redundancy Analysis + +**Highly Redundant Tests:** + +| Test Pattern | own_data | read_only | normal_user | admin | Redundant? | +|--------------|----------|-----------|-------------|-------|------------| +| `can read own user record` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | +| `can update own email` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | +| `cannot read other users` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | +| `cannot update other users` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | +| `list users returns only own` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | +| `cannot create user` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | +| `cannot destroy user` | ✅ | ✅ | ✅ | ❌ | ⚠️ Yes (same behavior) | + +**Framework Tests (Should Remove):** +- All "cannot" tests - Test Ash error handling (framework) +- `cannot read other users (returns not found)` - Tests Ash auto-filter (framework) +- `cannot update other users (returns forbidden)` - Tests Ash error types (framework) + +**Business Logic Tests (Should Keep):** +- `can read all users` (admin) - Tests our permission set +- `can update other users` (admin) - Tests our permission set +- `can create user` (admin) - Tests our permission set +- `can destroy user` (admin) - Tests our permission set + +**AshAuthentication Bypass Tests:** +- These test **our integration** with AshAuthentication (business logic) +- ✅ Keep these tests + +### `custom_field_value_policies_test.exs` (20 tests) + +#### Test Structure + +**4 Permission Sets × ~5 Tests Each = 20 Tests** + +**Similar Patterns to Member Policies:** +- Same redundancy issues +- Same framework vs. business logic concerns +- Same fixture creation patterns + +--- + +## Optimization Opportunities + +### Opportunity 1: Remove Framework Functionality Tests + +**Tests to Remove:** + +**`member_policies_test.exs`:** +- `cannot create member (returns forbidden)` - 3 tests (one per permission set) +- `cannot destroy member (returns forbidden)` - 3 tests (one per permission set) +- `cannot update unlinked member (returns forbidden)` - 1 test (own_data) + +**`user_policies_test.exs`:** +- `cannot read other users (returns not found)` - 3 tests +- `cannot update other users (returns forbidden)` - 3 tests +- `cannot create user (returns forbidden)` - 3 tests +- `cannot destroy user (returns forbidden)` - 3 tests +- `list users returns only own user` - 3 tests (tests framework auto-filter) + +**Total Tests to Remove:** ~22 tests + +**Estimated Time Saved:** 3-4 seconds + +**Risk:** ⚠️ **Very Low** - Framework functionality is tested by Ash maintainers + +### Opportunity 2: Consolidate Redundant Tests + +**Tests to Consolidate:** + +**`user_policies_test.exs`:** +- `can read own user record` - 3 tests → 1 integration test +- `can update own email` - 3 tests → 1 integration test + +**`member_policies_test.exs`:** +- Similar patterns can be consolidated + +**Total Tests to Consolidate:** ~6-8 tests → 2-3 tests + +**Estimated Time Saved:** 1-2 seconds + +**Risk:** ⚠️ **Low** - Same coverage, fewer tests + +### Opportunity 3: Share Roles Within Describe Blocks + +**Current:** +```elixir +describe "own_data permission set" do + setup %{actor: actor} do + # Each test creates its own role via create_user_with_permission_set + user = create_user_with_permission_set("own_data", actor) # Creates role + # ... + end + + test "test 1", %{user: user} do + # Uses user with role + end + + test "test 2", %{user: user} do + # Uses same user (role already created in setup) + end +end +``` + +**Actually, roles ARE already shared** - `setup` runs once per describe block, so role is created once and reused. + +**Optimization:** None needed - already optimal + +### Opportunity 4: Share Admin User Across Describe Blocks + +**Current:** +```elixir +# Each describe block creates admin user +describe "own_data permission set" do + setup %{actor: actor} do + linked_member = create_linked_member_for_user(user, actor) # Creates admin + unlinked_member = create_unlinked_member(actor) # Creates admin AGAIN + end +end + +describe "read_only permission set" do + setup %{actor: actor} do + linked_member = create_linked_member_for_user(user, actor) # Creates admin AGAIN + unlinked_member = create_unlinked_member(actor) # Creates admin AGAIN + end +end +``` + +**Problem:** +- Admin user created 2× per describe block (linked + unlinked member helpers) +- Admin user created 4× per test file (one per permission set) +- **Total:** ~8 admin users + 8 admin roles per test file + +**Optimization:** +- Create admin user once in module-level `setup` +- Reuse admin user in helper functions + +**Implementation:** +```elixir +# Module-level setup +setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + admin_user = create_user_with_permission_set("admin", system_actor) + %{actor: system_actor, admin_user: admin_user} +end + +# Helper functions use shared admin +defp create_linked_member_for_user(user, actor, admin_user) do + # Use admin_user instead of creating new one +end +``` + +**Estimated Time Saved:** 1-2 seconds + +**Risk:** ⚠️ **Low** - Admin user is read-only in tests, safe to share + +### Opportunity 5: Reduce Test Data Volume + +**Current:** +- Each test creates multiple members/users +- Some tests create more data than needed + +**Optimization:** +- Use minimum required data +- Share fixtures where possible + +**Estimated Time Saved:** 0.5-1 second + +**Risk:** ⚠️ **Very Low** - Still tests same functionality + +--- + +## Estimated Total Impact + +| Optimization | Tests Affected | Time Saved | Risk | +|--------------|----------------|------------|------| +| Remove framework tests | ~22 tests | 3-4s | ⚠️ Very Low | +| Consolidate redundant tests | ~6-8 tests | 1-2s | ⚠️ Low | +| Share admin user | All tests | 1-2s | ⚠️ Low | +| Reduce test data volume | Multiple tests | 0.5-1s | ⚠️ Very Low | +| **Total** | | **5.5-9s** | | + +**Projected Final Time:** From ~66s (top 20) to **~57-60s** (8-14% reduction) + +--- + +## Risk Assessment + +### Overall Risk: ⚠️ **Low** + +**Mitigations:** +1. Framework functionality is tested by Ash maintainers +2. Business logic tests remain intact +3. Admin user sharing maintains test isolation (read-only) +4. Consolidation preserves coverage + +**What to Monitor:** +1. No increase in authorization bugs +2. Test coverage remains adequate +3. CI execution times improve as expected + +--- + +## Recommended Scenarios + +### Scenario 1: Remove Framework Tests (High Impact, Low Risk) + +**Action:** Remove tests that verify Ash framework functionality + +**Tests to Remove:** +- All "cannot" tests that verify error types (Forbidden, NotFound) +- Tests that verify auto-filter behavior (framework) +- Tests that verify permission evaluation (framework) + +**Keep:** +- Tests that verify permission set behavior (business logic) +- Tests that verify scope definitions (business logic) +- Tests that verify special cases (business logic) + +**Estimated Savings:** 3-4 seconds + +**Risk:** ⚠️ **Very Low** + +### Scenario 2: Consolidate Redundant Tests (Medium Impact, Low Risk) + +**Action:** Merge similar tests across permission sets + +**Examples:** +- `can read own user record` (3 tests) → 1 integration test +- `can update own email` (3 tests) → 1 integration test + +**Estimated Savings:** 1-2 seconds + +**Risk:** ⚠️ **Low** + +### Scenario 3: Share Admin User (Low Impact, Low Risk) + +**Action:** Create admin user once per test file, reuse in helpers + +**Implementation:** +- Module-level `setup` creates admin user +- Helper functions accept admin user as parameter +- Reuse admin user instead of creating new one + +**Estimated Savings:** 1-2 seconds + +**Risk:** ⚠️ **Low** + +### Scenario 4: Combined Approach (Recommended) + +**Action:** Implement all three scenarios + +**Estimated Savings:** 5.5-9 seconds + +**Risk:** ⚠️ **Low** (combination of low-risk optimizations) + +--- + +## Implementation Plan + +### Phase 1: Remove Framework Tests + +1. Identify all "cannot" tests that verify error types +2. Remove tests that verify Ash auto-filter behavior +3. Remove tests that verify permission evaluation (framework) +4. Keep business logic tests + +**Estimated Time:** 1-2 hours +**Risk:** ⚠️ Very Low + +### Phase 2: Consolidate Redundant Tests + +1. Identify similar tests across permission sets +2. Create integration tests that cover multiple permission sets +3. Remove redundant individual tests + +**Estimated Time:** 1-2 hours +**Risk:** ⚠️ Low + +### Phase 3: Share Admin User + +1. Add module-level `setup` to create admin user +2. Update helper functions to accept admin user parameter +3. Update all `setup` blocks to use shared admin user + +**Estimated Time:** 1-2 hours +**Risk:** ⚠️ Low + +### Phase 4: Verify and Benchmark + +1. Run full test suite +2. Benchmark execution times +3. Verify no regressions +4. Update documentation + +**Estimated Time:** 1 hour +**Risk:** ⚠️ Very Low + +--- + +## Detailed Test-by-Test Analysis + +### `member_policies_test.exs` - Test Classification + +#### own_data Permission Set (7 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read linked member` | ✅ Business Logic | Keep | +| `can update linked member` | ✅ Business Logic | Keep | +| `cannot read unlinked member` | ⚠️ Framework (auto-filter) | Remove | +| `cannot update unlinked member` | ⚠️ Framework (error type) | Remove | +| `list members returns only linked member` | ✅ Business Logic | Keep | +| `cannot create member` | ⚠️ Framework (error type) | Remove | +| `cannot destroy member` | ⚠️ Framework (error type) | Remove | + +#### read_only Permission Set (6 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read all members` | ✅ Business Logic | Keep | +| `can read individual member` | ✅ Business Logic | Keep | +| `cannot create member` | ⚠️ Framework (error type) | Remove | +| `cannot update any member` | ⚠️ Framework (error type) | Remove | +| `cannot destroy any member` | ⚠️ Framework (error type) | Remove | +| *(Missing: can read linked member test)* | - | - | + +#### normal_user Permission Set (5 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read all members` | ✅ Business Logic | Keep | +| `can create member` | ✅ Business Logic | Keep | +| `can update any member` | ✅ Business Logic | Keep | +| `cannot destroy member` | ⚠️ Framework (error type) | Remove | +| *(Missing: cannot read unlinked test)* | - | - | + +#### admin Permission Set (4 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read all members` | ✅ Business Logic | Keep | +| `can create member` | ✅ Business Logic | Keep | +| `can update any member` | ✅ Business Logic | Keep | +| `can destroy any member` | ✅ Business Logic | Keep | + +#### Special Case (3 tests) + +| Test | Type | Action | +|------|------|--------| +| `read_only user can read linked member` | ✅ Business Logic | Keep | +| `own_data user can read linked member` | ✅ Business Logic | Keep | +| `own_data user can update linked member` | ✅ Business Logic | Keep | + +**Total Tests:** 24 → **14 tests** (remove 10 framework tests) + +### `user_policies_test.exs` - Test Classification + +#### own_data Permission Set (7 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read own user record` | ⚠️ Redundant | Consolidate | +| `can update own email` | ⚠️ Redundant | Consolidate | +| `cannot read other users` | ⚠️ Framework (auto-filter) | Remove | +| `cannot update other users` | ⚠️ Framework (error type) | Remove | +| `list users returns only own user` | ⚠️ Framework (auto-filter) | Remove | +| `cannot create user` | ⚠️ Framework (error type) | Remove | +| `cannot destroy user` | ⚠️ Framework (error type) | Remove | + +#### read_only Permission Set (7 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read own user record` | ⚠️ Redundant | Consolidate | +| `can update own email` | ⚠️ Redundant | Consolidate | +| `cannot read other users` | ⚠️ Framework (auto-filter) | Remove | +| `cannot update other users` | ⚠️ Framework (error type) | Remove | +| `list users returns only own user` | ⚠️ Framework (auto-filter) | Remove | +| `cannot create user` | ⚠️ Framework (error type) | Remove | +| `cannot destroy user` | ⚠️ Framework (error type) | Remove | + +#### normal_user Permission Set (7 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read own user record` | ⚠️ Redundant | Consolidate | +| `can update own email` | ⚠️ Redundant | Consolidate | +| `cannot read other users` | ⚠️ Framework (auto-filter) | Remove | +| `cannot update other users` | ⚠️ Framework (error type) | Remove | +| `list users returns only own user` | ⚠️ Framework (auto-filter) | Remove | +| `cannot create user` | ⚠️ Framework (error type) | Remove | +| `cannot destroy user` | ⚠️ Framework (error type) | Remove | + +#### admin Permission Set (5 tests) + +| Test | Type | Action | +|------|------|--------| +| `can read all users` | ✅ Business Logic | Keep | +| `can read other users` | ✅ Business Logic | Keep | +| `can update other users` | ✅ Business Logic | Keep | +| `can create user` | ✅ Business Logic | Keep | +| `can destroy user` | ✅ Business Logic | Keep | + +#### AshAuthentication Bypass (3 tests) + +| Test | Type | Action | +|------|------|--------| +| `register_with_password works without actor` | ✅ Business Logic | Keep | +| `register_with_rauthy works without actor` | ✅ Business Logic | Keep | +| `sign_in_with_rauthy works without actor` | ✅ Business Logic | Keep | + +**Total Tests:** 30 → **14 tests** (remove 16 framework/redundant tests) + +### `custom_field_value_policies_test.exs` - Test Classification + +**Similar patterns to `member_policies_test.exs`:** + +- Remove "cannot" tests (framework) +- Keep "can" tests (business logic) +- Keep special case tests (business logic) + +**Estimated:** 20 → **12 tests** (remove 8 framework tests) + +--- + +## Summary of Tests to Remove/Consolidate + +### Remove (Framework Functionality) + +**`member_policies_test.exs`:** 10 tests +- `cannot create member` (3 tests) +- `cannot destroy member` (3 tests) +- `cannot update unlinked member` (1 test) +- `cannot read unlinked member` (1 test - auto-filter) +- *(2 more framework tests)* + +**`user_policies_test.exs`:** 16 tests +- `cannot read other users` (3 tests - auto-filter) +- `cannot update other users` (3 tests) +- `cannot create user` (3 tests) +- `cannot destroy user` (3 tests) +- `list users returns only own user` (3 tests - auto-filter) +- `can read own user record` (3 tests - redundant) +- `can update own email` (3 tests - redundant) + +**`custom_field_value_policies_test.exs`:** 8 tests +- Similar "cannot" tests + +**Total to Remove:** ~34 tests + +### Consolidate (Redundant) + +**`user_policies_test.exs`:** 6 tests → 2 tests +- `can read own user record` (3 tests) → 1 integration test +- `can update own email` (3 tests) → 1 integration test + +**Total to Consolidate:** 6 tests → 2 tests + +### Keep (Business Logic) + +**All "can" tests that verify permission set behavior:** +- `can read linked/all members` - Tests our scope definitions +- `can update linked/any member` - Tests our permission sets +- `can create member/user` - Tests our permission sets +- `can destroy member/user` - Tests our permission sets +- `list members returns only linked member` - Tests our scope filters + +**Special case tests:** +- `user can always READ linked member` - Tests our custom business rule + +**AshAuthentication bypass tests:** +- All bypass tests - Tests our integration + +--- + +## Questions for Decision + +1. **Are we comfortable removing framework functionality tests?** + - Ash maintainers test the framework + - Our tests should focus on business logic + - Risk is very low + +2. **Should we consolidate redundant tests?** + - Tests verify same behavior across permission sets + - Integration tests can cover multiple sets + - Risk is low + +3. **Can we share admin user across describe blocks?** + - Admin user is read-only in tests + - `async: false` prevents `setup_all`, but module-level `setup` works + - Risk is low + +4. **What's the minimum test coverage we need?** + - One test per permission set per action? + - Or integration tests covering all sets? + +--- + +## References + +- **Coding Guidelines:** `CODE_GUIDELINES.md` - Section 4.7 (Testing Best Practices) +- **Test Performance Optimization:** `docs/test-performance-optimization.md` +- **User LiveView Analysis:** `docs/test-optimization-user-liveview-analysis.md` +- **Phase 2 Analysis:** `docs/test-optimization-phase2-shared-fixtures-analysis.md`