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

New issue

Closed

opened 2026-10-19 11:33:18 +02:00 by simon · 0 comments

simon commented

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

Owner

Motivation

#605 removed dead code and hardened every test in the files it touched. Four review rounds kept finding the same two classes of defect in pre-existing tests: assertions that cannot fail, and tests that a stricter sibling already covers. A sweep after #605 found about 100 candidates of the same classes in the test files #605 did not touch (about 45 verified in context). They give false confidence, and like the #598 flakes they can turn into flaky tests.

The rules that would have prevented them are not in CODE_GUIDELINES.md yet. §4.3.4 only says "prefer selector-based assertions", and the examples in §4.3.4 and §4.3.5 show the anti-pattern themselves (assert html =~ "Members", which the sidebar always renders; assert html =~ "input"). So this issue starts with the guideline update and then applies it across the suite.

Part 1 — CODE_GUIDELINES §4 (Testing Standards)

Add to §4.3.4 / §4.7 (wording to be finalized in the PR):

Every test must be able to fail. Before committing a new or changed test, break the behaviour it names once (temporary change, not committed) and see it red. A test that stays green under that break is rewritten or deleted.
No whole-page substring matches. render(view) =~ text also matches the layout: sidebar labels ("Members", "Groups", "Users", "Administration"), #flash-group/#client-error, the locale form, attributes such as required or phx-change="validate", digits in ids and CSS classes, and the random session/CSRF tokens (a token once contained "BOM"). Scope every check to the element: has_element?(view, "#row-#{id} td", text), data-testid, stable ids, ARIA attributes.
Flash and field errors through the helpers. Use assert_flash/3 and assert_field_error/3 (test/support/live_view_assertions.ex); they pin the message to its element, and the coupling to the markup lives in one place.
Exact values, no OR chains. Assert the exact message via gettext/dgettext (with its bindings) or the exact value. a or b where one operand implies the other, or both mean the same, accepts wrong results.
No existence-only or type-only checks. != nil, is_list, Map.has_key?, {:ok, _}/{:error, _}, or length over a list the test built with a fixed size do not check behaviour. Match the specific struct/value; after an authorization call match the specific error (Forbidden vs NotFound). A read that checks a delete needs an actor that may read the record, otherwise the read is denied either way.
List reads assert the fixture. A policy test on a list read asserts that the fixture's id is in the result; result != [] or is_list stays green when a policy filters everything away.
Create/update tests check what was stored, not only the redirect.
Refutes need a positive precondition. Refute a text only after asserting, in the same test, that it is rendered when the behaviour is intact. Refuting a text the app never renders cannot fail.
Names and comments claim only what is asserted. "… and cannot be deleted" must attempt the delete; "verify all groups are displayed" must check every group.
render_change params come from the input's name. For a search input named query, send %{query: "x"}; %{value: "x"} leaves the query empty and the "search" never runs.
One owner per scenario (test economy). Before adding a test, check whether a sibling already covers it with a stronger assertion; delete the weaker one and name the stronger one. Across layers (resource test vs form test on the same action and input) keep one owner — the form test when the rule is surfaced in the UI.
No theme or button classes in assertions (badge-*, btn-*, text-error); they change with the design system.
Replace the two examples in §4.3.4/§4.3.5 with scoped versions.

Part 2 — apply it to the untouched test files

Grep-based sweep after #605 (counts are candidates, each area starts with a full read of its files and a break-proof per changed test):

