diff --git a/docs/test-optimization-policy-tests-analysis.md b/docs/test-optimization-policy-tests-analysis.md deleted file mode 100644 index 94e53f8..0000000 --- a/docs/test-optimization-policy-tests-analysis.md +++ /dev/null @@ -1,868 +0,0 @@ -# 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` diff --git a/docs/test-optimization-user-liveview-analysis.md b/docs/test-optimization-user-liveview-analysis.md deleted file mode 100644 index 6955f62..0000000 --- a/docs/test-optimization-user-liveview-analysis.md +++ /dev/null @@ -1,524 +0,0 @@ -# User LiveView Tests - Optimization Analysis - -**Date:** 2026-01-28 -**Status:** 📋 Analysis Complete - Ready for Implementation - ---- - -## Executive Summary - -Analysis of User LiveView tests identified significant optimization opportunities: -- **Framework functionality over-testing:** ~15-20 tests test Phoenix/Ash core features -- **Redundant test data creation:** Each test creates users/members independently -- **Missing shared fixtures:** No `setup_all` usage for common data -- **Estimated time savings:** 15-20 seconds (from current ~35.5s for 10 tests in Top 20) - ---- - -## Current Performance Metrics - -### Top 20 Slowest Tests (User LiveView) - -| Rank | Test | File | Time | Category | -|------|------|------|------|----------| -| 3 | `test sorting functionality initially sorts by email ascending` | `index_test.exs` | 4.5s | Index | -| 5 | `test edit user form - updates admin sets new password for user` | `form_test.exs` | 4.2s | Form | -| 6 | `test edit user form - validation shows error for duplicate email` | `form_test.exs` | 4.0s | Form | -| 7 | `test mount and display displays linked member when present` | `show_test.exs` | 3.9s | Show | -| 8 | `test member linking - workflow selecting member and saving links member to user` | `form_test.exs` | 3.6s | Form | -| 10 | `test edit user form - validation shows error for invalid password` | `form_test.exs` | 3.5s | Form | -| 13 | `test mount and display displays password authentication status when enabled` | `show_test.exs` | 3.3s | Show | -| 14 | `test mount and display mounts successfully with valid user ID` | `show_test.exs` | 3.1s | Show | -| 15 | `test new user form - creation creates user with password when enabled` | `form_test.exs` | 3.0s | Form | -| 16 | `test edge cases handles very long email addresses` | `index_test.exs` | 2.9s | Index | -| 17 | `test checkbox selection functionality can select individual users` | `index_test.exs` | 2.8s | Index | -| 19 | `test checkbox selection functionality select all automatically checks when all individual users are selected` | `index_test.exs` | 2.7s | Index | - -**Total:** ~35.5 seconds for User LiveView tests in Top 20 - ---- - -## Analysis by Test File - -### 1. `test/mv_web/user_live/index_test.exs` - -**Total Tests:** 25 tests -**Performance Impact:** ~10.2s for 3 tests in Top 20 - -#### Issues Identified - -**1. Framework Functionality Over-Testing** - -**Navigation Tests (Lines 329-355):** -```elixir -test "clicking on user row navigates to user show page" -test "edit link points to correct edit page" -test "new user button points to correct new page" -``` -**Problem:** Tests Phoenix LiveView navigation (framework functionality) -**Recommendation:** Remove or consolidate into single integration test - -**Translation Tests (Lines 357-377):** -```elixir -test "shows German translations for selection" -test "shows English translations for selection" -``` -**Problem:** Tests Gettext (framework functionality) -**Recommendation:** Remove - translations are tested by framework - -**Basic Display Tests (Lines 6-47):** -```elixir -test "shows translated title in German" -test "shows translated title in English" -test "shows New User button" -test "displays users in a table" -test "shows correct action links" -``` -**Problem:** Tests basic HTML rendering (framework functionality) -**Recommendation:** Consolidate into single smoke test - -**2. Redundant Test Data Creation** - -**Sorting Tests (Lines 49-133):** -- Each test creates 3 users in `setup` block -- Users are identical across tests -- Could be shared in `setup_all` - -**Checkbox Tests (Lines 135-299):** -- Each test creates 2 users in `setup` block -- Users are identical across tests -- Could be shared in `setup_all` - -**3. Framework Sorting Logic Testing** - -**Sorting Tests:** -```elixir -test "initially sorts by email ascending" -test "can sort email descending by clicking sort button" -test "toggles back to ascending when clicking sort button twice" -test "shows sort direction icons" -``` -**Problem:** Tests `Enum.sort_by` (Elixir standard library) and basic UI state -**Recommendation:** Keep one integration test, remove others - -**4. Checkbox State Testing** - -**Checkbox Tests (Lines 135-299):** -- 7 tests testing checkbox state management -- Most test Phoenix LiveView event handling (framework functionality) -- Only business logic: "select all automatically checks when all individual users are selected" - -**Recommendation:** Keep business logic test, remove framework tests - -#### Optimization Opportunities - -1. **Consolidate Basic Display Tests:** 5 tests → 1 smoke test -2. **Remove Translation Tests:** 2 tests → 0 (framework functionality) -3. **Consolidate Navigation Tests:** 3 tests → 1 integration test -4. **Share Test Data:** Move user creation to `setup_all` for sorting/checkbox tests -5. **Reduce Sorting Tests:** 4 tests → 1 integration test -6. **Reduce Checkbox Tests:** 7 tests → 2 tests (business logic only) - -**Estimated Savings:** 5-7 seconds - ---- - -### 2. `test/mv_web/user_live/form_test.exs` - -**Total Tests:** 20 tests -**Performance Impact:** ~15.0s for 4 tests in Top 20 -**Note:** Uses `async: false` to prevent PostgreSQL deadlocks - -#### Issues Identified - -**1. Framework Validation Testing** - -**Validation Tests (Lines 122-153, 221-258):** -```elixir -test "shows error for duplicate email" -test "shows error for short password" -test "shows error for invalid password" -``` -**Problem:** Tests Ash validations (framework functionality) -**Recommendation:** Remove - validations are tested by Ash framework - -**2. Redundant Test Data Creation** - -**Form Display Tests:** -- Each test creates users/members independently -- `setup_live_view` creates new admin user each time -- Members created in each test that needs them - -**Recommendation:** Use `setup_all` for common fixtures - -**3. Framework Functionality Testing** - -**Internationalization Tests (Lines 261-292):** -```elixir -test "shows German labels" -test "shows English labels" -test "shows different labels for edit vs new" -``` -**Problem:** Tests Gettext (framework functionality) -**Recommendation:** Remove - translations are tested by framework - -**4. Password Storage Testing** - -**Password Storage Tests (Lines 92-119):** -```elixir -test "stores password when provided" -``` -**Problem:** Tests password hashing (AshAuthentication framework functionality) -**Recommendation:** Remove - password hashing is tested by AshAuthentication - -**5. Member Linking Tests** - -**Member Linking Tests (Lines 294-422):** -- Tests member linking workflow (business logic) ✅ Keep -- But creates members in each test -- Could share fixtures - -#### Optimization Opportunities - -1. **Remove Validation Tests:** 3 tests → 0 (framework functionality) -2. **Remove Translation Tests:** 3 tests → 0 (framework functionality) -3. **Remove Password Storage Test:** 1 test → 0 (framework functionality) -4. **Share Test Data:** Use `setup_all` for common users/members -5. **Consolidate Display Tests:** Multiple display tests → 1-2 smoke tests - -**Estimated Savings:** 6-8 seconds - ---- - -### 3. `test/mv_web/live/user_live/show_test.exs` - -**Total Tests:** 11 tests -**Performance Impact:** ~10.3s for 3 tests in Top 20 - -#### Issues Identified - -**1. Framework Functionality Testing** - -**Navigation Tests (Lines 105-133):** -```elixir -test "back button navigates to user list" -test "edit button navigates to edit form" -``` -**Problem:** Tests Phoenix LiveView navigation (framework functionality) -**Recommendation:** Remove or consolidate into single integration test - -**2. Basic Display Tests** - -**Display Tests (Lines 25-103):** -- Multiple tests for basic HTML rendering -- Some test framework functionality (Gettext) - -**Recommendation:** Consolidate into fewer tests - -**3. Redundant Test Data Creation** - -- Each test creates users/members independently -- `setup` creates one user, but many tests create additional users/members - -**Recommendation:** Use `setup_all` for common fixtures - -#### Optimization Opportunities - -1. **Consolidate Display Tests:** 5 tests → 2-3 tests -2. **Remove Navigation Tests:** 2 tests → 0 (framework functionality) -3. **Share Test Data:** Use `setup_all` for common users/members - -**Estimated Savings:** 3-4 seconds - ---- - -### 4. Additional Test Files - -#### `test/mv_web/user_live/form_member_selection_test.exs` -- **Tests:** 6 tests -- **Status:** ✅ Good - Tests business logic (member selection workflow) -- **Optimization:** Could share member fixtures in `setup_all` - -#### `test/mv_web/user_live/form_member_search_test.exs` -- **Tests:** 4 tests -- **Status:** ✅ Good - Tests business logic (fuzzy search) -- **Optimization:** Could share member fixtures in `setup_all` - ---- - -## Framework Functionality Over-Testing - -### Tests That Should Be Removed - -According to `CODE_GUIDELINES.md` Section 4.7, we should not test framework functionality: - -**Phoenix LiveView Framework:** -- Navigation (redirects, links) -- Basic HTML rendering -- Event handling (unless custom business logic) - -**Ash Framework:** -- Standard validations (duplicate email, password length) -- Standard CRUD operations -- Relationship loading (unless custom logic) - -**Gettext Framework:** -- Translation functionality - -**Elixir Standard Library:** -- `Enum.sort_by` sorting -- Basic list operations - -### Complete List of Tests to Remove/Consolidate - -| Test | File | Reason | Action | -|------|------|--------|--------| -| `shows translated title in German` | `index_test.exs` | Gettext framework | Remove | -| `shows translated title in English` | `index_test.exs` | Gettext framework | Remove | -| `shows New User button` | `index_test.exs` | Basic HTML rendering | Consolidate | -| `displays users in a table` | `index_test.exs` | Basic HTML rendering | Consolidate | -| `shows correct action links` | `index_test.exs` | Basic HTML rendering | Consolidate | -| `shows sort direction icons` | `index_test.exs` | UI state (framework) | Remove | -| `toggles back to ascending when clicking sort button twice` | `index_test.exs` | Framework sorting | Remove | -| `shows select all checkbox` | `index_test.exs` | Basic HTML rendering | Remove | -| `shows individual user checkboxes` | `index_test.exs` | Basic HTML rendering | Remove | -| `can select individual users` | `index_test.exs` | Framework event handling | Remove | -| `can deselect individual users` | `index_test.exs` | Framework event handling | Remove | -| `select all functionality selects all users` | `index_test.exs` | Framework event handling | Remove | -| `deselect all functionality deselects all users` | `index_test.exs` | Framework event handling | Remove | -| `clicking on user row navigates to user show page` | `index_test.exs` | Framework navigation | Remove | -| `edit link points to correct edit page` | `index_test.exs` | Framework navigation | Remove | -| `new user button points to correct new page` | `index_test.exs` | Framework navigation | Remove | -| `shows German translations for selection` | `index_test.exs` | Gettext framework | Remove | -| `shows English translations for selection` | `index_test.exs` | Gettext framework | Remove | -| `shows correct form elements` | `form_test.exs` | Basic HTML rendering | Consolidate | -| `hides password fields initially` | `form_test.exs` | UI state (framework) | Remove | -| `shows password fields when checkbox toggled` | `form_test.exs` | UI state (framework) | Remove | -| `shows error for duplicate email` | `form_test.exs` | Ash validation | Remove | -| `shows error for short password` | `form_test.exs` | Ash validation | Remove | -| `shows error for invalid password` | `form_test.exs` | Ash validation | Remove | -| `stores password when provided` | `form_test.exs` | AshAuthentication framework | Remove | -| `shows German labels` | `form_test.exs` | Gettext framework | Remove | -| `shows English labels` | `form_test.exs` | Gettext framework | Remove | -| `shows different labels for edit vs new` | `form_test.exs` | Gettext framework | Remove | -| `shows correct form elements for existing user` | `form_test.exs` | Basic HTML rendering | Consolidate | -| `shows admin password fields when enabled` | `form_test.exs` | UI state (framework) | Remove | -| `back button navigates to user list` | `show_test.exs` | Framework navigation | Remove | -| `edit button navigates to edit form` | `show_test.exs` | Framework navigation | Remove | -| `displays user email` | `show_test.exs` | Basic HTML rendering | Consolidate | -| `sets correct page title` | `show_test.exs` | Basic HTML rendering | Remove | - -**Total Tests to Remove:** ~30 tests -**Estimated Time Saved:** 8-12 seconds - ---- - -## Optimization Recommendations - -### Priority 1: Remove Framework Functionality Tests - -**Action:** Remove tests that verify framework functionality (Phoenix, Ash, Gettext) - -**Impact:** -- **Tests Removed:** ~30 tests -- **Time Saved:** 8-12 seconds -- **Risk:** ⚠️ Very Low - Framework functionality is tested by framework maintainers - -**Implementation:** -1. Remove tests listed in "Complete List of Tests to Remove/Consolidate" -2. Keep only business logic tests -3. Consolidate basic display tests into 1-2 smoke tests - -### Priority 2: Implement Shared Fixtures - -**Action:** Use `setup_all` for common test data - -**Current Problem:** -- Each test creates users/members independently -- `conn_with_oidc_user` creates new admin user each time -- Members created repeatedly for similar tests - -**Solution:** -```elixir -# In index_test.exs -setup_all do - # Create shared users for sorting tests - user_a = create_test_user(%{email: "alpha@example.com", oidc_id: "alpha"}) - user_z = create_test_user(%{email: "zulu@example.com", oidc_id: "zulu"}) - user_m = create_test_user(%{email: "mike@example.com", oidc_id: "mike"}) - - # Create shared users for checkbox tests - user1 = create_test_user(%{email: "user1@example.com", oidc_id: "user1"}) - user2 = create_test_user(%{email: "user2@example.com", oidc_id: "user2"}) - - %{ - sorting_users: [user_a, user_z, user_m], - checkbox_users: [user1, user2] - } -end -``` - -**Impact:** -- **Time Saved:** 3-5 seconds -- **Risk:** ⚠️ Low - Need to ensure test isolation is maintained - -**Note:** `async: false` in `form_test.exs` prevents `setup_all` usage due to sandbox limitations. Consider: -- Using `:shared` sandbox mode (adds complexity) -- Or keeping current approach for form tests - -### Priority 3: Consolidate Redundant Tests - -**Action:** Merge similar tests into single integration tests - -**Examples:** -- Basic display tests → 1 smoke test -- Navigation tests → 1 integration test -- Sorting tests → 1 integration test (keep business logic) - -**Impact:** -- **Tests Consolidated:** ~10 tests → 3-4 tests -- **Time Saved:** 2-3 seconds -- **Risk:** ⚠️ Very Low - Same coverage, fewer tests - -### Priority 4: Optimize Test Data Volume - -**Action:** Reduce number of test records where possible - -**Current:** -- Sorting tests create 3 users -- Checkbox tests create 2 users -- Some tests create multiple members - -**Optimization:** -- Use minimum required data (2 users for sorting, 2 for checkboxes) -- Share data across tests via `setup_all` - -**Impact:** -- **Time Saved:** 1-2 seconds -- **Risk:** ⚠️ Very Low - Still tests same functionality - ---- - -## Estimated Total Impact - -| Optimization | Tests Affected | Time Saved | Risk | -|--------------|----------------|------------|------| -| Remove framework tests | ~30 tests | 8-12s | ⚠️ Very Low | -| Shared fixtures | All test files | 3-5s | ⚠️ Low | -| Consolidate tests | ~10 tests | 2-3s | ⚠️ Very Low | -| Optimize data volume | Multiple tests | 1-2s | ⚠️ Very Low | -| **Total** | | **14-22s** | | - -**Projected Final Time:** From ~35.5s to **~13-21s** (40-60% reduction) - ---- - -## Risk Assessment - -### Overall Risk: ⚠️ Low - -**Mitigations:** -1. Framework functionality is tested by framework maintainers -2. Business logic tests remain intact -3. Shared fixtures maintain test isolation -4. Consolidation preserves coverage - -**What to Monitor:** -1. No increase in bugs related to removed functionality -2. Test coverage remains adequate -3. CI execution times improve as expected - ---- - -## Implementation Plan - -### Phase 1: Remove Framework Tests (Low Risk, High Impact) - -1. Remove translation tests (Gettext) -2. Remove navigation tests (Phoenix LiveView) -3. Remove validation tests (Ash) -4. Remove basic HTML rendering tests (consolidate into smoke test) - -**Estimated Time:** 1-2 hours -**Risk:** ⚠️ Very Low - -### Phase 2: Implement Shared Fixtures (Medium Risk, Medium Impact) - -1. Add `setup_all` to `index_test.exs` for sorting/checkbox users -2. Add `setup_all` to `show_test.exs` for common users/members -3. Update tests to use shared fixtures -4. Verify test isolation maintained - -**Estimated Time:** 2-3 hours -**Risk:** ⚠️ Low (need to verify sandbox isolation) - -### Phase 3: Consolidate Tests (Low Risk, Low Impact) - -1. Merge basic display tests into smoke test -2. Merge navigation tests into integration test -3. Reduce sorting tests to 1 integration test - -**Estimated Time:** 1-2 hours -**Risk:** ⚠️ Very 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 - ---- - -## Tests to Keep (Business Logic) - -### Index Tests -- ✅ `initially sorts by email ascending` - Tests default sort (business logic) -- ✅ `can sort email descending by clicking sort button` - Tests sort functionality -- ✅ `select all automatically checks when all individual users are selected` - Business logic -- ✅ `does not show system actor user in list` - Business rule -- ✅ `displays linked member name in user list` - Business logic -- ✅ `handles empty user list gracefully` - Edge case -- ✅ `handles users with missing OIDC ID` - Edge case - -### Form Tests -- ✅ `creates user without password` - Business logic -- ✅ `creates user with password when enabled` - Business logic -- ✅ `updates email without changing password` - Business logic -- ✅ `admin sets new password for user` - Business logic -- ✅ `selecting member and saving links member to user` - Business logic -- ✅ `unlinking member and saving removes member from user` - Business logic -- ✅ `redirects to user list when editing system actor user` - Business rule - -### Show Tests -- ✅ `displays password authentication status when enabled` - Business logic -- ✅ `displays password authentication status when not enabled` - Business logic -- ✅ `displays linked member when present` - Business logic -- ✅ `displays 'No member linked' when no member is linked` - Business logic -- ✅ `redirects to user list when viewing system actor user` - Business rule - -### Member Selection/Search Tests -- ✅ All tests in `form_member_selection_test.exs` - Business logic -- ✅ All tests in `form_member_search_test.exs` - Business logic (fuzzy search) - ---- - -## References - -- **Coding Guidelines:** `CODE_GUIDELINES.md` - Section 4.7 (Testing Best Practices) -- **Test Performance Optimization:** `docs/test-performance-optimization.md` -- **User LiveView Implementation:** `lib/mv_web/live/user_live/` - ---- - -## Next Steps - -1. ✅ Analysis complete -2. ⏳ Review with team -3. ⏳ Implement Phase 1 (Remove framework tests) -4. ⏳ Implement Phase 2 (Shared fixtures) -5. ⏳ Implement Phase 3 (Consolidate tests) -6. ⏳ Benchmark and verify diff --git a/docs/test-performance-optimization.md b/docs/test-performance-optimization.md index 807fb90..e8fdb09 100644 --- a/docs/test-performance-optimization.md +++ b/docs/test-performance-optimization.md @@ -373,30 +373,190 @@ After implementing the full test suite via promotion, the remaining slowest test ### Priority 1: User LiveView Tests Optimization -**Estimated Savings:** 15-20 seconds +**Estimated Savings:** 14-22 seconds +**Status:** 📋 Analysis Complete - Ready for Implementation -**Actions:** -1. Move shared fixtures to `setup_all` where possible -2. Reduce test data volume (use 3-5 users instead of 10+) -3. Analyze and optimize recurring setup patterns -4. Consider mocking expensive operations (if appropriate) +#### Analysis Summary + +Analysis of User LiveView tests identified significant optimization opportunities: +- **Framework functionality over-testing:** ~30 tests test Phoenix/Ash/Gettext core features +- **Redundant test data creation:** Each test creates users/members independently +- **Missing shared fixtures:** No `setup_all` usage for common data + +#### Current Performance + +**Top 20 Slowest Tests (User LiveView):** +- `index_test.exs`: ~10.2s for 3 tests in Top 20 +- `form_test.exs`: ~15.0s for 4 tests in Top 20 +- `show_test.exs`: ~10.3s for 3 tests in Top 20 +- **Total:** ~35.5 seconds for User LiveView tests + +#### Optimization Opportunities + +**1. Remove Framework Functionality Tests (~30 tests, 8-12s saved)** +- Remove translation tests (Gettext framework) +- Remove navigation tests (Phoenix LiveView framework) +- Remove validation tests (Ash framework) +- Remove basic HTML rendering tests (consolidate into smoke test) +- Remove password storage tests (AshAuthentication framework) + +**2. Implement Shared Fixtures (3-5s saved)** +- Use `setup_all` for common test data in `index_test.exs` and `show_test.exs` +- Share users for sorting/checkbox tests +- Share common users/members across tests +- **Note:** `form_test.exs` uses `async: false`, preventing `setup_all` usage + +**3. Consolidate Redundant Tests (~10 tests → 3-4 tests, 2-3s saved)** +- Merge basic display tests into smoke test +- Merge navigation tests into integration test +- Reduce sorting tests to 1 integration test + +**4. Optimize Test Data Volume (1-2s saved)** +- Use minimum required data (2 users for sorting, 2 for checkboxes) +- Share data across tests via `setup_all` + +#### Tests to Keep (Business Logic) + +**Index Tests:** +- `initially sorts by email ascending` - Tests default sort +- `can sort email descending by clicking sort button` - Tests sort functionality +- `select all automatically checks when all individual users are selected` - Business logic +- `does not show system actor user in list` - Business rule +- `displays linked member name in user list` - Business logic +- Edge case tests + +**Form Tests:** +- `creates user without password` - Business logic +- `creates user with password when enabled` - Business logic +- `admin sets new password for user` - Business logic +- `selecting member and saving links member to user` - Business logic +- Member linking/unlinking workflow tests + +**Show Tests:** +- `displays password authentication status` - Business logic +- `displays linked member when present` - Business logic +- `redirects to user list when viewing system actor user` - Business rule + +#### Implementation Plan + +**Phase 1: Remove Framework Tests (1-2 hours, ⚠️ Very Low Risk)** +- Remove translation, navigation, validation, and basic HTML rendering tests +- Consolidate remaining display tests into smoke test + +**Phase 2: Implement Shared Fixtures (2-3 hours, ⚠️ Low Risk)** +- Add `setup_all` to `index_test.exs` and `show_test.exs` +- Update tests to use shared fixtures +- Verify test isolation maintained + +**Phase 3: Consolidate Tests (1-2 hours, ⚠️ Very Low Risk)** +- Merge basic display tests into smoke test +- Merge navigation tests into integration test +- Reduce sorting tests to 1 integration test **Risk Assessment:** ⚠️ **Low** -- Requires careful testing to ensure isolation is maintained -- Should verify that shared fixtures don't cause test interdependencies +- Framework functionality is tested by framework maintainers +- Business logic tests remain intact +- Shared fixtures maintain test isolation +- Consolidation preserves coverage ### Priority 2: Policy Tests Optimization -**Estimated Savings:** 5-8 seconds +**Estimated Savings:** 5.5-9 seconds +**Status:** 📋 Analysis Complete - Ready for Decision -**Actions:** -1. Create roles in `setup_all` (they're identical across tests) -2. Reuse common fixtures (users, members) -3. Maintain test isolation while optimizing setup +#### Analysis 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 + +#### Current Performance + +**Policy Test Files Performance:** +- `member_policies_test.exs`: 24 tests, ~66s (top 20) +- `user_policies_test.exs`: 30 tests, ~66s (top 20) +- `custom_field_value_policies_test.exs`: 20 tests, ~66s (top 20) +- **Total:** 74 tests, ~152s total + +**Top 20 Slowest Policy Tests:** ~66 seconds + +#### Framework vs. Business Logic Analysis + +**Framework Functionality (Should NOT Test):** +- Policy evaluation (how Ash evaluates policies) +- Permission lookup (how Ash looks up permissions) +- Scope filtering (how Ash applies scope filters) +- Auto-filter behavior (how Ash auto-filters queries) +- Forbidden vs NotFound (how Ash returns errors) + +**Business Logic (Should Test):** +- Permission set definitions (what permissions each role has) +- Scope definitions (what scopes each permission set uses) +- Special cases (custom business rules) +- Permission set behavior (how our permission sets differ) + +#### Optimization Opportunities + +**1. Remove Framework Functionality Tests (~22-34 tests, 3-4s saved)** +- Remove "cannot" tests that verify error types (Forbidden, NotFound) +- Remove tests that verify auto-filter behavior (framework) +- Remove tests that verify permission evaluation (framework) +- **Risk:** ⚠️ Very Low - Framework functionality is tested by Ash maintainers + +**2. Consolidate Redundant Tests (~6-8 tests → 2-3 tests, 1-2s saved)** +- Merge similar tests across permission sets +- Create integration tests that cover multiple permission sets +- **Risk:** ⚠️ Low - Same coverage, fewer tests + +**3. Share Admin User Across Describe Blocks (1-2s saved)** +- Create admin user once in module-level `setup` +- Reuse admin user in helper functions +- **Note:** `async: false` prevents `setup_all`, but module-level `setup` works +- **Risk:** ⚠️ Low - Admin user is read-only in tests, safe to share + +**4. Reduce Test Data Volume (0.5-1s saved)** +- Use minimum required data +- Share fixtures where possible +- **Risk:** ⚠️ Very Low - Still tests same functionality + +#### Test Classification Summary + +**Tests to Remove (Framework):** +- `member_policies_test.exs`: ~10 tests (cannot create/destroy/update, auto-filter tests) +- `user_policies_test.exs`: ~16 tests (cannot read/update/create/destroy, auto-filter tests) +- `custom_field_value_policies_test.exs`: ~8 tests (similar "cannot" tests) + +**Tests to Consolidate (Redundant):** +- `user_policies_test.exs`: 6 tests → 2 tests (can read/update own user record) + +**Tests to Keep (Business Logic):** +- All "can" tests that verify permission set behavior +- Special case tests (e.g., "user can always READ linked member") +- AshAuthentication bypass tests (our integration) + +#### Implementation Plan + +**Phase 1: Remove Framework Tests (1-2 hours, ⚠️ Very Low Risk)** +- Identify all "cannot" tests that verify error types +- Remove tests that verify Ash auto-filter behavior +- Remove tests that verify permission evaluation (framework) + +**Phase 2: Consolidate Redundant Tests (1-2 hours, ⚠️ Low Risk)** +- Identify similar tests across permission sets +- Create integration tests that cover multiple permission sets +- Remove redundant individual tests + +**Phase 3: Share Admin User (1-2 hours, ⚠️ Low Risk)** +- Add module-level `setup` to create admin user +- Update helper functions to accept admin user parameter +- Update all `setup` blocks to use shared admin user **Risk Assessment:** ⚠️ **Low** -- Roles are read-only in tests, safe to share -- Need to ensure user/member fixtures don't interfere with each other +- Framework functionality is tested by Ash maintainers +- Business logic tests remain intact +- Admin user sharing maintains test isolation (read-only) +- Consolidation preserves coverage ### Priority 3: Seeds Tests Further Optimization @@ -431,13 +591,17 @@ After implementing the full test suite via promotion, the remaining slowest test | Priority | Optimization | Estimated Savings | |----------|-------------|-------------------| -| 1 | User LiveView Tests | 15-20s | -| 2 | Policy Tests | 5-8s | +| 1 | User LiveView Tests | 14-22s | +| 2 | Policy Tests | 5.5-9s | | 3 | Seeds Tests Further | 3-5s | | 4 | Additional Isolation | Variable | -| **Total Potential** | | **23-33 seconds** | +| **Total Potential** | | **22.5-36 seconds** | -**Projected Final Time:** From ~368 seconds (fast suite) to **~340-350 seconds** (~5.7-5.8 minutes) with remaining optimizations +**Projected Final Time:** From ~368 seconds (fast suite) to **~332-345 seconds** (~5.5-5.8 minutes) with remaining optimizations + +**Note:** Detailed analysis documents available: +- User LiveView Tests: See "Priority 1: User LiveView Tests Optimization" section above +- Policy Tests: See "Priority 2: Policy Tests Optimization" section above ---