Remove dead code and tests that cannot fail #605

New issue

Closed

opened 2026-10-17 10:46:50 +02:00 by simon · 0 comments

simon commented

2026-10-17 10:46:50 +02:00

Owner

Motivation

#598 traced two flaky tests to code nobody used and to assertions that could not fail: the member
:search read action had no production caller, and the HasPermission log tests passed through
log == "" because the message was never captured. A read-only sweep of the codebase found more of
both kinds. Dead code costs maintenance and misleads readers. Tests that cannot fail give false
confidence and, like in #598, can turn into flakes.

Sweep method and full tables: see "Evidence" below. Line numbers are from main at affbefe6;
re-check them when starting the work.

Findings

1. Production code without a caller (high confidence)

Read action CustomFieldValue :by_custom_field_id (lib/mv/membership/custom_field_value.ex:68): no caller in lib, priv or test.
Read action CustomField :prepare_deletion and interface prepare_custom_field_deletion (lib/mv/membership/custom_field.ex:103, membership.ex:67): used only by custom_field_deletion_test.exs. Remove them, or wire them into the delete confirmation if that was the intent.
Code interface overview_members (membership.ex:50): never called; production uses Ash.Query.for_read(:overview). Remove the define; the action stays.
Email confirmation leftovers: Mv.Accounts.User has no confirmation add-on, so these are unreachable:
- SendNewUserConfirmationEmail (lib/mv/accounts/user/senders/send_new_user_confirmation_email.ex)
- confirm_route Mv.Accounts.User, :confirm_new_user (lib/mv_web/router.ex:133-137)
- the {:confirm_new_user, :confirm} and CannotConfirmUnconfirmedUser branches in auth_controller.ex (:41, :196)
- mentions in CODE_GUIDELINES.md and docs/smtp-configuration-concept.md
MvWeb.MemberLive.Index.FieldSelection: save_to_session/2, get_from_cookie/1 and save_to_cookie/2 are used only by tests. get_from_session/1 is called in mount, but nothing writes the "member_field_selection" session key, so it always returns %{}. Check the intent, then remove the unused functions, their tests and the dead session read.
Mv.Vereinfacht.Client.get_contact/1: no caller, no test.
MvWeb.RoleLive.Helpers.permission_set_badge_class/1 (marked deprecated): no caller, no test.
MvWeb.CoreComponents.translate_errors/2 (generator boilerplate): no caller.
Mv.Constants.vereinfacht_required_field?/1: no caller, no test.
Statistics.open_amount_total/1: used only by tests.
CalendarCycles.current_cycle?/2 and last_completed_cycle?/2,3: unused or only called internally (medium; make private or remove).
Hand-written update_member_field_visibility/2 and update_single_member_field_visibility/2 next to code interfaces with the same names: both used only by tests (medium; settle together with decision 1).

1b. Front-end and translations (high confidence unless noted)

assets/js/app.js:671-678: listener for phx:set-input-value. The last push_event for it was removed in eb81d5f7.
Layouts.app attr :current_scope: never passed and never read.
Attrs never passed by any caller (medium): table :row_value_id, button :active, dropdown_menu :menu_class. Remove them, or keep them and document them as API.
priv/gettext/errors.pot (+ de/en errors.po): 4 stale entries.
- The OIDC message "'sub' or 'id'" no longer matches its source (user.ex:499 now says "'sub', 'uid', or 'id'"), so the German translation is never applied. This is a user-visible bug: fix the catalog entry.
- "must consist of 5 digits", "is not a valid phone number" and "cannot be in the future" no longer exist in the source (removed in 056fd04d, 17540c6b, fb59ef99). Remove them.
default.po: 23 (de) / 30 (en) obsolete #~ entries, because just gettext merges with --on-obsolete=mark_as_obsolete (medium). Purge them once, and decide whether the recipe should keep marking or should delete.

2. Tests that cannot fail (high confidence)

Every page that uses Layouts.app renders id="flash-group" and id="client-error", so
html =~ "error" and has_element?(view, "#flash-group") are always true. Substrings such as "0",
"2", "name", "new", "create" always occur in rendered HTML.

