diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 00000000..8e2f8abd --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,60 @@ +name: Tests + +# Runs the Django test suite on every push to master (our test-server deploy +# trigger) and on every pull request. A failing run is a red ✗ + email — it +# reports status only and does not block the push or the deploy. +on: + push: + branches: [master] + pull_request: + +jobs: + test: + runs-on: ubuntu-latest + + # Postgres service mirrors the local-dev / server `db` container + # (postgres:16, makeability / admin / password). + services: + postgres: + image: postgres:16 + env: + POSTGRES_DB: makeability + POSTGRES_USER: admin + POSTGRES_PASSWORD: password + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U admin -d makeability" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + + env: + # settings_test.py reads these to reach the Postgres service above. + DATABASE_HOST: localhost + DATABASE_PORT: 5432 + DJANGO_ENV: DEBUG + + steps: + - uses: actions/checkout@v4 + + # Mirror the Dockerfile's system deps: ImageMagick + Ghostscript power the + # PDF→thumbnail path that Artifact.save() runs (exercised by the Talk + # fixtures); libpq-dev is needed to build psycopg2 from source. + - name: Install system dependencies + run: | + sudo apt-get update + sudo apt-get install -y --no-install-recommends imagemagick ghostscript libpq-dev + sudo cp imagemagick-policy.xml /etc/ImageMagick-6/policy.xml + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.13" + cache: pip + + - name: Install Python dependencies + run: pip install -r requirements.txt + + - name: Run tests + run: python manage.py test website --settings=makeabilitylab.settings_test --verbosity=2 diff --git a/CLAUDE.md b/CLAUDE.md index 67f2b6d1..4d68a492 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,11 +37,12 @@ A superuser is required to use `/admin` and add content; create one with `python ## Tests and accessibility checks -- Tests: `python manage.py test website` (inside container). The suite has two styles, both in `website/tests.py`: +- Tests: `python manage.py test website --settings=makeabilitylab.settings_test` (inside container). The tests live in the `website/tests/` package (one `test_*.py` per concern; Django auto-discovers them) with shared DB fixtures in `website/tests/base.py`. The suite has two styles: - **Unit** — `SimpleTestCase` + `MagicMock` for pure logic (formatters, BibTeX generation, etc.); no DB, runs in ms. - - **Integration** — `DatabaseTestCase` (subclass of Django's `TestCase`) for view / queryset / template regressions; each test runs in a transaction and rolls back. Has fixture helpers `make_person` / `make_publication` / `make_news_item`. + - **Integration** — `DatabaseTestCase` (subclass of Django's `TestCase`, in `tests/base.py`) for view / queryset / template regressions; each test runs in a transaction and rolls back. Has fixture helpers `make_person` / `make_publication` / `make_talk` / `make_news_item`. - When fixing a bug reachable through a real queryset, URL, or view, add a regression test in the matching style before applying the fix (matches the tests-first workflow). - - **Gotcha:** `website/migrations/` is gitignored, so each env has its own history. If `manage.py test` fails at DB creation with `column "..." already exists`, drop the stale test DB with `docker exec makeabilitylabwebsite-db-1 psql -U admin -d postgres -c "DROP DATABASE IF EXISTS test_makeability;"` and re-run. See #1267 for the durable fix. + - **Always use the `--settings=makeabilitylab.settings_test` shim.** It sets `MIGRATION_MODULES = {'website': None}` so the test DB is built directly from the current models, sidestepping the gitignored, per-environment `website/migrations/` history. This is the durable fix for #1267 — without it, a fresh test DB can fail at creation with `column "..." already exists` (old workaround: `docker exec makeabilitylabwebsite-db-1 psql -U admin -d postgres -c "DROP DATABASE IF EXISTS test_makeability;"`). + - **CI:** `.github/workflows/test.yml` runs this same command on every push to `master` and every PR (free/unlimited for this public repo). It reports a green ✓ / red ✗ — it does not block pushes or the deploy. See the testing roadmap in #1278. - Accessibility (Pa11y CI + Axe, WCAG 2.0 AA): start the site, then `docker-compose -f docker-compose-local-dev.yml --profile testing run --rm a11y`. URLs to scan are configured in `.pa11yci.json`. Run this before submitting UI changes. ## Deployment diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bafd6eaf..1b9c7f67 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -249,20 +249,22 @@ A superuser account is required to access the Django admin interface and add con ## Running the Test Suite -The Python test suite lives in `website/tests.py` and runs inside the website container: +The Python test suite lives in the `website/tests/` package (one `test_*.py` module per concern — Django auto-discovers them) and runs inside the website container: ```bash -docker exec makeabilitylabwebsite-website-1 python manage.py test website +docker exec makeabilitylabwebsite-website-1 python manage.py test website --settings=makeabilitylab.settings_test ``` +**Always pass `--settings=makeabilitylab.settings_test`** (see [Troubleshooting tests](#troubleshooting-tests) for why). The same command runs automatically in CI on every push to `master` and every PR — see [Continuous integration](#continuous-integration). + The suite has two complementary styles: | Style | Base class | What it's for | |---|---|---| | **Unit** | `SimpleTestCase` + `MagicMock` | Pure logic — formatters, BibTeX generation, single-method behavior. No DB; runs in milliseconds. | -| **Integration** | `DatabaseTestCase` (subclass of Django's `TestCase`) | View, queryset, template, and URL-routing regressions. Each test runs inside a transaction that is rolled back, so tests stay isolated. | +| **Integration** | `DatabaseTestCase` (subclass of Django's `TestCase`, in `website/tests/base.py`) | View, queryset, template, and URL-routing regressions. Each test runs inside a transaction that is rolled back, so tests stay isolated. | -The `DatabaseTestCase` base provides `make_person`, `make_publication`, and `make_news_item` helpers built on plain `Model.objects.create()` — use those rather than hand-rolling fixtures. +The `DatabaseTestCase` base provides `make_person`, `make_publication`, `make_talk`, and `make_news_item` helpers built on plain `Model.objects.create()` — use those rather than hand-rolling fixtures. ### When to add a test @@ -276,13 +278,19 @@ If a fix is genuinely not unit-testable (FD leaks, `super().save()`-dependent pa ### Troubleshooting tests -`website/migrations/` is **gitignored** — each environment (your laptop, test, production) maintains its own migration history on disk. This sometimes drifts. If `manage.py test` fails at test-DB creation, the symptoms and fixes are: +`website/migrations/` is **gitignored** — each environment (your laptop, test, production) maintains its own migration history on disk, which can drift. The `--settings=makeabilitylab.settings_test` shim is the durable fix (#1267): it sets `MIGRATION_MODULES = {'website': None}`, so the test runner builds the `website` schema directly from the current models instead of replaying that local history. **Use the shim and these symptoms shouldn't appear at all.** + +If you forget the shim and the legacy `manage.py test website` fails at test-DB creation: -- **`database "test_makeability" already exists`** — a prior failed run left it half-built. Drop and retry: +- **`database "test_makeability" already exists`** — a prior failed run left it half-built. Drop and retry (or just switch to the shim): ```bash docker exec makeabilitylabwebsite-db-1 psql -U admin -d postgres -c "DROP DATABASE IF EXISTS test_makeability;" ``` -- **`column "..." of relation "..." already exists`** — a local migration file duplicates a field that a later `0001_initial` regeneration already includes. Same fix (drop the test DB) usually clears it; if it persists, the offending migration is a local stale artifact that needs to be edited or removed. See [#1267](https://github.com/makeabilitylab/makeabilitylabwebsite/issues/1267) for the durable fix (test-only settings shim using `MIGRATION_MODULES`). +- **`column "..." of relation "..." already exists`** — a local migration file duplicates a field that a later `0001_initial` regeneration already includes. The shim sidesteps this entirely. + +### Continuous integration + +`.github/workflows/test.yml` runs the suite (with the test-settings shim, against a Postgres 16 service container) on every push to `master` and every pull request. GitHub Actions is free and unlimited for this public repo. A failing run shows a red ✗ on the commit/PR and emails the author — it **reports** status, it does not block the push or the test-server deploy. The broader testing roadmap (coverage, Pa11y-in-CI, test backfill) is tracked in [#1278](https://github.com/makeabilitylab/makeabilitylabwebsite/issues/1278). ## Accessibility Testing @@ -317,7 +325,7 @@ Edit `.pa11yci.json` to add or remove URLs to test. The `urls` array lists every - **One issue per branch**: Keep PRs focused on a single issue for easier review. -- **Run the test suite**: `docker exec makeabilitylabwebsite-website-1 python manage.py test website` should pass before opening a PR. If your fix is reachable through a real queryset, view, or template, add a regression test (see [Running the Test Suite](#running-the-test-suite)). +- **Run the test suite**: `docker exec makeabilitylabwebsite-website-1 python manage.py test website --settings=makeabilitylab.settings_test` should pass before opening a PR (CI runs the same command). If your fix is reachable through a real queryset, view, or template, add a regression test (see [Running the Test Suite](#running-the-test-suite)). - **Test locally**: Verify your changes work in the browser before submitting. diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index c92e1588..b065333e 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -97,7 +97,7 @@ View current and past versions on the [Releases page](https://github.com/makeabi 2. Confirm the Python test suite passes locally: ```bash - docker exec makeabilitylabwebsite-website-1 python manage.py test website + docker exec makeabilitylabwebsite-website-1 python manage.py test website --settings=makeabilitylab.settings_test ``` (See [Running the Test Suite](../CONTRIBUTING.md#running-the-test-suite) in `CONTRIBUTING.md` for what the suite covers and how to add to it.) diff --git a/makeabilitylab/settings_test.py b/makeabilitylab/settings_test.py new file mode 100644 index 00000000..fede8304 --- /dev/null +++ b/makeabilitylab/settings_test.py @@ -0,0 +1,45 @@ +""" +Test-only Django settings. + +Run with: + python manage.py test website --settings=makeabilitylab.settings_test + +Why this exists +--------------- +``website/migrations/`` is gitignored, so every environment (laptop, CI, the +servers) carries its own migration history. That drift intermittently breaks a +fresh test-DB build with ``column "..." already exists`` (see #1267 and +CLAUDE.md). Setting ``MIGRATION_MODULES = {'website': None}`` tells Django to +ignore the website app's migration history entirely and build its tables +directly from the current models during test-DB setup (run_syncdb), which is +both reproducible across environments and the durable fix for that flakiness. + +Only the *website* app is affected; third-party apps (admin, auth, ckeditor, +sortedm2m, easy_thumbnails, image_cropping, ...) keep their shipped migrations. +""" +import os + +from makeabilitylab.settings import * # noqa: F401,F403 + +# Build website tables from models instead of replaying gitignored migrations. +MIGRATION_MODULES = {"website": None} + +# Let CI point the database at its Postgres service container. Locally (inside +# the website container) these env vars are unset, so we inherit HOST='db' from +# the base settings fallback; CI sets DATABASE_HOST=localhost. +DATABASES["default"]["HOST"] = os.environ.get( # noqa: F405 + "DATABASE_HOST", DATABASES["default"]["HOST"] # noqa: F405 +) +DATABASES["default"]["PORT"] = os.environ.get( # noqa: F405 + "DATABASE_PORT", DATABASES["default"].get("PORT", "5432") # noqa: F405 +) + +# The base settings wire a RotatingFileHandler to /code/media/debug.log (a +# container path). Django evaluates LOGGING at startup, so on any host without +# that directory — a CI runner, a fresh checkout — django.setup() crashes +# before a single test runs. Swap just the 'file' handler for a no-op; this +# keeps every logger's handler reference valid while never touching disk. +LOGGING["handlers"]["file"] = {"class": "logging.NullHandler"} # noqa: F405 + +# Speed up the auth tests (Data Health suite creates real superuser rows). +PASSWORD_HASHERS = ["django.contrib.auth.hashers.MD5PasswordHasher"] diff --git a/website/tests.py b/website/tests.py deleted file mode 100644 index 589f2785..00000000 --- a/website/tests.py +++ /dev/null @@ -1,1578 +0,0 @@ -""" -Tests for the website app. - -Currently covers website.utils.bio_utils.auto_generate_bio. Tests use mocks -rather than real Person/Position records so they don't have to navigate -Person.save() side-effects (random image assignment, file I/O); the unit -under test is pure presentation logic. -""" - -from datetime import date, timedelta -from unittest.mock import MagicMock, patch - -from django.test import SimpleTestCase - -from website.utils.bio_utils import ( - _join_with_oxford_comma, - auto_generate_bio, - humanize_duration, -) - - -# --- Mock fixtures --------------------------------------------------------- - - -class FakeQuerySet: - """Minimal stand-in for a Django QuerySet with the methods auto-bio uses.""" - - def __init__(self, items): - self._items = list(items) - - def count(self): - return len(self._items) - - def __iter__(self): - return iter(self._items) - - def __getitem__(self, key): - return self._items[key] - - -def _make_person( - *, - first_name="Jon", - last_name="Doe", - url_name="jondoe", - has_started=True, - is_current_member=False, - is_alumni_member=False, - is_current_collaborator=False, - is_past_collaborator=False, - is_active=False, - current_title="PhD Student", - latest_position=None, - publications=0, - projects=None, - mentors=None, - mentees=None, - total_time_as_member=None, -): - """Build a mock Person exposing only the attributes auto_generate_bio reads.""" - # If the caller indicates the person has started, default to a non-None - # latest_position so the "no positions at all" early-return doesn't - # short-circuit the role-branch tests. Callers exercising the no-position - # case pass has_started=False and leave latest_position as None. - if latest_position is None and has_started: - latest_position = _make_position( - start_date=date(2020, 1, 1), title=current_title - ) - - person = MagicMock(name=f"Person({first_name} {last_name})") - person.first_name = first_name - person.last_name = last_name - person.get_full_name.return_value = f"{first_name} {last_name}" - person.get_url_name.return_value = url_name - person.has_started = has_started - person.is_current_member = is_current_member - person.is_alumni_member = is_alumni_member - person.is_current_collaborator = is_current_collaborator - person.is_past_collaborator = is_past_collaborator - person.is_active = is_active - person.get_current_title = current_title - person.get_latest_position = latest_position - person.get_total_time_as_member = total_time_as_member - person.get_projects = projects or [] - - publication_set = MagicMock() - publication_set.exists.return_value = publications > 0 - publication_set.count.return_value = publications - person.publication_set = publication_set - - person.get_grad_mentors.return_value = FakeQuerySet(mentors or []) - person.get_mentees.return_value = FakeQuerySet(mentees or []) - return person - - -def _make_link_person(first_name, last_name, url_name): - p = MagicMock() - p.get_full_name.return_value = f"{first_name} {last_name}" - p.get_url_name.return_value = url_name - return p - - -def _make_project(name, short_name): - p = MagicMock() - p.name = name - p.short_name = short_name - return p - - -def _make_position(start_date=None, end_date=None, title="PhD Student"): - pos = MagicMock() - pos.start_date = start_date - pos.end_date = end_date - pos.title = title - return pos - - -# --- Role-sentence reachability matrix ------------------------------------- - - -class RoleSentenceTests(SimpleTestCase): - """One test per branch of _role_sentence.""" - - def test_no_position_no_publication_returns_empty(self): - """ - Regression for #1258: a Person with no Position records AND no - publications must produce an empty bio, not "will be joining…". - """ - person = _make_person(has_started=False, publications=0) - self.assertEqual(auto_generate_bio(person), "") - - def test_no_position_with_publication_says_has_published(self): - person = _make_person(has_started=False, publications=3) - bio = auto_generate_bio(person) - self.assertIn("Jon Doe has published with the Makeability Lab.", bio) - # Contributions sentence follows. - self.assertIn("3 publications", bio) - - def test_future_member_says_will_be_joining_with_pretty_date(self): - future_pos = _make_position(start_date=date(2026, 9, 15)) - person = _make_person(has_started=False, latest_position=future_pos) - bio = auto_generate_bio(person) - self.assertEqual( - bio, - "Jon Doe will be joining the Makeability Lab on Sep 2026.", - ) - - def test_current_member_with_duration(self): - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - total_time_as_member=timedelta(days=int(365 * 3 + 180)), - ) - bio = auto_generate_bio(person) - self.assertIn( - "Jon Doe is currently a PhD Student in the Makeability Lab.", bio - ) - self.assertIn("Jon has been in the lab for", bio) - self.assertIn("years.", bio) - - def test_current_member_without_duration_omits_duration_sentence(self): - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - total_time_as_member=None, - ) - bio = auto_generate_bio(person) - self.assertEqual( - bio, "Jon Doe is currently a PhD Student in the Makeability Lab." - ) - - def test_current_member_ms_uses_an_article(self): - person = _make_person( - is_current_member=True, - is_active=True, - current_title="MS Student", - total_time_as_member=None, - ) - bio = auto_generate_bio(person) - self.assertIn("is currently an MS Student", bio) - - @patch("website.utils.bio_utils._get_earliest_member_position") - @patch("website.utils.bio_utils._get_latest_member_position") - def test_alumni_member_branch(self, mock_latest, mock_earliest): - pos = _make_position( - start_date=date(2018, 9, 1), - end_date=date(2024, 3, 15), - title="PhD Student", - ) - mock_latest.return_value = pos - mock_earliest.return_value = pos - person = _make_person( - is_alumni_member=True, - current_title="PhD Student", - total_time_as_member=timedelta(days=int(365 * 5.5)), - ) - bio = auto_generate_bio(person) - self.assertIn("Jon Doe was a PhD Student in the Makeability Lab", bio) - self.assertIn("(Sep 2018 to Mar 2024).", bio) - # Duration appears between the title and the date range. - self.assertIn("years (Sep 2018 to Mar 2024).", bio) - - @patch("website.utils.bio_utils._get_earliest_member_position") - @patch("website.utils.bio_utils._get_latest_member_position") - def test_alumni_member_now_collaborator_gets_two_sentences( - self, mock_latest, mock_earliest - ): - """ - Pre-fix bug: a former member who later became a current collaborator - was described as "was a Collaborator … (… to present)". Now the - first sentence anchors on the latest MEMBER position, and the - current collaborator status is a separate trailing sentence. - """ - pos = _make_position( - start_date=date(2018, 9, 1), - end_date=date(2024, 3, 15), - title="PhD Student", - ) - mock_latest.return_value = pos - mock_earliest.return_value = pos - person = _make_person( - is_alumni_member=True, - is_current_collaborator=True, - current_title="Collaborator", - total_time_as_member=timedelta(days=int(365 * 5.5)), - ) - bio = auto_generate_bio(person) - self.assertIn( - "Jon Doe was a PhD Student in the Makeability Lab", bio - ) - self.assertIn("(Sep 2018 to Mar 2024).", bio) - self.assertIn( - "Jon is currently a collaborator with the Makeability Lab.", bio - ) - # The first sentence must NOT say "Collaborator" (the title attribute - # of get_latest_position would say that — bug we're guarding against). - self.assertNotIn("was a Collaborator", bio) - self.assertNotIn("to present", bio) - - def test_current_collaborator(self): - person = _make_person( - is_current_collaborator=True, is_active=True - ) - bio = auto_generate_bio(person) - self.assertEqual( - bio, "Jon Doe is a collaborator with the Makeability Lab." - ) - - def test_past_collaborator(self): - person = _make_person(is_past_collaborator=True) - bio = auto_generate_bio(person) - self.assertEqual( - bio, "Jon Doe was a collaborator with the Makeability Lab." - ) - - -# --- Contributions sentence ------------------------------------------------ - - -class ContributionsSentenceTests(SimpleTestCase): - """Shape of the projects/publications sentence.""" - - def test_pubs_only_singular(self): - person = _make_person(has_started=False, publications=1) - bio = auto_generate_bio(person) - self.assertIn("They contributed to 1 publication.", bio) - - def test_pubs_only_plural(self): - person = _make_person(has_started=False, publications=5) - bio = auto_generate_bio(person) - self.assertIn("They contributed to 5 publications.", bio) - - def test_one_project_no_pubs(self): - proj = _make_project("Sound Watch", "soundwatch") - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - projects=[proj], - ) - bio = auto_generate_bio(person) - self.assertIn("They contributed to a project called Sound Watch.", bio) - - def test_two_projects_uses_and(self): - projs = [ - _make_project("Alpha", "alpha"), - _make_project("Beta", "beta"), - ] - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - projects=projs, - ) - bio = auto_generate_bio(person) - self.assertIn("They contributed to 2 projects:", bio) - self.assertIn(">Alpha and Beta, and Alice Smith.", bio) - - @patch("website.utils.bio_utils._get_earliest_member_position") - @patch("website.utils.bio_utils._get_latest_member_position") - def test_mentor_sentence_inactive_uses_was(self, mock_latest, mock_earliest): - pos = _make_position( - start_date=date(2018, 9, 1), - end_date=date(2024, 3, 15), - title="PhD Student", - ) - mock_latest.return_value = pos - mock_earliest.return_value = pos - m = _make_link_person("Alice", "Smith", "alicesmith") - person = _make_person( - is_alumni_member=True, - is_active=False, - current_title="PhD Student", - mentors=[m], - ) - bio = auto_generate_bio(person) - self.assertIn("Jon was mentored by", bio) - - def test_mentor_sentence_multiple_uses_oxford_comma(self): - mentors = [ - _make_link_person("Alice", "Smith", "alicesmith"), - _make_link_person("Bob", "Jones", "bobjones"), - _make_link_person("Carol", "Lee", "carollee"), - ] - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - mentors=mentors, - ) - bio = auto_generate_bio(person) - self.assertIn("Jon is mentored by", bio) - self.assertIn(", and ", bio) - - def test_mentee_member_uses_during_their_time_intro(self): - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - mentees=[_make_link_person("Mia", "X", "miax")], - ) - bio = auto_generate_bio(person) - self.assertIn( - "During their time in the lab, Jon mentored 1 Makeability Lab student", - bio, - ) - - def test_mentee_collaborator_uses_has_mentored_intro(self): - person = _make_person( - is_current_collaborator=True, - is_active=True, - mentees=[_make_link_person("Mia", "X", "miax")], - ) - bio = auto_generate_bio(person) - self.assertNotIn("During their time in the lab", bio) - self.assertIn("Jon has mentored 1 Makeability Lab student", bio) - - def test_mentee_count_three_uses_colon(self): - mentees = [ - _make_link_person("Mia", "X", "miax"), - _make_link_person("Noah", "Y", "noahy"), - _make_link_person("Olivia", "Z", "oliviaz"), - ] - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - mentees=mentees, - ) - bio = auto_generate_bio(person) - self.assertIn("mentored 3 Makeability Lab students:", bio) - - def test_mentee_count_more_than_three_uses_including(self): - mentees = [ - _make_link_person(f"M{i}", "X", f"m{i}x") for i in range(10) - ] - person = _make_person( - is_current_member=True, - is_active=True, - current_title="PhD Student", - mentees=mentees, - ) - bio = auto_generate_bio(person) - self.assertIn("mentored 10 Makeability Lab students, including", bio) - - -# --- HTML escaping --------------------------------------------------------- - - -class HtmlEscapingTests(SimpleTestCase): - """Defensive: free-text spliced into anchor tags must be escaped.""" - - def test_person_name_with_angle_brackets_is_escaped(self): - person = _make_person( - first_name="