local-it/mitgliederverwaltung
2
3
Fork 0
Code Issues 67 Pull requests 6 Projects 3 Releases Packages 1 Activity Actions

docs: add performance analysis on policy tests

Browse source
This commit is contained in:
Simon 2026-01-28 14:01:41 +01:00
parent 91f8bb03bc
commit eb2b2436be
Signed by: simon
GPG key ID: 40E7A58C4AA1EDB2
1 changed files with 868 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

868
docs/test-optimization-policy-tests-analysis.md Normal file
View file

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