test/mv_web/live/member_live/form_error_handling_test.exs: all 5 tests. Assert the specific flash or field error text.
test/mv_web/live/group_live/form_test.exs (7 assertions), group_live/index_test.exs (4), group_live/integration_test.exs:107, member_live/index_test.exs:621: OR chains over such substrings.
test/mv_web/live/global_settings_live_test.exs:139: "shows warning when SMTP is not configured" does not test the warning (its own comment says so).
Assertions that accept every outcome (match?({:ok, _}) or match?({:error, _})): test/mv/membership/import/member_csv_test.exs:39, 76 (stricter siblings exist, delete) and test/mv/helpers/system_actor_test.exs:236 (force the failure path or delete).
Tests with no assertion at all: sort_header_component_test.exs:277, group_live/show_add_member_test.exs:282, group_live/show_authorization_test.exs:135, member_live/index_field_visibility_test.exs:257. Add the missing assertion (e.g. assert_patch) or delete.

3. Weak tests (medium confidence)

11 policy tests "can read … (list)" assert only is_list: group_policies_test.exs (4), membership_fee_type_policies_test.exs (4), membership_fee_cycle_policies_test.exs (3). A policy that filters out every record would still pass. Assert that the setup fixture is in the list.
html =~ "badge" or … / html =~ "true" or … in member_field_live/index_component_test.exs:45 and member_live/index_custom_fields_display_test.exs:228: scope the assertion to an element.
Smoke-only mount tests duplicated by content tests: role_live_test.exs:102, 235, 440.
calendar_cycles_test.exs:82 (only is_boolean, covered by the /3 tests) and csv_parser_test.exs:209 ("module has @moduledoc").

4. Unused test helpers (high confidence)

test/support/filter_builder_helpers.ex: apply_fee_type_filter/3, apply_boolean_filter/3, apply_payment_filter/2..6.
test/support/conn_case.ex: conn_with_admin_user/1.
test/support/fixtures.ex: password_user_with_role_fixture/1.

Decisions needed

Code interfaces without a production caller. No caller at all: list_custom_field_values, update_custom_field_value, destroy_custom_field_value, update_custom_field, destroy_custom_field, list_member_groups, list_users, destroy_user, list_membership_fee_cycles. Used only by tests: destroy_member, create_custom_field_value, update_group, create_register_with_oidc, fee type/cycle create/update/destroy, update_role. Keep them as deliberate domain API, or adopt the rule "interfaces only for production callers"?
Mv.MembershipFees.CycleGenerationJob: no caller and no tests; its moduledoc calls it a skeleton for future Oban use. Remove it, or turn it into a documented admin/release entry point?

Possibly a separate issue

mix join_requests.cleanup_expired is a Mix task. Mix tasks are not available in an OTP release, and Mv.Release has no equivalent. Check how production deletes expired join requests. If nothing does, this is a functional gap, not dead code.

Handed to other issues

These findings sit in code that a planned issue rewrites anyway. They are fixed there, not here. List them in the PR as "handled by #…".

FieldSelection cookie/session functions → #549 (saved views), together with #213, #252, #415. Do not delete them before #549 decides how a view is stored.
button :active → #602. dropdown_menu :menu_class → U8. table :row_value_id → U9. Layouts.app :current_scope → U6.
phx:set-input-value listener → #554.
SMTP warning test in global_settings_live_test.exs → #540.
CustomField :prepare_deletion → U4/U9: use it for the delete dialog with member count, or delete it.
Tests that cannot fail on the member form and the group pages → U4, U5, U9/U10 (their acceptance requires each test to assert the specific text).
CycleGenerationJob → keep until the fee-automation milestone (89) is decided.
mix join_requests.cleanup_expired release entry point → #267.
Obsolete #~ gettext entries and the merge recipe → the roadmap item "reduce gettext merge conflicts".

This issue replaces #225 ("Clean tests").

Scope rule during implementation

The two sweeps below are the complete list for this issue; /analyze verifies and prioritizes, it does not search again. If more instances of the same class turn up in a file the work already touches, fix them here. Anything else goes to a follow-up note, not into this issue.

Out of scope

Already handled in #598: the member :search action, fuzzy_search/2, and the HasPermission log tests.
Private functions (the compiler warns), unused HEEx attr/slot, JS hooks, gettext msgids, migrations: not swept.
Mv.Oidc.Discovery.put_cache/2, clear_cache/0 and SystemActor.invalidate_cache/0: deliberate test seams, keep them.
7 public functions that are only called inside their own module (e.g. classify_smtp_error/1, normalize_base_url/1): turning them into defp is optional cleanup, low value.
Low-confidence attrs that look like intentional API (sortable_table :row_item, input :multiple/:class/:error_class/:checked, :class on theme_swap/spinner).

