diff --git a/.drone.yml b/.drone.yml index 0f874a9..e207ca0 100644 --- a/.drone.yml +++ b/.drone.yml @@ -1,6 +1,6 @@ kind: pipeline type: docker -name: check-fast +name: check services: - name: postgres @@ -72,7 +72,7 @@ steps: echo "Postgres did not become available, aborting." exit 1 - - name: test-fast + - name: test image: docker.io/library/elixir:1.18.3-otp-27 environment: MIX_ENV: test @@ -83,114 +83,7 @@ steps: - mix local.hex --force # Fetch dependencies - mix deps.get - # Run fast tests (excludes slow/performance and UI tests) - - mix test --exclude slow --exclude ui - - - name: rebuild-cache - image: drillster/drone-volume-cache - settings: - rebuild: true - mount: - - ./deps - - ./_build - volumes: - - name: cache - path: /cache - -volumes: - - name: cache - host: - path: /tmp/drone_cache - ---- -kind: pipeline -type: docker -name: check-full - -services: - - name: postgres - image: docker.io/library/postgres:18.1 - environment: - POSTGRES_USER: postgres - POSTGRES_PASSWORD: postgres - -trigger: - event: - - promote - target: - - production - -steps: - - name: compute cache key - image: docker.io/library/elixir:1.18.3-otp-27 - commands: - - mix_lock_hash=$(sha256sum mix.lock | cut -d ' ' -f 1) - - echo "$DRONE_REPO_OWNER/$DRONE_REPO_NAME/$mix_lock_hash" >> .cache_key - # Print cache key for debugging - - cat .cache_key - - - name: restore-cache - image: drillster/drone-volume-cache - settings: - restore: true - mount: - - ./deps - - ./_build - ttl: 30 - volumes: - - name: cache - path: /cache - - - name: lint - image: docker.io/library/elixir:1.18.3-otp-27 - commands: - # Install hex package manager - - mix local.hex --force - # Fetch dependencies - - mix deps.get - # Check for compilation errors & warnings - - mix compile --warnings-as-errors - # Check formatting - - mix format --check-formatted - # Security checks - - mix sobelow --config - # Check dependencies for known vulnerabilities - - mix deps.audit - # Check for dependencies that are not maintained anymore - - mix hex.audit - # Provide hints for improving code quality - - mix credo - # Check that translations are up to date - - mix gettext.extract --check-up-to-date - - - name: wait_for_postgres - image: docker.io/library/postgres:18.1 - commands: - # Wait for postgres to become available - - | - for i in {1..20}; do - if pg_isready -h postgres -U postgres; then - exit 0 - else - true - fi - sleep 2 - done - echo "Postgres did not become available, aborting." - exit 1 - - - name: test-all - image: docker.io/library/elixir:1.18.3-otp-27 - environment: - MIX_ENV: test - TEST_POSTGRES_HOST: postgres - TEST_POSTGRES_PORT: 5432 - commands: - # Install hex package manager - - mix local.hex --force - # Fetch dependencies - - mix deps.get - # Run all tests (including slow/performance and UI tests) + # Run tests - mix test - name: rebuild-cache @@ -254,7 +147,7 @@ steps: - push depends_on: - - check-fast + - check --- kind: pipeline diff --git a/.forgejo/README.md b/.forgejo/README.md deleted file mode 100644 index 6fa6d72..0000000 --- a/.forgejo/README.md +++ /dev/null @@ -1,48 +0,0 @@ -# Forgejo Configuration - -This directory contains configuration files for Forgejo (self-hosted Git service). - -## Pull Request Template - -The `pull_request_template.md` is automatically loaded when creating a new Pull Request. It provides a checklist and instructions for the PR workflow, including how to run the full test suite before merging. - -## Branch Protection Setup - -To enforce the full test suite before merging to `main`, configure branch protection in Forgejo: - -### Steps: - -1. Go to **Repository Settings** → **Branches** → **Protected Branches** -2. Add a new rule for branch: `main` -3. Configure the following settings: - - ☑️ **Enable Branch Protection** - - ☑️ **Require status checks to pass before merging** - - Add required check: `check-full` - - ☐ **Require approvals** (optional, based on team preference) - - ☑️ **Block if there are outstanding requests** (optional) - -### What this does: - -- The **"Merge"** button in PRs will only be enabled after `check-full` passes -- `check-full` is triggered by **promoting** a build in Drone CI (see PR template) -- This ensures all tests (including slow and UI tests) run before merging - -## Workflow - -1. **Create PR** → Fast test suite (`check-fast`) runs automatically -2. **Development** → Fast tests run on every push for quick feedback -3. **Ready to merge:** - - Remove `WIP:` from PR title - - Go to Drone CI and **promote** the build to `production` - - This triggers `check-full` (full test suite) -4. **After full tests pass** → Merge button becomes available -5. **Merge to main** → Container is built and published - -## Secrets Required - -Make sure the following secrets are configured in Drone CI: - -- `DRONE_REGISTRY_USERNAME` - For container registry -- `DRONE_REGISTRY_TOKEN` - For container registry -- `RENOVATE_TOKEN` - For Renovate bot -- `GITHUB_COM_TOKEN` - For Renovate bot (GitHub dependencies) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 0a87836..eb3e4ed 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1661,59 +1661,6 @@ end - Mock external services - Use fixtures efficiently -**Performance Tests:** - -Performance tests that explicitly validate performance characteristics should be tagged with `@tag :slow` or `@describetag :slow` to exclude them from standard test runs. This improves developer feedback loops while maintaining comprehensive coverage. - -**When to Tag as `:slow`:** - -- Tests that explicitly measure execution time or validate performance characteristics -- Tests that use large datasets (e.g., 50+ records) to test scalability -- Tests that validate query optimization (N+1 prevention, index usage) - -**Tagging Guidelines:** - -- Use `@tag :slow` for individual tests -- Use `@describetag :slow` for entire describe blocks (not `@moduletag`, as it affects all tests in the module) -- Performance tests should include measurable assertions (query counts, timing with tolerance, etc.) - -**UI Tests:** - -UI tests that validate basic HTML rendering, Phoenix LiveView navigation, or framework functionality (Gettext translations, form elements, UI state changes) should be tagged with `@tag :ui` or `@describetag :ui` to exclude them from fast CI runs. Use `@tag :ui` for individual tests and `@describetag :ui` for describe blocks. UI tests can be consolidated when they test similar elements (e.g., multiple translation tests combined into one). Do not tag business logic tests (e.g., "can delete a user"), validation tests, or data persistence tests as `:ui`. - -**Running Tests:** - -```bash -# Fast tests only (excludes slow and UI tests) -mix test --exclude slow --exclude ui -# Or use the Justfile command: -just test-fast - -# UI tests only -mix test --only ui -# Or use the Justfile command: -just ui - -# Performance tests only -mix test --only slow -# Or use the Justfile command: -just slow - -# All tests (including slow and UI tests) -mix test -# Or use the Justfile command: -just test -# Or use the Justfile command: -just test-all -``` - -**Test Organization Best Practices:** - -- **Fast Tests (Standard CI):** Business logic, validations, data persistence, edge cases -- **UI Tests (Full Test Suite):** Basic HTML rendering, navigation, translations, UI state -- **Performance Tests (Full Test Suite):** Query optimization, large datasets, timing assertions - -This organization ensures fast feedback in standard CI while maintaining comprehensive coverage via promotion before merge. --- ## 5. Security Guidelines diff --git a/Justfile b/Justfile index bce8bf6..f25041c 100644 --- a/Justfile +++ b/Justfile @@ -22,7 +22,7 @@ seed-database: start-database: docker compose up -d -ci-dev: lint audit test-fast +ci-dev: lint audit test gettext: mix gettext.extract @@ -41,30 +41,9 @@ audit: mix deps.audit mix hex.audit -# Run all tests test *args: install-dependencies mix test {{args}} -# Run only fast tests (excludes slow/performance and UI tests) -test-fast *args: install-dependencies - mix test --exclude slow --exclude ui {{args}} - -# Run only UI tests -ui *args: install-dependencies - mix test --only ui {{args}} - -# Run only slow/performance tests -slow *args: install-dependencies - mix test --only slow {{args}} - -# Run only slow/performance tests (alias for consistency) -test-slow *args: install-dependencies - mix test --only slow {{args}} - -# Run all tests (fast + slow + ui) -test-all *args: install-dependencies - mix test {{args}} - format: mix format diff --git a/docs/test-performance-optimization.md b/docs/test-performance-optimization.md deleted file mode 100644 index e8fdb09..0000000 --- a/docs/test-performance-optimization.md +++ /dev/null @@ -1,877 +0,0 @@ -# Test Performance Optimization - -**Last Updated:** 2026-01-28 -**Status:** ✅ Active optimization program - ---- - -## Executive Summary - -This document provides a comprehensive overview of test performance optimizations, risk assessments, and future opportunities. The test suite execution time has been reduced through systematic analysis and targeted optimizations. - -### Current Performance Metrics - -| Metric | Value | -|--------|-------| -| **Total Execution Time** (without `:slow` tests) | ~368 seconds (~6.1 minutes) | -| **Total Tests** | 1,336 tests (+ 25 doctests) | -| **Async Execution** | 163.5 seconds | -| **Sync Execution** | 281.5 seconds | -| **Slow Tests Excluded** | 25 tests (tagged with `@tag :slow`) | -| **Top 50 Slowest Tests** | 121.9 seconds (27.4% of total time) | - -### Optimization Impact Summary - -| Optimization | Tests Affected | Time Saved | Status | -|--------------|----------------|------------|--------| -| Seeds tests reduction | 13 → 4 tests | ~10-16s | ✅ Completed | -| Performance tests tagging | 9 tests | ~3-4s per run | ✅ Completed | -| Critical test query filtering | 1 test | ~8-10s | ✅ Completed | -| Full test suite via promotion | 25 tests | ~77s per run | ✅ Completed | -| **Total Saved** | | **~98-107s** | | - ---- - -## Completed Optimizations - -### 1. Seeds Test Suite Optimization - -**Date:** 2026-01-28 -**Status:** ✅ Completed - -#### What Changed - -- **Reduced test count:** From 13 tests to 4 tests (69% reduction) -- **Reduced seeds executions:** From 8-10 times to 5 times per test run -- **Execution time:** From 24-30 seconds to 13-17 seconds -- **Time saved:** ~10-16 seconds per test run (40-50% faster) - -#### Removed Tests (9 tests) - -Tests were removed because their functionality is covered by domain-specific test suites: - -1. `"at least one member has no membership fee type assigned"` → Covered by `membership_fees/*_test.exs` -2. `"each membership fee type has at least one member"` → Covered by `membership_fees/*_test.exs` -3. `"members with fee types have cycles with various statuses"` → Covered by `cycle_generator_test.exs` -4. `"creates all 5 authorization roles with correct permission sets"` → Covered by `authorization/*_test.exs` -5. `"all roles have valid permission_set_names"` → Covered by `authorization/permission_sets_test.exs` -6. `"does not change role of users who already have a role"` → Merged into idempotency test -7. `"role creation is idempotent"` (detailed) → Merged into general idempotency test - -#### Retained Tests (4 tests) - -Critical deployment requirements are still covered: - -1. ✅ **Smoke Test:** Seeds run successfully and create basic data -2. ✅ **Idempotency Test:** Seeds can be run multiple times without duplicating data -3. ✅ **Admin Bootstrap:** Admin user exists with Admin role (critical for initial access) -4. ✅ **System Role Bootstrap:** Mitglied system role exists (critical for user registration) - -#### Risk Assessment - -| Removed Test Category | Alternative Coverage | Risk Level | -|----------------------|---------------------|------------| -| Member/fee type distribution | `membership_fees/*_test.exs` | ⚠️ Low | -| Cycle status variations | `cycle_generator_test.exs` | ⚠️ Low | -| Detailed role configs | `authorization/*_test.exs` | ⚠️ Very Low | -| Permission set validation | `permission_sets_test.exs` | ⚠️ Very Low | - -**Overall Risk:** ⚠️ **Low** - All removed tests have equivalent or better coverage in domain-specific test suites. - ---- - -### 2. Full Test Suite via Promotion (`@tag :slow`) - -**Date:** 2026-01-28 -**Status:** ✅ Completed - -#### What Changed - -Tests with **low risk** and **execution time >1 second** are now tagged with `@tag :slow` and excluded from standard test runs. These tests are important but not critical for every commit and are run via promotion before merging to `main`. - -#### Tagging Criteria - -**Tagged as `@tag :slow` when:** -- ✅ Test execution time >1 second -- ✅ Low risk (not critical for catching regressions in core business logic) -- ✅ UI/Display tests (formatting, rendering) -- ✅ Workflow detail tests (not core functionality) -- ✅ Edge cases with large datasets - -**NOT tagged when:** -- ❌ Core CRUD operations (Member/User Create/Update/Destroy) -- ❌ Basic Authentication/Authorization -- ❌ Critical Bootstrap (Admin user, system roles) -- ❌ Email Synchronization -- ❌ Representative tests per Permission Set + Action - -#### Identified Tests for Full Test Suite (25 tests) - -**1. Seeds Tests (2 tests) - 18.1s** -- `"runs successfully and creates basic data"` (9.0s) -- `"is idempotent when run multiple times"` (9.1s) -- **Note:** Critical bootstrap tests remain in fast suite - -**2. UserLive.ShowTest (3 tests) - 10.8s** -- `"mounts successfully with valid user ID"` (4.2s) -- `"displays linked member when present"` (2.4s) -- `"redirects to user list when viewing system actor user"` (4.2s) - -**3. UserLive.IndexTest (5 tests) - 25.0s** -- `"displays users in a table"` (1.0s) -- `"initially sorts by email ascending"` (2.2s) -- `"can sort email descending by clicking sort button"` (3.4s) -- `"select all automatically checks when all individual users are selected"` (2.0s) -- `"displays linked member name in user list"` (1.9s) - -**4. MemberLive.IndexCustomFieldsDisplayTest (3 tests) - 4.9s** -- `"displays custom field with show_in_overview: true"` (1.6s) -- `"formats date custom field values correctly"` (1.5s) -- `"formats email custom field values correctly"` (1.8s) - -**5. MemberLive.IndexCustomFieldsEdgeCasesTest (3 tests) - 3.6s** -- `"displays custom field column even when no members have values"` (1.1s) -- `"displays very long custom field values correctly"` (1.4s) -- `"handles multiple custom fields with show_in_overview correctly"` (1.2s) - -**6. RoleLive Tests (7 tests) - 7.7s** -- `role_live_test.exs`: `"mounts successfully"` (1.5s), `"deletes non-system role"` (2.1s) -- `role_live/show_test.exs`: 5 tests >1s (mount, display, navigation) - -**7. MemberAvailableForLinkingTest (1 test) - 1.5s** -- `"limits results to 10 members even when more exist"` (1.5s) - -**8. Performance Tests (1 test) - 3.8s** -- `"boolean filter performance with 150 members"` (3.8s) - -**Total:** 25 tests, ~77 seconds saved - -#### Execution Commands - -**Fast Tests (Default):** -```bash -just test-fast -# or -mix test --exclude slow -``` - -**Slow Tests Only:** -```bash -just test-slow -# or -mix test --only slow -``` - -**All Tests:** -```bash -just test -# or -mix test -``` - -#### CI/CD Integration - -- **Standard CI (`check-fast`):** Runs `mix test --exclude slow --exclude ui` for faster feedback loops (~6 minutes) -- **Full Test Suite (`check-full`):** Triggered via promotion before merge, executes `mix test` (all tests, including slow and UI) for comprehensive coverage (~7.4 minutes) -- **Pre-Merge:** Full test suite (`mix test`) runs via promotion before merging to main -- **Manual Execution:** Promote build to `production` in Drone CI to trigger full test suite - -#### Risk Assessment - -**Risk Level:** ✅ **Very Low** - -- All tagged tests have **low risk** - they don't catch critical regressions -- Core functionality remains tested (CRUD, Auth, Bootstrap) -- Standard test runs are faster (~6 minutes vs ~7.4 minutes) -- Full test suite runs via promotion before merge ensures comprehensive coverage -- No functionality is lost, only execution timing changed - -**Critical Tests Remain in Fast Suite:** -- Core CRUD operations (Member/User Create/Update/Destroy) -- Basic Authentication/Authorization -- Critical Bootstrap (Admin user, system roles) -- Email Synchronization -- Representative Policy tests (one per Permission Set + Action) - ---- - -### 3. Critical Test Optimization - -**Date:** 2026-01-28 -**Status:** ✅ Completed - -#### Problem Identified - -The test `test respects show_in_overview config` was the slowest test in the suite: -- **Isolated execution:** 4.8 seconds -- **In full test run:** 14.7 seconds -- **Difference:** 9.9 seconds (test isolation issue) - -#### Root Cause - -The test loaded **all members** from the database, not just the 2 members from the test setup. In full test runs, many members from other tests were present in the database, significantly slowing down the query. - -#### Solution Implemented - -**Query Filtering:** Added search query parameter to filter to only the expected member. - -**Code Change:** -```elixir -# Before: -{:ok, _view, html} = live(conn, "/members") - -# After: -{:ok, _view, html} = live(conn, "/members?query=Alice") -``` - -#### Results - -| Execution | Before | After | Improvement | -|-----------|--------|-------|-------------| -| **Isolated** | 4.8s | 1.1s | **-77%** (3.7s saved) | -| **In Module** | 4.2s | 0.4s | **-90%** (3.8s saved) | -| **Expected in Full Run** | 14.7s | ~4-6s | **-65% to -73%** (8-10s saved) | - -#### Risk Assessment - -**Risk Level:** ✅ **Very Low** - -- Test functionality unchanged - only loads expected data -- All assertions still pass -- Test is now faster and more isolated -- No impact on test coverage - ---- - -### 3. Full Test Suite Analysis and Categorization - -**Date:** 2026-01-28 -**Status:** ✅ Completed - -#### Analysis Methodology - -A comprehensive analysis was performed to identify tests suitable for the full test suite (via promotion) based on: -- **Execution time:** Tests taking >1 second -- **Risk assessment:** Tests that don't catch critical regressions -- **Test category:** UI/Display, workflow details, edge cases - -#### Test Categorization - -**🔴 CRITICAL - Must Stay in Fast Suite:** -- Core Business Logic (Member/User CRUD) -- Authentication & Authorization Basics -- Critical Bootstrap (Admin user, system roles) -- Email Synchronization -- Representative Policy Tests (one per Permission Set + Action) - -**🟡 LOW RISK - Moved to Full Test Suite (via Promotion):** -- Seeds Tests (non-critical: smoke test, idempotency) -- LiveView Display/Formatting Tests -- UserLive.ShowTest (core functionality covered by Index/Form) -- UserLive.IndexTest UI Features (sorting, checkboxes, navigation) -- RoleLive Tests (role management, not core authorization) -- MemberLive Custom Fields Display Tests -- Edge Cases with Large Datasets - -#### Risk Assessment Summary - -| Category | Tests | Time Saved | Risk Level | Rationale | -|----------|-------|------------|------------|-----------| -| Seeds (non-critical) | 2 | 18.1s | ⚠️ Low | Critical bootstrap tests remain | -| UserLive.ShowTest | 3 | 10.8s | ⚠️ Low | Core CRUD covered by Index/Form | -| UserLive.IndexTest (UI) | 5 | 25.0s | ⚠️ Low | UI features, not core functionality | -| Custom Fields Display | 6 | 8.5s | ⚠️ Low | Formatting tests, visible in code review | -| RoleLive Tests | 7 | 7.7s | ⚠️ Low | Role management, not authorization | -| Edge Cases | 1 | 1.5s | ⚠️ Low | Edge case, not critical path | -| Performance Tests | 1 | 3.8s | ✅ Very Low | Explicit performance validation | -| **Total** | **25** | **~77s** | **⚠️ Low** | | - -**Overall Risk:** ⚠️ **Low** - All moved tests have low risk and don't catch critical regressions. Core functionality remains fully tested. - -#### Tests Excluded from Full Test Suite - -The following tests were **NOT** moved to full test suite (via promotion) despite being slow: - -- **Policy Tests:** Medium risk - kept in fast suite (representative tests remain) -- **UserLive.FormTest:** Medium risk - core CRUD functionality -- **Tests <1s:** Don't meet execution time threshold -- **Critical Bootstrap Tests:** High risk - deployment critical - ---- - -## Current Performance Analysis - -### Top 20 Slowest Tests (without `:slow`) - -After implementing the full test suite via promotion, the remaining slowest tests are: - -| Rank | Test | File | Time | Category | -|------|------|------|------|----------| -| 1 | `test Critical bootstrap invariants Mitglied system role exists` | `seeds_test.exs` | 6.7s | Critical Bootstrap | -| 2 | `test Critical bootstrap invariants Admin user has Admin role` | `seeds_test.exs` | 5.0s | Critical Bootstrap | -| 3 | `test normal_user permission set can read own user record` | `user_policies_test.exs` | 2.6s | Policy Test | -| 4 | `test normal_user permission set can create member` | `member_policies_test.exs` | 2.5s | Policy Test | -| 5-20 | Various Policy and LiveView tests | Multiple files | 1.5-2.4s each | Policy/LiveView | - -**Total Top 20:** ~44 seconds (12% of total time without `:slow`) - -**Note:** Many previously slow tests (UserLive.IndexTest, UserLive.ShowTest, Display/Formatting tests) are now tagged with `@tag :slow` and excluded from standard runs. - -### Performance Hotspots Identified - -#### 1. Seeds Tests (~16.2s for 4 tests) - -**Status:** ✅ Optimized (reduced from 13 tests) -**Remaining Optimization Potential:** 3-5 seconds - -**Opportunities:** -- Settings update could potentially be moved to `setup_all` (if sandbox allows) -- Seeds execution could be further optimized (less data in test mode) -- Idempotency test could be optimized (only 1x seeds instead of 2x) - -#### 2. User LiveView Tests (~35.5s for 10 tests) - -**Status:** ⏳ Identified for optimization -**Optimization Potential:** 15-20 seconds - -**Files:** -- `test/mv_web/user_live/index_test.exs` (3 tests, ~10.2s) -- `test/mv_web/user_live/form_test.exs` (4 tests, ~15.0s) -- `test/mv_web/user_live/show_test.exs` (3 tests, ~10.3s) - -**Patterns:** -- Many tests create user/member data -- LiveView mounts are expensive -- Form submissions with validations are slow - -**Recommended Actions:** -- Move shared fixtures to `setup_all` -- Reduce test data volume (3-5 users instead of 10+) -- Optimize setup patterns for recurring patterns - -#### 3. Policy Tests (~8.7s for 3 tests) - -**Status:** ⏳ Identified for optimization -**Optimization Potential:** 5-8 seconds - -**Files:** -- `test/mv/membership/member_policies_test.exs` (2 tests, ~6.1s) -- `test/mv/accounts/user_policies_test.exs` (1 test, ~2.6s) - -**Pattern:** -- Each test creates new roles/users/members -- Roles are identical across tests - -**Recommended Actions:** -- Create roles in `setup_all` (shared across tests) -- Reuse common fixtures -- Maintain test isolation while optimizing setup - ---- - -## Future Optimization Opportunities - -### Priority 1: User LiveView Tests Optimization - -**Estimated Savings:** 14-22 seconds -**Status:** 📋 Analysis Complete - Ready for Implementation - -#### 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** -- 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.5-9 seconds -**Status:** 📋 Analysis Complete - Ready for Decision - -#### 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** -- 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 - -**Estimated Savings:** 3-5 seconds - -**Actions:** -1. Investigate if settings update can be moved to `setup_all` -2. Introduce seeds mode for tests (less data in test mode) -3. Optimize idempotency test (only 1x seeds instead of 2x) - -**Risk Assessment:** ⚠️ **Low to Medium** -- Sandbox limitations may prevent `setup_all` usage -- Seeds mode would require careful implementation -- Idempotency test optimization needs to maintain test validity - -### Priority 4: Additional Test Isolation Improvements - -**Estimated Savings:** Variable (depends on specific tests) - -**Actions:** -1. Review tests that load all records (similar to the critical test fix) -2. Add query filters where appropriate -3. Ensure proper test isolation in async tests - -**Risk Assessment:** ⚠️ **Very Low** -- Similar to the critical test optimization (proven approach) -- Improves test isolation and reliability - ---- - -## Estimated Total Optimization Potential - -| Priority | Optimization | Estimated Savings | -|----------|-------------|-------------------| -| 1 | User LiveView Tests | 14-22s | -| 2 | Policy Tests | 5.5-9s | -| 3 | Seeds Tests Further | 3-5s | -| 4 | Additional Isolation | Variable | -| **Total Potential** | | **22.5-36 seconds** | - -**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 - ---- - -## Risk Assessment Summary - -### Overall Risk Level: ⚠️ **Low** - -All optimizations maintain test coverage while improving performance: - -| Optimization | Risk Level | Mitigation | -|-------------|------------|------------| -| Seeds tests reduction | ⚠️ Low | Coverage mapped to domain tests | -| Performance tests tagging | ✅ Very Low | Tests still executed, just separately | -| Critical test optimization | ✅ Very Low | Functionality unchanged, better isolation | -| Future optimizations | ⚠️ Low | Careful implementation with verification | - -### Monitoring Plan - -#### Success Criteria - -- ✅ Seeds tests execute in <20 seconds consistently -- ✅ No increase in seeds-related deployment failures -- ✅ No regression in authorization or membership fee bugs -- ✅ Top 20 slowest tests: < 60 seconds (currently ~44s) -- ✅ Total execution time (without `:slow`): < 10 minutes (currently 6.1 min) -- ⏳ Slow tests execution time: < 2 minutes (currently ~1.3 min) - -#### What to Watch For - -1. **Production Seeds Failures:** - - Monitor deployment logs for seeds errors - - If failures increase, consider restoring detailed tests - -2. **Authorization Bugs After Seeds Changes:** - - If role/permission bugs appear after seeds modifications - - May indicate need for more seeds-specific role validation - -3. **Test Performance Regression:** - - Monitor test execution times in CI - - Alert if times increase significantly - -4. **Developer Feedback:** - - If developers report missing test coverage - - Adjust based on real-world experience - ---- - -## Benchmarking and Analysis - -### How to Benchmark Tests - -**ExUnit Built-in Benchmarking:** - -The test suite is configured to show the slowest tests automatically: - -```elixir -# test/test_helper.exs -ExUnit.start( - slowest: 10 # Shows 10 slowest tests at the end of test run -) -``` - -**Run Benchmark Analysis:** - -```bash -# Show slowest tests -mix test --slowest 20 - -# Exclude slow tests for faster feedback -mix test --exclude slow --slowest 20 - -# Run only slow tests -mix test --only slow --slowest 10 - -# Benchmark specific test file -mix test test/mv_web/member_live/index_member_fields_display_test.exs --slowest 5 -``` - -### Benchmarking Best Practices - -1. **Run benchmarks regularly** (e.g., monthly) to catch performance regressions -2. **Compare isolated vs. full runs** to identify test isolation issues -3. **Monitor CI execution times** to track trends over time -4. **Document significant changes** in test performance - ---- - -## Test Suite Structure - -### Test Execution Modes - -**Fast Tests (Default):** -- Excludes slow tests (`@tag :slow`) -- Used for standard development workflow -- Execution time: ~6 minutes -- Command: `mix test --exclude slow` or `just test-fast` - -**Slow Tests:** -- Tests tagged with `@tag :slow` or `@describetag :slow` (25 tests) -- Low risk, >1 second execution time -- UI/Display tests, workflow details, edge cases, performance tests -- Execution time: ~1.3 minutes -- Command: `mix test --only slow` or `just test-slow` -- Excluded from standard CI runs - -**Full Test Suite (via Promotion):** -- Triggered by promoting a build to `production` in Drone CI -- Runs all tests (`mix test`) for comprehensive coverage -- Execution time: ~7.4 minutes -- Required before merging to `main` (enforced via branch protection) - -**All Tests:** -- Includes both fast and slow tests -- Used for comprehensive validation (pre-merge) -- Execution time: ~7.4 minutes -- Command: `mix test` or `just test` - -### Test Organization - -Tests are organized to mirror the `lib/` directory structure: - -``` -test/ -├── accounts/ # Accounts domain tests -├── membership/ # Membership domain tests -├── membership_fees/ # Membership fees domain tests -├── mv/ # Core application tests -│ ├── accounts/ # User-related tests -│ ├── membership/ # Member-related tests -│ └── authorization/ # Authorization tests -├── mv_web/ # Web layer tests -│ ├── controllers/ # Controller tests -│ ├── live/ # LiveView tests -│ └── components/ # Component tests -└── support/ # Test helpers - ├── conn_case.ex # Controller test setup - └── data_case.ex # Database test setup -``` - ---- - -## Best Practices for Test Performance - -### When Writing New Tests - -1. **Use `async: true`** when possible (for parallel execution) -2. **Filter queries** to only load necessary data -3. **Share fixtures** in `setup_all` when appropriate -4. **Tag performance tests** with `@tag :slow` if they use large datasets -5. **Keep test data minimal** - only create what's needed for the test - -### When Optimizing Existing Tests - -1. **Measure first** - Use `mix test --slowest` to identify bottlenecks -2. **Compare isolated vs. full runs** - Identify test isolation issues -3. **Optimize setup** - Move shared data to `setup_all` where possible -4. **Filter queries** - Only load data needed for the test -5. **Verify coverage** - Ensure optimizations don't reduce test coverage - -### Test Tagging Guidelines - -#### Tag as `@tag :slow` when: - -1. **Performance Tests:** - - Explicitly testing performance characteristics - - Using large datasets (50+ records) - - Testing scalability or query optimization - - Validating N+1 query prevention - -2. **Low-Risk Tests (>1s):** - - UI/Display/Formatting tests (not critical for every commit) - - Workflow detail tests (not core functionality) - - Edge cases with large datasets - - Show page tests (core functionality covered by Index/Form tests) - - Non-critical seeds tests (smoke tests, idempotency) - -#### Do NOT tag as `@tag :slow` when: - -- ❌ Test is slow due to inefficient setup (fix the setup instead) -- ❌ Test is slow due to bugs (fix the bug instead) -- ❌ Core CRUD operations (Member/User Create/Update/Destroy) -- ❌ Basic Authentication/Authorization -- ❌ Critical Bootstrap (Admin user, system roles) -- ❌ Email Synchronization -- ❌ Representative Policy tests (one per Permission Set + Action) -- ❌ It's an integration test (use `@tag :integration` instead) - ---- - -## Changelog - -### 2026-01-28: Initial Optimization Phase - -**Completed:** -- ✅ Reduced seeds tests from 13 to 4 tests -- ✅ Tagged 9 performance tests with `@tag :slow` -- ✅ Optimized critical test with query filtering -- ✅ Created slow test suite infrastructure -- ✅ Updated CI/CD to exclude slow tests from standard runs -- ✅ Added promotion-based full test suite pipeline (`check-full`) - -**Time Saved:** ~21-30 seconds per test run - -### 2026-01-28: Full Test Suite via Promotion Implementation - -**Completed:** -- ✅ Analyzed all tests for full test suite candidates -- ✅ Identified 36 tests with low risk and >1s execution time -- ✅ Tagged 25 tests with `@tag :slow` for full test suite (via promotion) -- ✅ Categorized tests by risk level and execution time -- ✅ Documented tagging criteria and guidelines - -**Tests Tagged:** -- 2 Seeds tests (non-critical) - 18.1s -- 3 UserLive.ShowTest tests - 10.8s -- 5 UserLive.IndexTest tests - 25.0s -- 3 MemberLive.IndexCustomFieldsDisplayTest tests - 4.9s -- 3 MemberLive.IndexCustomFieldsEdgeCasesTest tests - 3.6s -- 7 RoleLive tests - 7.7s -- 1 MemberAvailableForLinkingTest - 1.5s -- 1 Performance test (already tagged) - 3.8s - -**Time Saved:** ~77 seconds per test run - -**Total Optimization Impact:** -- **Before:** ~445 seconds (7.4 minutes) -- **After (fast suite):** ~368 seconds (6.1 minutes) -- **Time saved:** ~77 seconds (17% reduction) - -**Next Steps:** -- ⏳ Monitor full test suite execution via promotion in CI -- ⏳ Optimize remaining slow tests (Policy tests, etc.) -- ⏳ Further optimize Seeds tests (Priority 3) - ---- - -## References - -- **Testing Standards:** `CODE_GUIDELINES.md` - Section 4 (Testing Standards) -- **CI/CD Configuration:** `.drone.yml` -- **Test Helper:** `test/test_helper.exs` -- **Justfile Commands:** `Justfile` (test-fast, test-slow, test-all) - ---- - -## Questions & Answers - -**Q: What if seeds create wrong data and break the system?** -A: The smoke test will fail if seeds raise errors. Domain tests ensure business logic is correct regardless of seeds content. - -**Q: What if we add a new critical bootstrap requirement?** -A: Add a new test to the "Critical bootstrap invariants" section in `test/seeds_test.exs`. - -**Q: How do we know the removed tests aren't needed?** -A: Monitor for 2-3 months. If no seeds-related bugs appear that would have been caught by removed tests, they were redundant. - -**Q: Should we restore the tests for important releases?** -A: Consider running the full test suite (including slow tests) before major releases. Daily development uses the optimized suite. - -**Q: How do I add a new performance test?** -A: Tag it with `@tag :slow` for individual tests or `@describetag :slow` for describe blocks. Use `@describetag` instead of `@moduletag` to avoid tagging unrelated tests. Include measurable performance assertions (query counts, timing with tolerance, etc.). See "Performance Test Guidelines" section above. - -**Q: Can I run slow tests locally?** -A: Yes, use `just test-slow` or `mix test --only slow`. They're excluded from standard runs for faster feedback. - -**Q: What is the "full test suite"?** -A: The full test suite runs **all tests** (`mix test`), including slow and UI tests. Tests tagged with `@tag :slow` or `@describetag :slow` are excluded from standard CI runs (`check-fast`) for faster feedback, but are included when promoting a build to `production` (`check-full`) before merging to `main`. - -**Q: Which tests should I tag as `:slow`?** -A: Tag tests with `@tag :slow` if they: (1) take >1 second, (2) have low risk (not critical for catching regressions), and (3) test UI/Display/Formatting or workflow details. See "Test Tagging Guidelines" section for details. - -**Q: What if a slow test fails in the full test suite?** -A: If a test in the full test suite fails, investigate the failure. If it indicates a critical regression, consider moving it back to the fast suite. If it's a flaky test, fix the test itself. The merge will be blocked until all tests pass. diff --git a/lib/mv_web/live/group_live/index.ex b/lib/mv_web/live/group_live/index.ex index deab7e1..b0d428d 100644 --- a/lib/mv_web/live/group_live/index.ex +++ b/lib/mv_web/live/group_live/index.ex @@ -113,22 +113,16 @@ defmodule MvWeb.GroupLive.Index do defp load_groups(actor) do require Ash.Query - # Load groups without aggregates first (faster) query = Mv.Membership.Group + |> Ash.Query.load(:member_count) |> Ash.Query.sort(:name) opts = ash_actor_opts(actor) case Ash.read(query, opts) do {:ok, groups} -> - # Load all member counts in a single batch query (avoids N+1) - member_counts = load_member_counts_batch(groups) - - # Attach counts to groups - Enum.map(groups, fn group -> - Map.put(group, :member_count, Map.get(member_counts, group.id, 0)) - end) + groups {:error, _error} -> require Logger @@ -136,33 +130,4 @@ defmodule MvWeb.GroupLive.Index do [] end end - - # Loads all member counts for groups using DB-side aggregation for better performance - # This avoids N+1 queries when loading member_count aggregate for each group - @spec load_member_counts_batch([Mv.Membership.Group.t()]) :: %{ - Ecto.UUID.t() => non_neg_integer() - } - defp load_member_counts_batch(groups) do - group_ids = Enum.map(groups, & &1.id) - - if Enum.empty?(group_ids) do - %{} - else - # Use Ecto directly for efficient GROUP BY COUNT query - # This is much more performant than loading aggregates for each group individually - # Note: We bypass Ash here for performance, but this is a simple read-only query - import Ecto.Query - - query = - from mg in Mv.Membership.MemberGroup, - where: mg.group_id in ^group_ids, - group_by: mg.group_id, - select: {mg.group_id, count(mg.id)} - - results = Mv.Repo.all(query) - - results - |> Enum.into(%{}, fn {group_id, count} -> {group_id, count} end) - end - end end diff --git a/lib/mv_web/live/group_live/show.ex b/lib/mv_web/live/group_live/show.ex index 0899728..8114fa5 100644 --- a/lib/mv_web/live/group_live/show.ex +++ b/lib/mv_web/live/group_live/show.ex @@ -38,16 +38,7 @@ defmodule MvWeb.GroupLive.Show do end defp load_group_by_slug(socket, slug, actor) do - # Load group with members and member_count - # Using explicit load ensures efficient preloading of members relationship - require Ash.Query - - query = - Mv.Membership.Group - |> Ash.Query.filter(slug == ^slug) - |> Ash.Query.load([:members, :member_count]) - - case Ash.read_one(query, actor: actor, domain: Mv.Membership) do + case Membership.get_group_by_slug(slug, actor: actor, load: [:members, :member_count]) do {:ok, nil} -> {:noreply, socket diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index 673502d..50b0cfa 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/lib/mv_web/live/member_live/index.ex @@ -328,12 +328,8 @@ defmodule MvWeb.MemberLive.Index do {new_field, new_order} = determine_new_sort(field, socket) - old_field = socket.assigns.sort_field - socket - |> assign(:sort_field, new_field) - |> assign(:sort_order, new_order) - |> update_sort_components(old_field, new_field, new_order) + |> update_sort_components(socket.assigns.sort_field, new_field, new_order) |> push_sort_url(new_field, new_order) end diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index 1eb3e47..4526e4c 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -90,14 +90,11 @@ defmodule MvWeb.UserLive.Index do # Selects one user in the list of users @impl true def handle_event("select_user", %{"id" => id}, socket) do - # Normalize ID to string for consistent comparison - id_str = to_string(id) - selected = - if id_str in socket.assigns.selected_users do - List.delete(socket.assigns.selected_users, id_str) + if id in socket.assigns.selected_users do + List.delete(socket.assigns.selected_users, id) else - [id_str | socket.assigns.selected_users] + [id | socket.assigns.selected_users] end {:noreply, assign(socket, :selected_users, selected)} @@ -132,8 +129,7 @@ defmodule MvWeb.UserLive.Index do def handle_event("select_all", _params, socket) do users = socket.assigns.users - # Normalize IDs to strings for consistent comparison - all_ids = Enum.map(users, &to_string(&1.id)) + all_ids = Enum.map(users, & &1.id) selected = if Enum.sort(socket.assigns.selected_users) == Enum.sort(all_ids) do diff --git a/lib/mv_web/live/user_live/index.html.heex b/lib/mv_web/live/user_live/index.html.heex index 9314f1e..e7fd72e 100644 --- a/lib/mv_web/live/user_live/index.html.heex +++ b/lib/mv_web/live/user_live/index.html.heex @@ -17,7 +17,7 @@ type="checkbox" name="select_all" phx-click="select_all" - checked={Enum.sort(@selected_users) == Enum.map(@users, &to_string(&1.id)) |> Enum.sort()} + checked={Enum.sort(@selected_users) == Enum.map(@users, & &1.id) |> Enum.sort()} aria-label={gettext("Select all users")} role="checkbox" /> @@ -26,10 +26,10 @@ > <.input type="checkbox" - name={to_string(user.id)} + name={user.id} phx-click="select_user" - phx-value-id={to_string(user.id)} - checked={to_string(user.id) in @selected_users} + phx-value-id={user.id} + checked={user.id in @selected_users} phx-capture-click phx-stop-propagation aria-label={gettext("Select user")} diff --git a/test/membership/group_integration_test.exs b/test/membership/group_integration_test.exs index 8e36119..2333a66 100644 --- a/test/membership/group_integration_test.exs +++ b/test/membership/group_integration_test.exs @@ -79,7 +79,6 @@ defmodule Mv.Membership.GroupIntegrationTest do end end - @moduletag :slow describe "Query Performance" do test "preloading groups with members avoids N+1 queries", %{actor: actor} do # Create test data diff --git a/test/membership/member_available_for_linking_test.exs b/test/membership/member_available_for_linking_test.exs index 079fc4c..5cf9c5b 100644 --- a/test/membership/member_available_for_linking_test.exs +++ b/test/membership/member_available_for_linking_test.exs @@ -130,7 +130,6 @@ defmodule Mv.Membership.MemberAvailableForLinkingTest do end) end - @tag :slow test "limits results to 10 members even when more exist" do system_actor = Mv.Helpers.SystemActor.get_system_actor() diff --git a/test/mv_web/live/group_live/index_test.exs b/test/mv_web/live/group_live/index_test.exs index b972095..7dabcab 100644 --- a/test/mv_web/live/group_live/index_test.exs +++ b/test/mv_web/live/group_live/index_test.exs @@ -115,35 +115,15 @@ defmodule MvWeb.GroupLive.IndexTest do end describe "performance" do - @describetag :slow test "page loads efficiently with many groups", %{conn: conn} do # Create multiple groups Enum.each(1..10, fn _ -> Fixtures.group_fixture() end) - # Count queries using Telemetry - query_count_agent = Agent.start_link(fn -> 0 end) |> elem(1) - - handler = fn _event, _measurements, _metadata, _config -> - Agent.update(query_count_agent, &(&1 + 1)) - end - - handler_id = "test-query-counter-#{System.unique_integer([:positive])}" - :telemetry.attach(handler_id, [:ash, :query, :start], handler, nil) - # Should load without N+1 queries {:ok, _view, html} = live(conn, "/groups") - final_count = Agent.get(query_count_agent, & &1) - :telemetry.detach(handler_id) - # Verify all groups are displayed assert html =~ gettext("Groups") - - # Verify query count is reasonable (should avoid N+1 queries) - # Expected: 1 query for groups list + 1 batch query for member counts + LiveView setup queries - # Allow overhead for authorization, LiveView setup, and other initialization queries - assert final_count <= 12, - "Expected max 12 queries (groups list + batch member counts + LiveView setup + auth), got #{final_count}. This suggests N+1 query problem." end test "member count is loaded efficiently via calculation", %{conn: conn} do @@ -162,29 +142,10 @@ defmodule MvWeb.GroupLive.IndexTest do actor: system_actor ) - # Count queries using Telemetry - query_count_agent = Agent.start_link(fn -> 0 end) |> elem(1) - - handler = fn _event, _measurements, _metadata, _config -> - Agent.update(query_count_agent, &(&1 + 1)) - end - - handler_id = "test-query-counter-#{System.unique_integer([:positive])}" - :telemetry.attach(handler_id, [:ash, :query, :start], handler, nil) - {:ok, _view, html} = live(conn, "/groups") - final_count = Agent.get(query_count_agent, & &1) - :telemetry.detach(handler_id) - # Member count should be displayed (should be 2) assert html =~ "2" or html =~ gettext("Members") or html =~ "Mitglieder" - - # Verify query count is reasonable (member count should be calculated efficiently) - # Expected: 1 query for groups + 1 batch query for member counts + LiveView setup queries - # Allow overhead for authorization, LiveView setup, and other initialization queries - assert final_count <= 12, - "Expected max 12 queries (groups + batch member counts + LiveView setup + auth), got #{final_count}. This suggests inefficient member count calculation." end end end diff --git a/test/mv_web/live/group_live/show_test.exs b/test/mv_web/live/group_live/show_test.exs index bef234b..af298fd 100644 --- a/test/mv_web/live/group_live/show_test.exs +++ b/test/mv_web/live/group_live/show_test.exs @@ -220,7 +220,6 @@ defmodule MvWeb.GroupLive.ShowTest do end describe "performance" do - @describetag :slow test "member list is loaded efficiently (no N+1 queries)", %{conn: conn} do group = Fixtures.group_fixture() @@ -236,32 +235,12 @@ defmodule MvWeb.GroupLive.ShowTest do ) end) - # Count queries using Telemetry - query_count_agent = Agent.start_link(fn -> 0 end) |> elem(1) - - handler = fn _event, _measurements, _metadata, _config -> - Agent.update(query_count_agent, &(&1 + 1)) - end - - handler_id = "test-query-counter-#{System.unique_integer([:positive])}" - :telemetry.attach(handler_id, [:ash, :query, :start], handler, nil) - {:ok, _view, html} = live(conn, "/groups/#{group.slug}") - final_count = Agent.get(query_count_agent, & &1) - :telemetry.detach(handler_id) - # All members should be displayed Enum.each(members, fn member -> assert html =~ member.first_name or html =~ member.last_name end) - - # Verify query count is reasonable (should avoid N+1 queries) - # Expected: 1 query for group lookup + 1 query for members (with preload) + member_count aggregate - # Allow overhead for authorization, LiveView setup, and other initialization queries - # Note: member_count aggregate and authorization checks may add additional queries - assert final_count <= 20, - "Expected max 20 queries (group + members preload + member_count aggregate + LiveView setup + auth), got #{final_count}. This suggests N+1 query problem." end test "slug lookup is efficient (uses unique_slug index)", %{conn: conn} do diff --git a/test/mv_web/live/role_live/show_test.exs b/test/mv_web/live/role_live/show_test.exs index ed099ec..4931058 100644 --- a/test/mv_web/live/role_live/show_test.exs +++ b/test/mv_web/live/role_live/show_test.exs @@ -93,7 +93,6 @@ defmodule MvWeb.RoleLive.ShowTest do %{conn: conn, actor: system_actor} end - @tag :slow test "mounts successfully with valid role ID", %{conn: conn} do role = create_role() @@ -102,7 +101,6 @@ defmodule MvWeb.RoleLive.ShowTest do assert html =~ role.name end - @tag :slow test "displays role name", %{conn: conn} do role = create_role(%{name: "Test Role Name"}) @@ -129,7 +127,6 @@ defmodule MvWeb.RoleLive.ShowTest do assert html =~ gettext("No description") end - @tag :slow test "displays permission set name", %{conn: conn} do role = create_role(%{permission_set_name: "read_only"}) @@ -155,7 +152,6 @@ defmodule MvWeb.RoleLive.ShowTest do assert html =~ gettext("Yes") end - @tag :slow test "displays non-system role badge when is_system_role is false", %{conn: conn} do role = create_role() @@ -182,7 +178,6 @@ defmodule MvWeb.RoleLive.ShowTest do %{conn: conn, actor: system_actor} end - @tag :slow test "back button navigates to role list", %{conn: conn} do role = create_role() diff --git a/test/mv_web/live/role_live_test.exs b/test/mv_web/live/role_live_test.exs index 8cac22a..d3db337 100644 --- a/test/mv_web/live/role_live_test.exs +++ b/test/mv_web/live/role_live_test.exs @@ -98,7 +98,6 @@ defmodule MvWeb.RoleLiveTest do %{conn: conn, actor: system_actor, user: user} end - @tag :slow test "mounts successfully", %{conn: conn} do {:ok, _view, _html} = live(conn, "/admin/roles") end @@ -389,7 +388,6 @@ defmodule MvWeb.RoleLiveTest do %{conn: conn, actor: system_actor, user: user} end - @tag :slow test "deletes non-system role", %{conn: conn} do role = create_role() diff --git a/test/mv_web/live/user_live/show_test.exs b/test/mv_web/live/user_live/show_test.exs index 8f7ea93..9f499e2 100644 --- a/test/mv_web/live/user_live/show_test.exs +++ b/test/mv_web/live/user_live/show_test.exs @@ -23,12 +23,17 @@ defmodule MvWeb.UserLive.ShowTest do end describe "mount and display" do - @tag :ui - test "mounts successfully and displays user information", %{conn: conn, user: user} do + test "mounts successfully with valid user ID", %{conn: conn, user: user} do + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/users/#{user.id}") + + assert html =~ to_string(user.email) + end + + test "displays user email", %{conn: conn, user: user} do conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, ~p"/users/#{user.id}") - # Basic display assert html =~ to_string(user.email) assert html =~ gettext("Email") end @@ -58,7 +63,6 @@ defmodule MvWeb.UserLive.ShowTest do assert html =~ gettext("Not enabled") end - @tag :slow test "displays linked member when present", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() @@ -96,12 +100,10 @@ defmodule MvWeb.UserLive.ShowTest do end describe "navigation" do - @tag :ui - test "navigation buttons work correctly", %{conn: conn, user: user} do + test "back button navigates to user list", %{conn: conn, user: user} do conn = conn_with_oidc_user(conn) {:ok, view, _html} = live(conn, ~p"/users/#{user.id}") - # Back button navigates to user list assert {:error, {:live_redirect, %{to: to}}} = view |> element( @@ -110,8 +112,10 @@ defmodule MvWeb.UserLive.ShowTest do |> render_click() assert to == "/users" + end - # Edit button navigates to edit form + test "edit button navigates to edit form", %{conn: conn, user: user} do + conn = conn_with_oidc_user(conn) {:ok, view, _html} = live(conn, ~p"/users/#{user.id}") assert {:error, {:live_redirect, %{to: to}}} = @@ -125,17 +129,6 @@ defmodule MvWeb.UserLive.ShowTest do end end - describe "page title" do - @tag :ui - test "sets correct page title", %{conn: conn, user: user} do - conn = conn_with_oidc_user(conn) - {:ok, _view, html} = live(conn, ~p"/users/#{user.id}") - - # Check that page title is set (might be in title tag or header) - assert html =~ gettext("Show User") || html =~ to_string(user.email) - end - end - describe "error handling" do test "raises exception for invalid user ID", %{conn: conn} do invalid_id = Ecto.UUID.generate() @@ -149,8 +142,17 @@ defmodule MvWeb.UserLive.ShowTest do end end + describe "page title" do + test "sets correct page title", %{conn: conn, user: user} do + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/users/#{user.id}") + + # Check that page title is set (might be in title tag or header) + assert html =~ gettext("Show User") || html =~ to_string(user.email) + end + end + describe "system actor user" do - @tag :slow test "redirects to user list when viewing system actor user", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() conn = conn_with_oidc_user(conn) diff --git a/test/mv_web/member_live/index_custom_fields_display_test.exs b/test/mv_web/member_live/index_custom_fields_display_test.exs index 9ada92b..1b89b8e 100644 --- a/test/mv_web/member_live/index_custom_fields_display_test.exs +++ b/test/mv_web/member_live/index_custom_fields_display_test.exs @@ -155,7 +155,6 @@ defmodule MvWeb.MemberLive.IndexCustomFieldsDisplayTest do } end - @tag :slow test "displays custom field with show_in_overview: true", %{ conn: conn, member1: _member1, @@ -224,7 +223,6 @@ defmodule MvWeb.MemberLive.IndexCustomFieldsDisplayTest do assert html =~ "true" or html =~ "Yes" or html =~ "Ja" end - @tag :slow test "formats date custom field values correctly", %{conn: conn, member1: _member1} do conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, "/members") @@ -233,7 +231,6 @@ defmodule MvWeb.MemberLive.IndexCustomFieldsDisplayTest do assert html =~ "15.05.1990" end - @tag :slow test "formats email custom field values correctly", %{conn: conn, member1: _member1} do conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, "/members") diff --git a/test/mv_web/member_live/index_custom_fields_edge_cases_test.exs b/test/mv_web/member_live/index_custom_fields_edge_cases_test.exs index cf67fc3..b4ff998 100644 --- a/test/mv_web/member_live/index_custom_fields_edge_cases_test.exs +++ b/test/mv_web/member_live/index_custom_fields_edge_cases_test.exs @@ -12,7 +12,6 @@ defmodule MvWeb.MemberLive.IndexCustomFieldsEdgeCasesTest do alias Mv.Membership.CustomField - @tag :slow test "displays custom field column even when no members have values", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() @@ -46,7 +45,6 @@ defmodule MvWeb.MemberLive.IndexCustomFieldsEdgeCasesTest do assert html =~ field.name end - @tag :slow test "displays very long custom field values correctly", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() @@ -87,7 +85,6 @@ defmodule MvWeb.MemberLive.IndexCustomFieldsEdgeCasesTest do assert html =~ "A" or html =~ long_value end - @tag :slow test "handles multiple custom fields with show_in_overview correctly", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() diff --git a/test/mv_web/member_live/index_member_fields_display_test.exs b/test/mv_web/member_live/index_member_fields_display_test.exs index fe33bb4..c34dbb3 100644 --- a/test/mv_web/member_live/index_member_fields_display_test.exs +++ b/test/mv_web/member_live/index_member_fields_display_test.exs @@ -52,9 +52,7 @@ defmodule MvWeb.MemberLive.IndexMemberFieldsDisplayTest do }) conn = conn_with_oidc_user(conn) - # Use search query to filter to only the expected member (Alice) - # This significantly improves test performance by avoiding loading all members from other tests - {:ok, _view, html} = live(conn, "/members?query=Alice") + {:ok, _view, html} = live(conn, "/members") assert html =~ "Email" assert html =~ m.email diff --git a/test/mv_web/member_live/index_membership_fee_status_test.exs b/test/mv_web/member_live/index_membership_fee_status_test.exs index bbd9159..9994b4e 100644 --- a/test/mv_web/member_live/index_membership_fee_status_test.exs +++ b/test/mv_web/member_live/index_membership_fee_status_test.exs @@ -235,7 +235,6 @@ defmodule MvWeb.MemberLive.IndexMembershipFeeStatusTest do end end - @moduletag :slow describe "performance" do test "loads cycles efficiently without N+1 queries", %{conn: conn} do fee_type = create_fee_type(%{interval: :yearly}) diff --git a/test/mv_web/member_live/index_test.exs b/test/mv_web/member_live/index_test.exs index 3234761..7a7cfa4 100644 --- a/test/mv_web/member_live/index_test.exs +++ b/test/mv_web/member_live/index_test.exs @@ -1787,7 +1787,6 @@ defmodule MvWeb.MemberLive.IndexTest do assert Enum.any?(boolean_fields_after, &(&1.name == "Newly Added Field")) end - @tag :slow test "boolean filter performance with 150 members", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() conn = conn_with_oidc_user(conn) diff --git a/test/mv_web/user_live/form_test.exs b/test/mv_web/user_live/form_test.exs index 48c0238..4b76c19 100644 --- a/test/mv_web/user_live/form_test.exs +++ b/test/mv_web/user_live/form_test.exs @@ -10,26 +10,29 @@ defmodule MvWeb.UserLive.FormTest do end describe "new user form - display" do - @tag :ui - test "shows correct form elements and password field toggling", %{conn: conn} do + test "shows correct form elements", %{conn: conn} do {:ok, view, html} = setup_live_view(conn, "/users/new") - # Basic form elements assert html =~ "New User" assert html =~ "Email" assert html =~ "Set Password" assert has_element?(view, "form#user-form[phx-submit='save']") assert has_element?(view, "input[name='user[email]']") assert has_element?(view, "input[type='checkbox'][name='set_password']") + end + + test "hides password fields initially", %{conn: conn} do + {:ok, view, _html} = setup_live_view(conn, "/users/new") - # Password fields should be hidden initially refute has_element?(view, "input[name='user[password]']") refute has_element?(view, "input[name='user[password_confirmation]']") + end + + test "shows password fields when checkbox toggled", %{conn: conn} do + {:ok, view, _html} = setup_live_view(conn, "/users/new") - # Toggle password fields view |> element("input[name='set_password']") |> render_click() - # Password fields should now be visible assert has_element?(view, "input[name='user[password]']") assert has_element?(view, "input[name='user[password_confirmation]']") assert render(view) =~ "Password requirements" @@ -112,7 +115,7 @@ defmodule MvWeb.UserLive.FormTest do ) assert user.hashed_password != nil - refute is_nil(user.hashed_password) + assert String.starts_with?(user.hashed_password, "$2b$") end end @@ -150,21 +153,22 @@ defmodule MvWeb.UserLive.FormTest do end describe "edit user form - display" do - @tag :ui - test "shows correct form elements and admin password fields", %{conn: conn} do + test "shows correct form elements for existing user", %{conn: conn} do user = create_test_user(%{email: "editme@example.com"}) {:ok, view, html} = setup_live_view(conn, "/users/#{user.id}/edit") - # Basic form elements assert html =~ "Edit User" assert html =~ "Change Password" assert has_element?(view, "input[name='user[email]'][value='editme@example.com']") assert html =~ "Check 'Change Password' above to set a new password for this user" + end + + test "shows admin password fields when enabled", %{conn: conn} do + user = create_test_user(%{email: "editme@example.com"}) + {:ok, view, _html} = setup_live_view(conn, "/users/#{user.id}/edit") - # Toggle admin password fields view |> element("input[name='set_password']") |> render_click() - # Admin password fields should be visible (no confirmation field for admin) assert has_element?(view, "input[name='user[password]']") refute has_element?(view, "input[name='user[password_confirmation]']") assert render(view) =~ "Admin Note" @@ -210,8 +214,7 @@ defmodule MvWeb.UserLive.FormTest do system_actor = Mv.Helpers.SystemActor.get_system_actor() updated_user = Ash.reload!(user, domain: Mv.Accounts, actor: system_actor) assert updated_user.hashed_password != original_password - assert not is_nil(updated_user.hashed_password) - assert updated_user.hashed_password != "" + assert String.starts_with?(updated_user.hashed_password, "$2b$") end end @@ -255,6 +258,39 @@ defmodule MvWeb.UserLive.FormTest do end end + describe "internationalization" do + test "shows German labels", %{conn: conn} do + conn = conn_with_oidc_user(conn, %{email: "admin_de@example.com"}) + conn = Plug.Test.init_test_session(conn, locale: "de") + {:ok, _view, html} = live(conn, "/users/new") + + assert html =~ "Neue*r Benutzer*in" + assert html =~ "E-Mail" + assert html =~ "Passwort setzen" + end + + test "shows English labels", %{conn: conn} do + conn = conn_with_oidc_user(conn, %{email: "admin_en@example.com"}) + Gettext.put_locale(MvWeb.Gettext, "en") + {:ok, _view, html} = live(conn, "/users/new") + + assert html =~ "New User" + assert html =~ "Email" + assert html =~ "Set Password" + end + + test "shows different labels for edit vs new", %{conn: conn} do + user = create_test_user(%{email: "test@example.com"}) + conn = conn_with_oidc_user(conn, %{email: "admin@example.com"}) + + {:ok, _view, new_html} = live(conn, "/users/new") + {:ok, _view, edit_html} = live(conn, "/users/#{user.id}/edit") + + assert new_html =~ "Set Password" + assert edit_html =~ "Change Password" + end + end + describe "member linking - display" do test "shows linked member with unlink button when user has member", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() @@ -385,39 +421,6 @@ defmodule MvWeb.UserLive.FormTest do end end - describe "internationalization" do - @tag :ui - test "shows translated labels in different locales", %{conn: conn} do - # Test German labels - conn = conn_with_oidc_user(conn, %{email: "admin_de@example.com"}) - conn = Plug.Test.init_test_session(conn, locale: "de") - {:ok, _view, html_de} = live(conn, "/users/new") - - assert html_de =~ "Neue*r Benutzer*in" - assert html_de =~ "E-Mail" - assert html_de =~ "Passwort setzen" - - # Test English labels - conn = conn_with_oidc_user(conn, %{email: "admin_en@example.com"}) - conn = Plug.Test.init_test_session(conn, locale: "en") - {:ok, _view, html_en} = live(conn, "/users/new") - - assert html_en =~ "New User" - assert html_en =~ "Email" - assert html_en =~ "Set Password" - - # Test different labels for edit vs new - user = create_test_user(%{email: "test@example.com"}) - conn = conn_with_oidc_user(conn, %{email: "admin@example.com"}) - - {:ok, _view, new_html} = live(conn, "/users/new") - {:ok, _view, edit_html} = live(conn, "/users/#{user.id}/edit") - - assert new_html =~ "Set Password" - assert edit_html =~ "Change Password" - end - end - describe "system actor user" do test "redirects to user list when editing system actor user", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() diff --git a/test/mv_web/user_live/index_test.exs b/test/mv_web/user_live/index_test.exs index 6dbbe3d..a1a02ea 100644 --- a/test/mv_web/user_live/index_test.exs +++ b/test/mv_web/user_live/index_test.exs @@ -3,38 +3,46 @@ defmodule MvWeb.UserLive.IndexTest do import Phoenix.LiveViewTest describe "basic functionality" do - @tag :ui - test "displays users in a table with basic UI elements", %{conn: conn} do + test "shows translated title in German", %{conn: conn} do + conn = conn_with_oidc_user(conn) + conn = Plug.Test.init_test_session(conn, locale: "de") + {:ok, _view, html} = live(conn, "/users") + assert html =~ "Benutzer*innen auflisten" + end + + test "shows translated title in English", %{conn: conn} do + conn = conn_with_oidc_user(conn) + Gettext.put_locale(MvWeb.Gettext, "en") + {:ok, _view, html} = live(conn, "/users") + assert html =~ "Listing Users" + end + + test "shows New User button", %{conn: conn} do + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, "/users") + assert html =~ "New User" + end + + test "displays users in a table", %{conn: conn} do # Create test users - user1 = create_test_user(%{email: "alice@example.com", oidc_id: "alice123"}) + _user1 = create_test_user(%{email: "alice@example.com", oidc_id: "alice123"}) _user2 = create_test_user(%{email: "bob@example.com", oidc_id: "bob456"}) conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, "/users") - # Basic table rendering assert html =~ "alice@example.com" assert html =~ "bob@example.com" - - # UI elements: New User button, action links - assert html =~ "New User" - assert html =~ "Edit" - assert html =~ "Delete" - assert html =~ ~r/href="[^"]*\/users\/#{user1.id}\/edit"/ end - @tag :ui - test "shows translated titles in different locales", %{conn: conn} do - # Test German translation + test "shows correct action links", %{conn: conn} do + user = create_test_user(%{email: "test@example.com"}) conn = conn_with_oidc_user(conn) - conn = Plug.Test.init_test_session(conn, locale: "de") - {:ok, _view, html_de} = live(conn, "/users") - assert html_de =~ "Benutzer*innen auflisten" + {:ok, _view, html} = live(conn, "/users") - # Test English translation - conn = Plug.Test.init_test_session(conn, locale: "en") - {:ok, _view, html_en} = live(conn, "/users") - assert html_en =~ "Listing Users" + assert html =~ "Edit" + assert html =~ "Delete" + assert html =~ ~r/href="[^"]*\/users\/#{user.id}\/edit"/ end end @@ -48,7 +56,6 @@ defmodule MvWeb.UserLive.IndexTest do %{users: [user_a, user_z, user_m]} end - @tag :slow test "initially sorts by email ascending", %{conn: conn} do conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, "/users") @@ -66,7 +73,6 @@ defmodule MvWeb.UserLive.IndexTest do assert mike_pos < zulu_pos, "mike@example.com should appear before zulu@example.com" end - @tag :slow test "can sort email descending by clicking sort button", %{conn: conn} do conn = conn_with_oidc_user(conn) {:ok, view, _html} = live(conn, "/users") @@ -90,22 +96,15 @@ defmodule MvWeb.UserLive.IndexTest do "mike@example.com should appear before alpha@example.com when sorted desc" end - @tag :ui - test "toggles sort direction and shows correct icons", %{conn: conn} do + test "toggles back to ascending when clicking sort button twice", %{conn: conn} do conn = conn_with_oidc_user(conn) {:ok, view, _html} = live(conn, "/users") - # Initially ascending - should show up arrow - html = render(view) - assert html =~ "hero-chevron-up" - - # After clicking, should show down arrow + # Click twice to toggle: asc -> desc -> asc view |> element("button[phx-value-field='email']") |> render_click() - html = render(view) - assert html =~ "hero-chevron-down" - - # Click again to toggle back to ascending html = view |> element("button[phx-value-field='email']") |> render_click() + + # Should be back to ascending assert html =~ "hero-chevron-up" assert html =~ ~s(aria-sort="ascending") @@ -117,6 +116,20 @@ defmodule MvWeb.UserLive.IndexTest do assert alpha_pos < mike_pos, "Should be back to ascending: alpha before mike" assert mike_pos < zulu_pos, "Should be back to ascending: mike before zulu" end + + test "shows sort direction icons", %{conn: conn} do + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, "/users") + + # Initially ascending - should show up arrow + html = render(view) + assert html =~ "hero-chevron-up" + + # After clicking, should show down arrow + view |> element("button[phx-value-field='email']") |> render_click() + html = render(view) + assert html =~ "hero-chevron-down" + end end describe "checkbox selection functionality" do @@ -126,23 +139,24 @@ defmodule MvWeb.UserLive.IndexTest do %{users: [user1, user2]} end - @tag :ui - test "shows checkbox UI elements", %{conn: conn, users: [user1, user2]} do + test "shows select all checkbox", %{conn: conn} do conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, "/users") - # Check select all checkbox exists assert html =~ ~s(name="select_all") assert html =~ ~s(phx-click="select_all") + end + + test "shows individual user checkboxes", %{conn: conn, users: [user1, user2]} do + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, "/users") - # Check individual user checkboxes exist assert html =~ ~s(name="#{user1.id}") assert html =~ ~s(name="#{user2.id}") assert html =~ ~s(phx-click="select_user") end - @tag :ui - test "can select and deselect individual users", %{conn: conn, users: [user1, user2]} do + test "can select individual users", %{conn: conn, users: [user1, user2]} do conn = conn_with_oidc_user(conn) {:ok, view, _html} = live(conn, "/users") @@ -150,31 +164,45 @@ defmodule MvWeb.UserLive.IndexTest do assert view |> element("input[type='checkbox'][name='#{user1.id}']") |> has_element?() assert view |> element("input[type='checkbox'][name='#{user2.id}']") |> has_element?() + # Initially, select_all should not be checked (since no individual items are selected) refute view |> element("input[type='checkbox'][name='select_all'][checked]") |> has_element?() # Select first user checkbox html = view |> element("input[type='checkbox'][name='#{user1.id}']") |> render_click() - assert html =~ "Email" - assert html =~ to_string(user1.email) # The select_all checkbox should still not be checked (not all users selected) refute view |> element("input[type='checkbox'][name='select_all'][checked]") |> has_element?() - # Deselect user - html = view |> element("input[type='checkbox'][name='#{user1.id}']") |> render_click() + # Page should still function normally assert html =~ "Email" + assert html =~ to_string(user1.email) + end + test "can deselect individual users", %{conn: conn, users: [user1, _user2]} do + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, "/users") + + # Select user first + view |> element("input[type='checkbox'][name='#{user1.id}']") |> render_click() + + # Then deselect user + html = view |> element("input[type='checkbox'][name='#{user1.id}']") |> render_click() + + # Select all should not be checked after deselecting individual user refute view |> element("input[type='checkbox'][name='select_all'][checked]") |> has_element?() + + # Page should still function normally + assert html =~ "Email" + assert html =~ to_string(user1.email) end - @tag :ui - test "select all and deselect all functionality", %{conn: conn, users: [user1, user2]} do + test "select all functionality selects all users", %{conn: conn, users: [user1, user2]} do conn = conn_with_oidc_user(conn) {:ok, view, _html} = live(conn, "/users") @@ -199,9 +227,23 @@ defmodule MvWeb.UserLive.IndexTest do |> element("input[type='checkbox'][name='select_all'][checked]") |> has_element?() + # Page should still function normally and show all users assert html =~ "Email" assert html =~ to_string(user1.email) assert html =~ to_string(user2.email) + end + + test "deselect all functionality deselects all users", %{conn: conn, users: [user1, user2]} do + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, "/users") + + # Select all first + view |> element("input[type='checkbox'][name='select_all']") |> render_click() + + # Verify that select_all is checked + assert view + |> element("input[type='checkbox'][name='select_all'][checked]") + |> has_element?() # Then deselect all html = view |> element("input[type='checkbox'][name='select_all']") |> render_click() @@ -219,47 +261,40 @@ defmodule MvWeb.UserLive.IndexTest do |> element("input[type='checkbox'][name='#{user2.id}'][checked]") |> has_element?() + # Page should still function normally assert html =~ "Email" + assert html =~ to_string(user1.email) + assert html =~ to_string(user2.email) end - @tag :slow test "select all automatically checks when all individual users are selected", %{ conn: conn, - users: [_user1, _user2] + users: [user1, user2] } do conn = conn_with_oidc_user(conn) - {:ok, view, html} = live(conn, "/users") + {:ok, view, _html} = live(conn, "/users") - # Get all user IDs from the rendered HTML by finding all checkboxes with phx-click="select_user" - # Extract user IDs from the HTML (they appear as name attributes on checkboxes) - user_ids = - html - |> String.split("phx-click=\"select_user\"") - |> Enum.flat_map(fn part -> - case Regex.run(~r/name="([^"]+)"[^>]*phx-value-id/, part) do - [_, user_id] -> [user_id] - _ -> [] - end - end) - |> Enum.uniq() + # Initially nothing should be checked + refute view + |> element("input[type='checkbox'][name='select_all'][checked]") + |> has_element?() - # Skip if no users found (shouldn't happen, but be safe) - if user_ids != [] do - # Initially nothing should be checked - refute view - |> element("input[type='checkbox'][name='select_all'][checked]") - |> has_element?() + # Select first user + view |> element("input[type='checkbox'][name='#{user1.id}']") |> render_click() + # Select all should still not be checked (only 1 of 2+ users selected) + refute view + |> element("input[type='checkbox'][name='select_all'][checked]") + |> has_element?() - # Select all users one by one - Enum.each(user_ids, fn user_id -> - view |> element("input[type='checkbox'][name='#{user_id}']") |> render_click() - end) + # Select second user + html = view |> element("input[type='checkbox'][name='#{user2.id}']") |> render_click() - # Now select all should be automatically checked (all individual users are selected) - assert view - |> element("input[type='checkbox'][name='select_all'][checked]") - |> has_element?() - end + # Now select all should be automatically checked (all individual users are selected) + # Note: This test might need adjustment based on actual implementation + # The logic depends on whether authenticated user is included in the count + assert html =~ "Email" + assert html =~ to_string(user1.email) + assert html =~ to_string(user2.email) end end @@ -272,12 +307,11 @@ defmodule MvWeb.UserLive.IndexTest do # Confirm user is displayed assert render(view) =~ "delete-me@example.com" - # Click the delete button (phx-click="delete" event) + # Click the first delete button to test the functionality view |> element("tbody tr:first-child a[data-confirm]") |> render_click() - # Verify user was actually deleted (should not appear in HTML anymore) + # The page should still render (basic functionality test) html = render(view) - refute html =~ "delete-me@example.com" # Table header should still be there assert html =~ "Email" end @@ -293,39 +327,52 @@ defmodule MvWeb.UserLive.IndexTest do end describe "navigation" do - @tag :ui - test "navigation links point to correct pages", %{conn: conn} do + test "clicking on user row navigates to user show page", %{conn: conn} do user = create_test_user(%{email: "navigate@example.com"}) conn = conn_with_oidc_user(conn) - {:ok, view, html} = live(conn, "/users") + {:ok, view, _html} = live(conn, "/users") - # Check that user row contains link to show page + # This test would need to check row click behavior + # The actual navigation would happen via JavaScript + html = render(view) assert html =~ ~s(/users/#{user.id}) + end + + test "edit link points to correct edit page", %{conn: conn} do + user = create_test_user(%{email: "edit-me@example.com"}) + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, "/users") - # Check edit link points to correct edit page assert html =~ ~s(/users/#{user.id}/edit) + end + + test "new user button points to correct new page", %{conn: conn} do + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, "/users") - # Check new user button points to correct new page assert html =~ ~s(/users/new) end end describe "translations" do - @tag :ui - test "shows translations for selection in different locales", %{conn: conn} do + test "shows German translations for selection", %{conn: conn} do conn = conn_with_oidc_user(conn) - - # Test German translations conn = Plug.Test.init_test_session(conn, locale: "de") - {:ok, _view, html_de} = live(conn, "/users") - assert html_de =~ "Alle Benutzer*innen auswählen" - assert html_de =~ "Benutzer*in auswählen" + {:ok, _view, html} = live(conn, "/users") - # Test English translations + assert html =~ "Alle Benutzer*innen auswählen" + assert html =~ "Benutzer*in auswählen" + end + + test "shows English translations for selection", %{conn: conn} do + conn = conn_with_oidc_user(conn) Gettext.put_locale(MvWeb.Gettext, "en") - {:ok, _view, html_en} = live(conn, "/users") - # Check that aria-label attributes exist (structure is there) - assert html_en =~ ~s(aria-label=) + {:ok, _view, html} = live(conn, "/users") + + # Note: English translations might be empty strings by default + # This test would verify the structure is there + # Checking that aria-label attributes exist + assert html =~ ~s(aria-label=) end end @@ -380,7 +427,6 @@ defmodule MvWeb.UserLive.IndexTest do end describe "member linking display" do - @tag :slow test "displays linked member name in user list", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() diff --git a/test/seeds_test.exs b/test/seeds_test.exs index 316b99f..67b376e 100644 --- a/test/seeds_test.exs +++ b/test/seeds_test.exs @@ -1,81 +1,44 @@ defmodule Mv.SeedsTest do - @moduledoc """ - Minimal test suite for database seeds. - - This test suite focuses on critical deployment requirements: - - Seeds run without errors (smoke test) - - Seeds are idempotent (can be run multiple times) - - Critical bootstrap invariants are met (Admin user, system roles) - - ## Removed Tests - - The following tests were removed to reduce test execution time. - Their functionality is covered by domain-specific tests: - - - Member/fee type distribution tests → covered by `membership_fees/*_test.exs` - - Cycle status variation tests → covered by `membership_fees/cycle_generator_test.exs` - - Detailed role configuration tests → covered by `authorization/*_test.exs` - - Permission set validation → covered by `authorization/permission_sets_test.exs` - - See `docs/test-optimization-seeds.md` for detailed rationale and coverage mapping. - """ - use Mv.DataCase, async: false require Ash.Query - # Module attribute to track if seeds have been run - # This allows us to run seeds once per test module while respecting sandbox isolation - @seeds_run_key :seeds_test_run - setup do system_actor = Mv.Helpers.SystemActor.get_system_actor() - - # Run seeds once per test process (due to sandbox isolation, each test runs in its own process) - # Process.get/put ensures seeds are only executed once per test process, not once per test - # Note: With async: false and sandbox isolation, this effectively runs seeds once per test, - # but the guard prevents multiple executions within the same test process if setup is called multiple times - unless Process.get(@seeds_run_key) do - Code.eval_file("priv/repo/seeds.exs") - Process.put(@seeds_run_key, true) - end - %{actor: system_actor} end describe "Seeds script" do - @tag :slow - test "runs successfully and creates basic data", %{actor: actor} do - # Smoke test: verify seeds created essential data structures + test "runs successfully without errors", %{actor: actor} do + # Run the seeds script - should not raise any errors + assert Code.eval_file("priv/repo/seeds.exs") + + # Basic smoke test: ensure some data was created {:ok, users} = Ash.read(Mv.Accounts.User, actor: actor) {:ok, members} = Ash.read(Mv.Membership.Member, actor: actor) {:ok, custom_fields} = Ash.read(Mv.Membership.CustomField, actor: actor) - {:ok, roles} = Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) assert not Enum.empty?(users), "Seeds should create at least one user" assert not Enum.empty?(members), "Seeds should create at least one member" assert not Enum.empty?(custom_fields), "Seeds should create at least one custom field" - assert length(roles) >= 5, "Seeds should create at least 5 authorization roles" end - @tag :slow - test "is idempotent when run multiple times", %{actor: actor} do - # Seeds already run once in setup - count current state + test "can be run multiple times (idempotent)", %{actor: actor} do + # Run seeds first time + assert Code.eval_file("priv/repo/seeds.exs") + + # Count records {:ok, users_count_1} = Ash.read(Mv.Accounts.User, actor: actor) {:ok, members_count_1} = Ash.read(Mv.Membership.Member, actor: actor) + {:ok, custom_fields_count_1} = Ash.read(Mv.Membership.CustomField, actor: actor) - {:ok, roles_count_1} = - Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) - - # Run seeds again - should not raise errors and should be idempotent + # Run seeds second time - should not raise errors assert Code.eval_file("priv/repo/seeds.exs") # Count records again - should be the same (upsert, not duplicate) {:ok, users_count_2} = Ash.read(Mv.Accounts.User, actor: actor) {:ok, members_count_2} = Ash.read(Mv.Membership.Member, actor: actor) - - {:ok, roles_count_2} = - Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + {:ok, custom_fields_count_2} = Ash.read(Mv.Membership.CustomField, actor: actor) assert length(users_count_1) == length(users_count_2), "Users count should remain same after re-running seeds" @@ -83,14 +46,136 @@ defmodule Mv.SeedsTest do assert length(members_count_1) == length(members_count_2), "Members count should remain same after re-running seeds" - assert length(roles_count_1) == length(roles_count_2), - "Roles count should remain same after re-running seeds" + assert length(custom_fields_count_1) == length(custom_fields_count_2), + "CustomFields count should remain same after re-running seeds" + end + + test "at least one member has no membership fee type assigned", %{actor: actor} do + # Run the seeds script + assert Code.eval_file("priv/repo/seeds.exs") + + # Get all members + {:ok, members} = Ash.read(Mv.Membership.Member, actor: actor) + + # At least one member should have no membership_fee_type_id + members_without_fee_type = + Enum.filter(members, fn member -> member.membership_fee_type_id == nil end) + + assert not Enum.empty?(members_without_fee_type), + "At least one member should have no membership fee type assigned" + end + + test "each membership fee type has at least one member", %{actor: actor} do + # Run the seeds script + assert Code.eval_file("priv/repo/seeds.exs") + + # Get all fee types and members + {:ok, fee_types} = Ash.read(Mv.MembershipFees.MembershipFeeType, actor: actor) + {:ok, members} = Ash.read(Mv.Membership.Member, actor: actor) + + # Group members by fee type (excluding nil) + members_by_fee_type = + members + |> Enum.filter(&(&1.membership_fee_type_id != nil)) + |> Enum.group_by(& &1.membership_fee_type_id) + + # Each fee type should have at least one member + Enum.each(fee_types, fn fee_type -> + members_for_type = Map.get(members_by_fee_type, fee_type.id, []) + + assert not Enum.empty?(members_for_type), + "Membership fee type #{fee_type.name} should have at least one member assigned" + end) + end + + test "members with fee types have cycles with various statuses", %{actor: actor} do + # Run the seeds script + assert Code.eval_file("priv/repo/seeds.exs") + + # Get all members with fee types + {:ok, members} = Ash.read(Mv.Membership.Member, actor: actor) + + members_with_fee_types = + members + |> Enum.filter(&(&1.membership_fee_type_id != nil)) + + # At least one member should have cycles + assert not Enum.empty?(members_with_fee_types), + "At least one member should have a membership fee type" + + # Check that cycles exist and have various statuses + all_cycle_statuses = + members_with_fee_types + |> Enum.flat_map(fn member -> + Mv.MembershipFees.MembershipFeeCycle + |> Ash.Query.filter(member_id == ^member.id) + |> Ash.read!(actor: actor) + end) + |> Enum.map(& &1.status) + + # At least one cycle should be paid + assert :paid in all_cycle_statuses, "At least one cycle should be paid" + # At least one cycle should be unpaid + assert :unpaid in all_cycle_statuses, "At least one cycle should be unpaid" + # At least one cycle should be suspended + assert :suspended in all_cycle_statuses, "At least one cycle should be suspended" end end - describe "Critical bootstrap invariants" do - test "Admin user has Admin role and can authenticate", %{actor: _actor} do - # Critical: Without this, initial system setup fails + describe "Authorization roles (from seeds)" do + test "creates all 5 authorization roles with correct permission sets" do + # Run seeds once for this test + Code.eval_file("priv/repo/seeds.exs") + {:ok, roles} = Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + + assert length(roles) >= 5, "Should have at least 5 roles" + + # Check each role + role_configs = [ + {"Mitglied", "own_data", true}, + {"Vorstand", "read_only", false}, + {"Kassenwart", "normal_user", false}, + {"Buchhaltung", "read_only", false}, + {"Admin", "admin", false} + ] + + Enum.each(role_configs, fn {name, perm_set, is_system} -> + role = Enum.find(roles, &(&1.name == name)) + assert role, "Role #{name} should exist" + assert role.permission_set_name == perm_set + assert role.is_system_role == is_system + end) + end + + test "Mitglied role is marked as system role" do + Code.eval_file("priv/repo/seeds.exs") + + {:ok, mitglied} = + Mv.Authorization.Role + |> Ash.Query.filter(name == "Mitglied") + |> Ash.read_one(domain: Mv.Authorization, authorize?: false) + + assert mitglied.is_system_role == true + end + + test "all roles have valid permission_set_names" do + Code.eval_file("priv/repo/seeds.exs") + + {:ok, roles} = Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + + valid_sets = + Mv.Authorization.PermissionSets.all_permission_sets() + |> Enum.map(&Atom.to_string/1) + + Enum.each(roles, fn role -> + assert role.permission_set_name in valid_sets, + "Role #{role.name} has invalid permission_set_name: #{role.permission_set_name}" + end) + end + + test "assigns Admin role to ADMIN_EMAIL user" do + Code.eval_file("priv/repo/seeds.exs") + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" {:ok, admin_user} = @@ -98,30 +183,78 @@ defmodule Mv.SeedsTest do |> Ash.Query.filter(email == ^admin_email) |> Ash.read_one(domain: Mv.Accounts, authorize?: false) - assert admin_user != nil, "Admin user must exist after seeds run" + assert admin_user != nil, "Admin user should exist after seeds run" {:ok, admin_user_with_role} = Ash.load(admin_user, :role, domain: Mv.Accounts, authorize?: false) - assert admin_user_with_role.role != nil, "Admin user must have a role assigned" - assert admin_user_with_role.role.name == "Admin", "Admin user must have Admin role" + assert admin_user_with_role.role != nil, "Admin user should have a role assigned" + assert admin_user_with_role.role.name == "Admin" + assert admin_user_with_role.role.permission_set_name == "admin" + end + end - assert admin_user_with_role.role.permission_set_name == "admin", - "Admin role must have admin permission set" + describe "Authorization role assignment" do + test "does not change role of users who already have a role" do + # Seeds once (creates Admin with Admin role) + Code.eval_file("priv/repo/seeds.exs") + + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + {:ok, admin_user} = + Mv.Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) + + assert admin_user != nil, "Admin user should exist after seeds run" + + {:ok, admin_user_with_role} = + Ash.load(admin_user, :role, domain: Mv.Accounts, authorize?: false) + + assert admin_user_with_role.role != nil, "Admin user should have a role assigned" + original_role_id = admin_user_with_role.role_id + assert admin_user_with_role.role.name == "Admin" + + # Seeds again + Code.eval_file("priv/repo/seeds.exs") + + # Admin reloaded + {:ok, admin_reloaded} = + Mv.Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) + + assert admin_reloaded != nil, "Admin user should still exist after re-running seeds" + + {:ok, admin_reloaded_with_role} = + Ash.load(admin_reloaded, :role, domain: Mv.Accounts, authorize?: false) + + assert admin_reloaded_with_role.role != nil, + "Admin user should still have a role after re-running seeds" + + assert admin_reloaded_with_role.role_id == original_role_id + assert admin_reloaded_with_role.role.name == "Admin" end - test "Mitglied system role exists and is correctly configured" do - # Critical: Default role for new users - system breaks without this - {:ok, mitglied} = - Mv.Authorization.Role - |> Ash.Query.filter(name == "Mitglied") - |> Ash.read_one(domain: Mv.Authorization, authorize?: false) + test "role creation is idempotent" do + Code.eval_file("priv/repo/seeds.exs") - assert mitglied != nil, "Mitglied role must exist" - assert mitglied.is_system_role == true, "Mitglied role must be marked as system role" + {:ok, roles_1} = + Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) - assert mitglied.permission_set_name == "own_data", - "Mitglied role must have own_data permission set" + Code.eval_file("priv/repo/seeds.exs") + + {:ok, roles_2} = + Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + + assert length(roles_1) == length(roles_2), + "Role count should remain same after re-running seeds" + + # Each role should appear exactly once + role_names = Enum.map(roles_2, & &1.name) + + assert length(role_names) == length(Enum.uniq(role_names)), + "Each role name should appear exactly once" end end end diff --git a/test/test_helper.exs b/test/test_helper.exs index c6cb1b8..a52775b 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -1,6 +1,2 @@ -ExUnit.start( - # shows 10 slowest tests at the end of the test run - # slowest: 10 -) - +ExUnit.start() Ecto.Adapters.SQL.Sandbox.mode(Mv.Repo, :manual)