mitgliederverwaltung/docs/test-optimization-policy-tests-analysis.md

868 lines
30 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

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