member_live show/form (show_membership_fees_test, form_membership_fee_type_test, show_groups_display_test, index_groups_integration_test, index_custom_fields_edge_cases_test, membership_fee_integration_test): about 24 — e.g. gettext("Groups") in show_groups_display_test.exs:52/:69/:82/:169/:193/:218 matches the sidebar; "Membership Fees" || … in show_membership_fees_test.exs:59/:256 is the tab label on every show page; "2024" || "date" in form_membership_fee_type_test.exs:213; html =~ "A" or … in index_custom_fields_edge_cases_test.exs:88.
Policies and authorization: about 23 — reads without actor after a delete (user_policies_test.exs:178, member_policies_test.exs:335); {:error, _} without a reason in custom_field_deletion_test (6×), custom_field_value_policies_test (3×), custom_field_live/deletion_test, join_request_approval; tautologies true or :unknown or false in has_permission_test.exs:227/:246; is_list or is_map / not is_nil in has_permission_integration_test.exs:42/:55/:70; role_policies_test.exs:27/:48/:69/:90 never check the fixture role's id.
Membership-fee LiveViews (membership_fee_settings_live_test, membership_fee_type_live/form_test): about 15 — e.g. "3" || "Members" || "Mitglieder" (membership_fee_settings_live_test.exs:67, membership_fee_type_live/form_test.exs:125), || html =~ "required" matches the input attribute (form_test.exs:194).
Users, roles, auth (user_live index/show/form/form_member_dropdown, role_live/show_test, auth_controller_test): about 12 — e.g. sidebar label "Users"/"Benutzer*innen" in user_live/index_test.exs:33/:35; page-title ORs with an always-present operand (role_live/show_test.exs:283, user_live/show_test.exs:135); redirect-only creates (user_live/form_test.exs:43/:53, subsumed by :69/:91; form_member_dropdown_test.exs:74).
Settings and join flow (global_settings_live_test, setting_env_test, join_live_test, join_live_email_failure_test, join_confirm_controller_test): about 9 — e.g. club_name != nil on an allow_nil? false attribute (setting_env_test.exs:16).
Fee and custom-field domain (cycle_generator_edge_cases_test, foreign_key_test, membership_fee_type_test, custom_field_validation_test, custom_field_slug_test, member_groups_relationship_test): about 14 — e.g. leap-year February cycles only check != nil and never cycle_end (cycle_generator_edge_cases_test.exs:462/:490/:517); result == :ok || match?({:error, _}, result) accepts everything (member_groups_relationship_test.exs:215).
Groups and leftovers: the group form's "must be present" check via assert_field_error/3 matches a substring, so a message that only gains a suffix stays green (tighten to the exact message); group_live/show_test (5× redirect || live_redirect without the flash), show_add_remove_members_test.exs:46, duplicate scenarios in show_remove_member_test.exs ("member is removed …" / "… successfully removed (verified in list)"), sort_header_component_test ("shows ascending icon …" is contained in "multiple fields can have different icon states"), badge-variant asserts in member_live/index_test bulk-trigger tests and status_color/1 in membership_fee_helpers_test (theme classes, coordinate with the button/theme work in #602).

Not yet verified (read first): membership_fee_type_live/form_test.exs:69/:89, role_live/show_test.exs:266, auth_controller_test.exs:40/:468, join_live_test.exs:134, join_live_email_failure_test.exs:49, join_confirm_controller_test.exs:74, mailer_test.exs:28, foreign_key_test.exs:124/:176, membership_fee_type_test.exs:168/:204, header_mapper_test.exs:128-129, smtp/config_test.exs:37.

Acceptance criteria

CODE_GUIDELINES.md §4 contains the rules of Part 1, and its LiveView/component examples follow them.
Every listed area is swept in full: each instance is hardened (with a break-proof) or deleted with its stricter sibling named in the PR.
No assertion in the changed files matches whole-page HTML for a text the layout also renders, uses an OR chain over equivalent operands, or checks only existence/type.
just ci-dev and just test --only ui are green.

Notes

Large: about 100 sites in about 45 files. It can be done in one PR per area checkbox under this issue, so the review stays focused.
Reference implementation: the #605 PR (helpers, hardened group/member/role/import tests).