Acceptance criteria

Every high-confidence finding in sections 1, 1b, 2 and 4 is removed or fixed. The German OIDC error for a missing sub/uid/id claim is translated again. Tests that are rewritten assert something that fails when the behaviour breaks; check each one by breaking the behaviour once locally.
The two decisions are recorded, and the code follows them.
just ci-dev and just typecheck green.
If the cleanup task turns out to be missing in production, a separate issue exists for it.

Evidence

Two read-only sweeps, 2026-10-17, on main at affbefe6.

Ash action inventory via Ash.Resource.Info; a caller grep per action and interface; a module and function inventory with a grep per name (dynamic callers excluded); an AST walk for tests without assertions; a grep for tautological assertion patterns; a usage grep for test/support helpers.
JS hooks registered vs phx-hook usages (15/15 match); function components and their attr/slot usage; gettext source references in default.pot/errors.pot; a call graph from the compiled BEAM files plus mix xref for same-name functions and modules without incoming edges (52 files, 49 referenced from Ash DSL, migrations, config or OTP entry points).

Limits: the test suite was not run and nothing was mutation-tested, so "cannot fail" is derived from the rendered layout. The BEAM graph does not see apply/3 or MFA tuples; callers in priv/config were grep-checked.

## Motivation #598 traced two flaky tests to code nobody used and to assertions that could not fail: the member `:search` read action had no production caller, and the HasPermission log tests passed through `log == ""` because the message was never captured. A read-only sweep of the codebase found more of both kinds. Dead code costs maintenance and misleads readers. Tests that cannot fail give false confidence and, like in #598, can turn into flakes. Sweep method and full tables: see "Evidence" below. Line numbers are from `main` at `affbefe6`; re-check them when starting the work. ## Findings ### 1. Production code without a caller (high confidence) - [ ] Read action `CustomFieldValue :by_custom_field_id` (`lib/mv/membership/custom_field_value.ex:68`): no caller in lib, priv or test. - [ ] Read action `CustomField :prepare_deletion` and interface `prepare_custom_field_deletion` (`lib/mv/membership/custom_field.ex:103`, `membership.ex:67`): used only by `custom_field_deletion_test.exs`. Remove them, or wire them into the delete confirmation if that was the intent. - [ ] Code interface `overview_members` (`membership.ex:50`): never called; production uses `Ash.Query.for_read(:overview)`. Remove the `define`; the action stays. - [ ] Email confirmation leftovers: `Mv.Accounts.User` has no `confirmation` add-on, so these are unreachable: - `SendNewUserConfirmationEmail` (`lib/mv/accounts/user/senders/send_new_user_confirmation_email.ex`) - `confirm_route Mv.Accounts.User, :confirm_new_user` (`lib/mv_web/router.ex:133-137`) - the `{:confirm_new_user, :confirm}` and `CannotConfirmUnconfirmedUser` branches in `auth_controller.ex` (`:41`, `:196`) - mentions in `CODE_GUIDELINES.md` and `docs/smtp-configuration-concept.md` - [ ] `MvWeb.MemberLive.Index.FieldSelection`: `save_to_session/2`, `get_from_cookie/1` and `save_to_cookie/2` are used only by tests. `get_from_session/1` is called in `mount`, but nothing writes the `"member_field_selection"` session key, so it always returns `%{}`. Check the intent, then remove the unused functions, their tests and the dead session read. - [ ] `Mv.Vereinfacht.Client.get_contact/1`: no caller, no test. - [ ] `MvWeb.RoleLive.Helpers.permission_set_badge_class/1` (marked deprecated): no caller, no test. - [ ] `MvWeb.CoreComponents.translate_errors/2` (generator boilerplate): no caller. - [ ] `Mv.Constants.vereinfacht_required_field?/1`: no caller, no test. - [ ] `Statistics.open_amount_total/1`: used only by tests. - [ ] `CalendarCycles.current_cycle?/2` and `last_completed_cycle?/2,3`: unused or only called internally (medium; make private or remove). - [ ] Hand-written `update_member_field_visibility/2` and `update_single_member_field_visibility/2` next to code interfaces with the same names: both used only by tests (medium; settle together with decision 1). ### 1b. Front-end and translations (high confidence unless noted) - [ ] `assets/js/app.js:671-678`: listener for `phx:set-input-value`. The last `push_event` for it was removed in `eb81d5f7`. - [ ] `Layouts.app` attr `:current_scope`: never passed and never read. - [ ] Attrs never passed by any caller (medium): `table :row_value_id`, `button :active`, `dropdown_menu :menu_class`. Remove them, or keep them and document them as API. - [ ] `priv/gettext/errors.pot` (+ `de`/`en` `errors.po`): 4 stale entries. - The OIDC message "'sub' or 'id'" no longer matches its source (`user.ex:499` now says "'sub', 'uid', or 'id'"), so the German translation is never applied. This is a user-visible bug: fix the catalog entry. - "must consist of 5 digits", "is not a valid phone number" and "cannot be in the future" no longer exist in the source (removed in `056fd04d`, `17540c6b`, `fb59ef99`). Remove them. - [ ] `default.po`: 23 (de) / 30 (en) obsolete `#~` entries, because `just gettext` merges with `--on-obsolete=mark_as_obsolete` (medium). Purge them once, and decide whether the recipe should keep marking or should delete. ### 2. Tests that cannot fail (high confidence) Every page that uses `Layouts.app` renders `id="flash-group"` and `id="client-error"`, so `html =~ "error"` and `has_element?(view, "#flash-group")` are always true. Substrings such as `"0"`, `"2"`, `"name"`, `"new"`, `"create"` always occur in rendered HTML. - [ ] `test/mv_web/live/member_live/form_error_handling_test.exs`: all 5 tests. Assert the specific flash or field error text. - [ ] `test/mv_web/live/group_live/form_test.exs` (7 assertions), `group_live/index_test.exs` (4), `group_live/integration_test.exs:107`, `member_live/index_test.exs:621`: OR chains over such substrings. - [ ] `test/mv_web/live/global_settings_live_test.exs:139`: "shows warning when SMTP is not configured" does not test the warning (its own comment says so). - [ ] Assertions that accept every outcome (`match?({:ok, _}) or match?({:error, _})`): `test/mv/membership/import/member_csv_test.exs:39, 76` (stricter siblings exist, delete) and `test/mv/helpers/system_actor_test.exs:236` (force the failure path or delete). - [ ] Tests with no assertion at all: `sort_header_component_test.exs:277`, `group_live/show_add_member_test.exs:282`, `group_live/show_authorization_test.exs:135`, `member_live/index_field_visibility_test.exs:257`. Add the missing assertion (e.g. `assert_patch`) or delete. ### 3. Weak tests (medium confidence) - [ ] 11 policy tests "can read … (list)" assert only `is_list`: `group_policies_test.exs` (4), `membership_fee_type_policies_test.exs` (4), `membership_fee_cycle_policies_test.exs` (3). A policy that filters out every record would still pass. Assert that the setup fixture is in the list. - [ ] `html =~ "badge" or …` / `html =~ "true" or …` in `member_field_live/index_component_test.exs:45` and `member_live/index_custom_fields_display_test.exs:228`: scope the assertion to an element. - [ ] Smoke-only mount tests duplicated by content tests: `role_live_test.exs:102, 235, 440`. - [ ] `calendar_cycles_test.exs:82` (only `is_boolean`, covered by the /3 tests) and `csv_parser_test.exs:209` ("module has @moduledoc"). ### 4. Unused test helpers (high confidence) - [ ] `test/support/filter_builder_helpers.ex`: `apply_fee_type_filter/3`, `apply_boolean_filter/3`, `apply_payment_filter/2..6`. - [ ] `test/support/conn_case.ex`: `conn_with_admin_user/1`. - [ ] `test/support/fixtures.ex`: `password_user_with_role_fixture/1`. ## Decisions needed 1. **Code interfaces without a production caller.** No caller at all: `list_custom_field_values`, `update_custom_field_value`, `destroy_custom_field_value`, `update_custom_field`, `destroy_custom_field`, `list_member_groups`, `list_users`, `destroy_user`, `list_membership_fee_cycles`. Used only by tests: `destroy_member`, `create_custom_field_value`, `update_group`, `create_register_with_oidc`, fee type/cycle create/update/destroy, `update_role`. Keep them as deliberate domain API, or adopt the rule "interfaces only for production callers"? 2. **`Mv.MembershipFees.CycleGenerationJob`**: no caller and no tests; its moduledoc calls it a skeleton for future Oban use. Remove it, or turn it into a documented admin/release entry point? ## Possibly a separate issue - `mix join_requests.cleanup_expired` is a Mix task. Mix tasks are not available in an OTP release, and `Mv.Release` has no equivalent. Check how production deletes expired join requests. If nothing does, this is a functional gap, not dead code. ## Handed to other issues These findings sit in code that a planned issue rewrites anyway. They are fixed there, not here. List them in the PR as "handled by #…". - `FieldSelection` cookie/session functions → #549 (saved views), together with #213, #252, #415. Do not delete them before #549 decides how a view is stored. - `button :active` → #602. `dropdown_menu :menu_class` → U8. `table :row_value_id` → U9. `Layouts.app :current_scope` → U6. - `phx:set-input-value` listener → #554. - SMTP warning test in `global_settings_live_test.exs` → #540. - `CustomField :prepare_deletion` → U4/U9: use it for the delete dialog with member count, or delete it. - Tests that cannot fail on the member form and the group pages → U4, U5, U9/U10 (their acceptance requires each test to assert the specific text). - `CycleGenerationJob` → keep until the fee-automation milestone (89) is decided. - `mix join_requests.cleanup_expired` release entry point → #267. - Obsolete `#~` gettext entries and the merge recipe → the roadmap item "reduce gettext merge conflicts". This issue replaces #225 ("Clean tests"). ## Scope rule during implementation The two sweeps below are the complete list for this issue; `/analyze` verifies and prioritizes, it does not search again. If more instances of the same class turn up in a file the work already touches, fix them here. Anything else goes to a follow-up note, not into this issue. ## Out of scope - Already handled in #598: the member `:search` action, `fuzzy_search/2`, and the HasPermission log tests. - Private functions (the compiler warns), unused HEEx `attr`/`slot`, JS hooks, gettext msgids, migrations: not swept. - `Mv.Oidc.Discovery.put_cache/2`, `clear_cache/0` and `SystemActor.invalidate_cache/0`: deliberate test seams, keep them. - 7 public functions that are only called inside their own module (e.g. `classify_smtp_error/1`, `normalize_base_url/1`): turning them into `defp` is optional cleanup, low value. - Low-confidence attrs that look like intentional API (`sortable_table :row_item`, `input :multiple/:class/:error_class/:checked`, `:class` on `theme_swap`/`spinner`). ## Acceptance criteria - Every high-confidence finding in sections 1, 1b, 2 and 4 is removed or fixed. The German OIDC error for a missing `sub`/`uid`/`id` claim is translated again. Tests that are rewritten assert something that fails when the behaviour breaks; check each one by breaking the behaviour once locally. - The two decisions are recorded, and the code follows them. - `just ci-dev` and `just typecheck` green. - If the cleanup task turns out to be missing in production, a separate issue exists for it. ## Evidence Two read-only sweeps, 2026-10-17, on `main` at `affbefe6`. 1. Ash action inventory via `Ash.Resource.Info`; a caller grep per action and interface; a module and function inventory with a grep per name (dynamic callers excluded); an AST walk for tests without assertions; a grep for tautological assertion patterns; a usage grep for `test/support` helpers. 2. JS hooks registered vs `phx-hook` usages (15/15 match); function components and their `attr`/`slot` usage; gettext source references in `default.pot`/`errors.pot`; a call graph from the compiled BEAM files plus `mix xref` for same-name functions and modules without incoming edges (52 files, 49 referenced from Ash DSL, migrations, config or OTP entry points). Limits: the test suite was not run and nothing was mutation-tested, so "cannot fail" is derived from the rendered layout. The BEAM graph does not see `apply/3` or MFA tuples; callers in `priv`/`config` were grep-checked.

simon added this to the Code and Test Refactoring | TI I milestone

2026-10-17 10:46:50 +02:00

simon added this to the Sprint 18: Oktober 2026 project

2026-10-17 10:46:50 +02:00

simon referenced this issue from a pull request that will close it,

2026-10-19 11:26:36 +02:00

Remove dead code and tests that cannot fail closes #605 #608

simon referenced this issue

2026-10-19 11:33:18 +02:00

Make tests fail when the behaviour they name breaks, #609

simon referenced this issue

2026-10-19 11:37:34 +02:00