fix: unblock CI — 2 stale test assertions (#500) #501

Merged

forgejo_admin merged 1 commit from 500-ci-red-fix into main

2026-04-18 05:54:20 +00:00

forgejo_admin commented

2026-04-18 05:48:05 +00:00

Owner

Summary

Unblocks CI on main. Two test assertions had drifted from the current code state and were failing on every pipeline (#500, #501, #502, #503, #504 all red).

Closes #500.

Changes

tests/test_first_payment_email.py — subject assertion updated from "Great First Practice" to "Your First Monthly Payment". The subject copy was changed in commit 2d75115 ("feat: add apology opening to first-payment email (#478)"); the test assertion wasn't updated at the time.
tests/test_westside_streamlit_ro_role.py — migration filename reference updated from 041_add_westside_streamlit_ro_role.py to 044_add_westside_streamlit_ro_role.py. The migration was renumbered to resolve an alembic collision (slot 041 is now 041_add_contract_audit_log.py); the test kept the old filename.

No code changes. No test widening (assertions still check specific substring / specific file).

Why these exist

Discovered while reviewing PR #499 (monthly-fee Payment Links). Woodpecker pipelines #502 (main push) and #503 (PR #499) both showed the same 2 failures with different "passed" counts, meaning every recent PR has either been merged with red CI or ignored it. Per feedback_qa_ci_blockers, CI red masks real regressions under "inherited noise." Fixing now so pipeline #499 can be validated against a green main.

Test Plan

tests/test_first_payment_email.py::test_send_first_payment_email passes locally (was failing on main)
tests/test_westside_streamlit_ro_role.py::test_migration_file_exists passes locally (was failing on main)
CI pipeline green (full suite) after this PR merges

Review Checklist

Both failing tests root-caused (one-line narrative each above)
Fix applied to tests (assertions), not to underlying code — code is correct, tests were stale
No test widening; each assertion still checks a specific substring / specific file
Locally verified both tests now pass
Reviewer confirms full CI suite green on this branch before merge
No new regressions introduced by the fix

Forgejo issue: #500
Blocked from merging cleanly: #499 (monthly-fee Payment Links)
Subject change origin: commit 2d75115 — "feat: add apology opening to first-payment email (#478)"
Migration renumber origin: alembic collision, slot 041 now 041_add_contract_audit_log.py

Memory: feedback_qa_ci_blockers.md — CI red masks real regressions; tests that can't reliably run are blockers, not nits.
Memory: feedback_enterprise_no_workarounds.md — fix the root cause, don't suppress / skip / widen.
No runbook update required — pure test-assertion alignment.

## Summary Unblocks CI on main. Two test assertions had drifted from the current code state and were failing on every pipeline (#500, #501, #502, #503, #504 all red). Closes #500. ## Changes - **`tests/test_first_payment_email.py`** — subject assertion updated from `"Great First Practice"` to `"Your First Monthly Payment"`. The subject copy was changed in commit `2d75115` ("feat: add apology opening to first-payment email (#478)"); the test assertion wasn't updated at the time. - **`tests/test_westside_streamlit_ro_role.py`** — migration filename reference updated from `041_add_westside_streamlit_ro_role.py` to `044_add_westside_streamlit_ro_role.py`. The migration was renumbered to resolve an alembic collision (slot 041 is now `041_add_contract_audit_log.py`); the test kept the old filename. No code changes. No test widening (assertions still check specific substring / specific file). ## Why these exist Discovered while reviewing PR #499 (monthly-fee Payment Links). Woodpecker pipelines #502 (main push) and #503 (PR #499) both showed the same 2 failures with different "passed" counts, meaning every recent PR has either been merged with red CI or ignored it. Per `feedback_qa_ci_blockers`, CI red masks real regressions under "inherited noise." Fixing now so pipeline #499 can be validated against a green main. ## Test Plan - [x] `tests/test_first_payment_email.py::test_send_first_payment_email` passes locally (was failing on main) - [x] `tests/test_westside_streamlit_ro_role.py::test_migration_file_exists` passes locally (was failing on main) - [ ] CI pipeline green (full suite) after this PR merges ## Review Checklist - [x] Both failing tests root-caused (one-line narrative each above) - [x] Fix applied to tests (assertions), not to underlying code — code is correct, tests were stale - [x] No test widening; each assertion still checks a specific substring / specific file - [x] Locally verified both tests now pass - [ ] Reviewer confirms full CI suite green on this branch before merge - [ ] No new regressions introduced by the fix ## Related - Forgejo issue: #500 - Blocked from merging cleanly: #499 (monthly-fee Payment Links) - Subject change origin: commit `2d75115` — "feat: add apology opening to first-payment email (#478)" - Migration renumber origin: alembic collision, slot 041 now `041_add_contract_audit_log.py` ## Related Notes - Memory: `feedback_qa_ci_blockers.md` — CI red masks real regressions; tests that can't reliably run are blockers, not nits. - Memory: `feedback_enterprise_no_workarounds.md` — fix the root cause, don't suppress / skip / widen. - No runbook update required — pure test-assertion alignment.

forgejo_admin added 1 commit

2026-04-18 05:48:05 +00:00

fix: update 2 stale test assertions blocking CI on main

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

960be4434a

Both tests had been failing on main through at least pipelines #500-#504.

1. tests/test_first_payment_email.py: subject copy was changed from
   "Great First Practice" to "Your First Monthly Payment" (commit 2d75115,
   "feat: add apology opening to first-payment email #478"). Test assertion
   was not updated.

2. tests/test_westside_streamlit_ro_role.py: migration file was renumbered
   from 041_ to 044_add_westside_streamlit_ro_role.py (alembic collision
   resolved — 041 is now 041_add_contract_audit_log.py). Test had a
   hardcoded filename reference.

Both fixes align assertions with current code state. No underlying code
changed. No test widening — assertions still check specific substring / file.

Closes #500.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-04-18 05:49:20 +00:00

Author

Owner

PR #501 Review

DOMAIN REVIEW

Stack: Python / pytest. This is a pure test-assertion alignment change — no production code, no migration, no schema, no endpoint, no secrets. Two one-line edits.

Verified both claims against the live tree:

tests/test_first_payment_email.py:101 — Production code at src/basketball_api/services/email.py:1044 emits subject = f"Your First Monthly Payment | {tenant.name}". The stale "Great First Practice" substring assertion cannot possibly pass. Fix is correct.
tests/test_westside_streamlit_ro_role.py:175 — Slot 041 is now occupied by alembic/versions/041_add_contract_audit_log.py; the RO role migration lives at alembic/versions/044_add_westside_streamlit_ro_role.py. Fix matches the renumber.

Grep of the full repo confirms no other stale references to "Great First Practice" or 041_add_westside_streamlit_ro_role remain.

Assertion quality preserved — each still checks a specific substring / specific filename. No widening to assert True, no removing the assertion, no pytest.skip. This is exactly the shape of fix feedback_enterprise_no_workarounds calls for.

BLOCKERS

None.

NITS

The test in test_westside_streamlit_ro_role.py::test_migration_file_exists is a filename-coupling smell — any future alembic renumber collision will break it again. A more durable check would be alembic.script.ScriptDirectory walking revisions and asserting the RO role revision id exists, independent of filename. Not blocking and out of scope here (file a follow-up if desired); the current fix is the right minimum change to unblock CI.
Similarly the subject assertion is tightly coupled to copy. Consider asserting on email_type == EmailType.first_payment on the EmailLog row (already done below) plus a regex like r"First.*Payment" on the subject if copy continues to churn. Again, out of scope.

SOP COMPLIANCE

Branch 500-ci-red-fix follows {issue-number}-{kebab-case-purpose}
PR body has Summary / Changes / Test Plan / Related sections
Closes #500 referenced
No secrets, .env, or credentials committed
Scope is tight — 2 files, +2/-2 lines, zero scope creep
Both failures root-caused with one-line narrative each
Honors feedback_qa_ci_blockers — CI red treated as a blocker, not a nit
Honors feedback_enterprise_no_workarounds — fixes the assertion drift at the root, no skips or widening

One open checkbox in the PR body is CI pipeline green (full suite) after this PR merges — that is the post-merge validation gate, not a pre-merge blocker. Reviewer should confirm the pipeline on this branch passes the previously-failing two tests before merge (the stated Review Checklist item).

PROCESS OBSERVATIONS

Positive DORA impact: unblocks MTTR on every other PR by restoring CI signal. Pipelines #502 / #503 / #504 have been merging under red, which means inherited noise has been masking real regressions. This PR flips the signal back on — highest-leverage 2-line change in recent memory.
Change failure risk: near-zero. Test-only change, assertions narrowed to match current production behavior.
Documentation gap: once CI is green again, the team should backfill why tests weren't updated in #478 (subject change) and the 041→044 alembic renumber. Both are one-line follow-ups — not required here. The PR body already links the commit and the collision, which is enough traceability.
Branch hygiene note: #468, #469, #470, #472 are still open with overlapping titles around skip_proration / first-payment. Worth a board sweep after this merges so green CI isn't wasted on stale branches.

VERDICT: APPROVED

## PR #501 Review ### DOMAIN REVIEW Stack: Python / pytest. This is a pure test-assertion alignment change — no production code, no migration, no schema, no endpoint, no secrets. Two one-line edits. Verified both claims against the live tree: 1. **`tests/test_first_payment_email.py:101`** — Production code at `src/basketball_api/services/email.py:1044` emits `subject = f"Your First Monthly Payment | {tenant.name}"`. The stale `"Great First Practice"` substring assertion cannot possibly pass. Fix is correct. 2. **`tests/test_westside_streamlit_ro_role.py:175`** — Slot 041 is now occupied by `alembic/versions/041_add_contract_audit_log.py`; the RO role migration lives at `alembic/versions/044_add_westside_streamlit_ro_role.py`. Fix matches the renumber. Grep of the full repo confirms no other stale references to `"Great First Practice"` or `041_add_westside_streamlit_ro_role` remain. Assertion quality preserved — each still checks a specific substring / specific filename. No widening to `assert True`, no removing the assertion, no `pytest.skip`. This is exactly the shape of fix `feedback_enterprise_no_workarounds` calls for. ### BLOCKERS None. ### NITS - The test in `test_westside_streamlit_ro_role.py::test_migration_file_exists` is a filename-coupling smell — any future alembic renumber collision will break it again. A more durable check would be `alembic.script.ScriptDirectory` walking revisions and asserting the RO role revision id exists, independent of filename. Not blocking and out of scope here (file a follow-up if desired); the current fix is the right minimum change to unblock CI. - Similarly the subject assertion is tightly coupled to copy. Consider asserting on `email_type == EmailType.first_payment` on the `EmailLog` row (already done below) plus a regex like `r"First.*Payment"` on the subject if copy continues to churn. Again, out of scope. ### SOP COMPLIANCE - [x] Branch `500-ci-red-fix` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has Summary / Changes / Test Plan / Related sections - [x] Closes #500 referenced - [x] No secrets, `.env`, or credentials committed - [x] Scope is tight — 2 files, +2/-2 lines, zero scope creep - [x] Both failures root-caused with one-line narrative each - [x] Honors `feedback_qa_ci_blockers` — CI red treated as a blocker, not a nit - [x] Honors `feedback_enterprise_no_workarounds` — fixes the assertion drift at the root, no skips or widening One open checkbox in the PR body is `CI pipeline green (full suite) after this PR merges` — that is the post-merge validation gate, not a pre-merge blocker. Reviewer should confirm the pipeline on this branch passes the previously-failing two tests before merge (the stated Review Checklist item). ### PROCESS OBSERVATIONS - Positive DORA impact: unblocks MTTR on every other PR by restoring CI signal. Pipelines #502 / #503 / #504 have been merging under red, which means inherited noise has been masking real regressions. This PR flips the signal back on — highest-leverage 2-line change in recent memory. - Change failure risk: near-zero. Test-only change, assertions narrowed to match current production behavior. - Documentation gap: once CI is green again, the team should backfill why tests weren't updated in #478 (subject change) and the 041→044 alembic renumber. Both are one-line follow-ups — not required here. The PR body already links the commit and the collision, which is enough traceability. - Branch hygiene note: #468, #469, #470, #472 are still open with overlapping titles around `skip_proration` / first-payment. Worth a board sweep after this merges so green CI isn't wasted on stale branches. ### VERDICT: APPROVED