## Motivation #605 removed dead code and hardened every test in the files it touched. Four review rounds kept finding the same two classes of defect in pre-existing tests: assertions that cannot fail, and tests that a stricter sibling already covers. A sweep after #605 found about 100 candidates of the same classes in the test files #605 did not touch (about 45 verified in context). They give false confidence, and like the #598 flakes they can turn into flaky tests. The rules that would have prevented them are not in `CODE_GUIDELINES.md` yet. §4.3.4 only says "prefer selector-based assertions", and the examples in §4.3.4 and §4.3.5 show the anti-pattern themselves (`assert html =~ "Members"`, which the sidebar always renders; `assert html =~ "input"`). So this issue starts with the guideline update and then applies it across the suite. ## Part 1 — CODE_GUIDELINES §4 (Testing Standards) Add to §4.3.4 / §4.7 (wording to be finalized in the PR): - **Every test must be able to fail.** Before committing a new or changed test, break the behaviour it names once (temporary change, not committed) and see it red. A test that stays green under that break is rewritten or deleted. - **No whole-page substring matches.** `render(view) =~ text` also matches the layout: sidebar labels ("Members", "Groups", "Users", "Administration"), `#flash-group`/`#client-error`, the locale form, attributes such as `required` or `phx-change="validate"`, digits in ids and CSS classes, and the random session/CSRF tokens (a token once contained "BOM"). Scope every check to the element: `has_element?(view, "#row-#{id} td", text)`, `data-testid`, stable ids, ARIA attributes. - **Flash and field errors through the helpers.** Use `assert_flash/3` and `assert_field_error/3` (`test/support/live_view_assertions.ex`); they pin the message to its element, and the coupling to the markup lives in one place. - **Exact values, no OR chains.** Assert the exact message via `gettext`/`dgettext` (with its bindings) or the exact value. `a or b` where one operand implies the other, or both mean the same, accepts wrong results. - **No existence-only or type-only checks.** `!= nil`, `is_list`, `Map.has_key?`, `{:ok, _}`/`{:error, _}`, or `length` over a list the test built with a fixed size do not check behaviour. Match the specific struct/value; after an authorization call match the specific error (`Forbidden` vs `NotFound`). A read that checks a delete needs an actor that may read the record, otherwise the read is denied either way. - **List reads assert the fixture.** A policy test on a list read asserts that the fixture's id is in the result; `result != []` or `is_list` stays green when a policy filters everything away. - **Create/update tests check what was stored**, not only the redirect. - **Refutes need a positive precondition.** Refute a text only after asserting, in the same test, that it is rendered when the behaviour is intact. Refuting a text the app never renders cannot fail. - **Names and comments claim only what is asserted.** "… and cannot be deleted" must attempt the delete; "verify all groups are displayed" must check every group. - **`render_change` params come from the input's `name`.** For a search input named `query`, send `%{query: "x"}`; `%{value: "x"}` leaves the query empty and the "search" never runs. - **One owner per scenario (test economy).** Before adding a test, check whether a sibling already covers it with a stronger assertion; delete the weaker one and name the stronger one. Across layers (resource test vs form test on the same action and input) keep one owner — the form test when the rule is surfaced in the UI. - **No theme or button classes in assertions** (`badge-*`, `btn-*`, `text-error`); they change with the design system. - Replace the two examples in §4.3.4/§4.3.5 with scoped versions. ## Part 2 — apply it to the untouched test files Grep-based sweep after #605 (counts are candidates, each area starts with a full read of its files and a break-proof per changed test): - [ ] **member_live show/form** (`show_membership_fees_test`, `form_membership_fee_type_test`, `show_groups_display_test`, `index_groups_integration_test`, `index_custom_fields_edge_cases_test`, `membership_fee_integration_test`): about 24 — e.g. `gettext("Groups")` in `show_groups_display_test.exs:52/:69/:82/:169/:193/:218` matches the sidebar; `"Membership Fees" || …` in `show_membership_fees_test.exs:59/:256` is the tab label on every show page; `"2024" || "date"` in `form_membership_fee_type_test.exs:213`; `html =~ "A" or …` in `index_custom_fields_edge_cases_test.exs:88`. - [ ] **Policies and authorization**: about 23 — reads without actor after a delete (`user_policies_test.exs:178`, `member_policies_test.exs:335`); `{:error, _}` without a reason in `custom_field_deletion_test` (6×), `custom_field_value_policies_test` (3×), `custom_field_live/deletion_test`, `join_request_approval`; tautologies `true or :unknown or false` in `has_permission_test.exs:227/:246`; `is_list or is_map` / `not is_nil` in `has_permission_integration_test.exs:42/:55/:70`; `role_policies_test.exs:27/:48/:69/:90` never check the fixture role's id. - [ ] **Membership-fee LiveViews** (`membership_fee_settings_live_test`, `membership_fee_type_live/form_test`): about 15 — e.g. `"3" || "Members" || "Mitglieder"` (`membership_fee_settings_live_test.exs:67`, `membership_fee_type_live/form_test.exs:125`), `|| html =~ "required"` matches the input attribute (`form_test.exs:194`). - [ ] **Users, roles, auth** (`user_live` index/show/form/form_member_dropdown, `role_live/show_test`, `auth_controller_test`): about 12 — e.g. sidebar label "Users"/"Benutzer*innen" in `user_live/index_test.exs:33/:35`; page-title ORs with an always-present operand (`role_live/show_test.exs:283`, `user_live/show_test.exs:135`); redirect-only creates (`user_live/form_test.exs:43/:53`, subsumed by `:69/:91`; `form_member_dropdown_test.exs:74`). - [ ] **Settings and join flow** (`global_settings_live_test`, `setting_env_test`, `join_live_test`, `join_live_email_failure_test`, `join_confirm_controller_test`): about 9 — e.g. `club_name != nil` on an `allow_nil? false` attribute (`setting_env_test.exs:16`). - [ ] **Fee and custom-field domain** (`cycle_generator_edge_cases_test`, `foreign_key_test`, `membership_fee_type_test`, `custom_field_validation_test`, `custom_field_slug_test`, `member_groups_relationship_test`): about 14 — e.g. leap-year February cycles only check `!= nil` and never `cycle_end` (`cycle_generator_edge_cases_test.exs:462/:490/:517`); `result == :ok || match?({:error, _}, result)` accepts everything (`member_groups_relationship_test.exs:215`). - [ ] **Groups and leftovers**: the group form's "must be present" check via `assert_field_error/3` matches a substring, so a message that only gains a suffix stays green (tighten to the exact message); `group_live/show_test` (5× `redirect || live_redirect` without the flash), `show_add_remove_members_test.exs:46`, duplicate scenarios in `show_remove_member_test.exs` ("member is removed …" / "… successfully removed (verified in list)"), `sort_header_component_test` ("shows ascending icon …" is contained in "multiple fields can have different icon states"), badge-variant asserts in `member_live/index_test` bulk-trigger tests and `status_color/1` in `membership_fee_helpers_test` (theme classes, coordinate with the button/theme work in #602). Not yet verified (read first): `membership_fee_type_live/form_test.exs:69/:89`, `role_live/show_test.exs:266`, `auth_controller_test.exs:40/:468`, `join_live_test.exs:134`, `join_live_email_failure_test.exs:49`, `join_confirm_controller_test.exs:74`, `mailer_test.exs:28`, `foreign_key_test.exs:124/:176`, `membership_fee_type_test.exs:168/:204`, `header_mapper_test.exs:128-129`, `smtp/config_test.exs:37`. ## Acceptance criteria - `CODE_GUIDELINES.md` §4 contains the rules of Part 1, and its LiveView/component examples follow them. - Every listed area is swept in full: each instance is hardened (with a break-proof) or deleted with its stricter sibling named in the PR. - No assertion in the changed files matches whole-page HTML for a text the layout also renders, uses an OR chain over equivalent operands, or checks only existence/type. - `just ci-dev` and `just test --only ui` are green. ## Notes - Large: about 100 sites in about 45 files. It can be done in one PR per area checkbox under this issue, so the review stays focused. - Reference implementation: the #605 PR (helpers, hardened group/member/role/import tests).

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

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

simon added this to the Sprint 18: Oktober 2026 project

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

simon referenced this issue

2026-10-19 13:47:38 +02:00

Run the LiveView test files in parallel #611

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

2026-10-20 14:14:53 +02:00

Make tests fail when the behaviour they name breaks closes #609 #614

simon closed this issue

2026-10-20 14:42:10 +02:00