diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b9c7f67..a063fac1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -264,7 +264,7 @@ The suite has two complementary styles: | **Unit** | `SimpleTestCase` + `MagicMock` | Pure logic — formatters, BibTeX generation, single-method behavior. No DB; runs in milliseconds. | | **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`, `make_talk`, 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 — use those rather than hand-rolling fixtures. They're thin wrappers over the `factory_boy` factories in `website/tests/factories.py`, which are the single source of truth for building model instances; reach for a factory directly when you need an entity the helpers don't cover or want to customize fields. ### When to add a test diff --git a/requirements.txt b/requirements.txt index 25b3e67b..40716b56 100644 --- a/requirements.txt +++ b/requirements.txt @@ -123,4 +123,19 @@ django-ckeditor==6.7.3 pyOpenSSL==26.0.0 # Requests - HTTP library for Python -requests==2.33.0 \ No newline at end of file +requests==2.33.0 + +# ----------------------------------------------------------------------------- +# Testing +# ----------------------------------------------------------------------------- +# factory_boy - model fixtures for the test suite (#1272). The de-facto Django +# standard; we use explicit factories (website/tests/factories.py) over +# model_bakery because they're easier to debug with our M2M / SortedManyToMany / +# ProjectRole-through relationship graph. Pulls in Faker (pinned below) as a dep. +# See: https://factoryboy.readthedocs.io/ +factory_boy==3.3.3 + +# Faker - realistic fake data (names, sentences, dates) used by the factories. +# Dependency of factory_boy; pinned explicitly so test data stays reproducible. +# See: https://faker.readthedocs.io/ +Faker==40.23.0 \ No newline at end of file diff --git a/website/tests/base.py b/website/tests/base.py index 8b494c62..1bad76c6 100644 --- a/website/tests/base.py +++ b/website/tests/base.py @@ -12,103 +12,69 @@ coverage incrementally. See #1267 for the broader plan. """ -from django.core.files.uploadedfile import SimpleUploadedFile -from django.test import TestCase +from datetime import date as _date +from django.test import TestCase -# Minimal 1x1 GIF used to satisfy Person.image / Person.easter_egg without -# touching the filesystem. Person.save() picks a random Star Wars image when -# either field is empty, opening a real file from media/. Pre-setting both -# with this SimpleUploadedFile skips the fallback branch entirely. -_GIF_1PX = ( - b"GIF89a\x01\x00\x01\x00\x80\x00\x00\xff\xff\xff\x00\x00\x00" - b"!\xf9\x04\x01\x00\x00\x00\x00,\x00\x00\x00\x00\x01\x00\x01" - b"\x00\x00\x02\x02D\x01\x00;" +from website.tests.factories import ( + NewsItemFactory, + PersonFactory, + ProjectFactory, + PublicationFactory, + TalkFactory, + VideoFactory, + image_upload, ) -def _make_image_upload(name): - """Return a SimpleUploadedFile that satisfies an ImageField.""" - return SimpleUploadedFile(name, _GIF_1PX, content_type="image/gif") - - class DatabaseTestCase(TestCase): """ Shared base for tests that touch the database. Provides small fixture - helpers (make_person / make_publication / make_news_item) built on - plain Model.objects.create() — no third-party fixture library. Each - test runs inside a transaction and is rolled back, so tests stay - isolated without manual cleanup. - - Why a base class instead of module-level helpers: subclasses can - override the defaults in setUp() and the helpers can grow without - cluttering the module namespace. + helpers (make_person / make_publication / make_news_item / ...) that + delegate to the factory_boy factories in :mod:`website.tests.factories` + (#1272). The factories are the single source of truth for building + instances; these helpers preserve the original keyword API (notably the + ``year`` shorthand) so the existing suite keeps working unchanged. Each + test runs inside a transaction and is rolled back, so tests stay isolated + without manual cleanup. + + Why keep the helpers at all: they encode test-friendly defaults (fixed + dates via ``year``, ``with_thumbnail`` for the project visibility backfill) + and give subclasses a stable seam to override in ``setUp()``. Tests that + want richer fixtures (Faker values, batches, the relationship graph) can + import and use the factories directly. """ def make_person(self, first_name="Jane", last_name="Doe", **kwargs): """ - Create and return a Person. Image fields are pre-populated to - skip Person.save()'s Star Wars fallback (which reads a real file - from media/). Override by passing image=... explicitly. + Create and return a Person. Image fields are pre-populated by + PersonFactory to skip Person.save()'s Star Wars fallback (which reads a + real file from media/). Override by passing image=... explicitly. """ - from website.models import Person - kwargs.setdefault( - "image", _make_image_upload(f"{first_name}_{last_name}.gif") - ) - kwargs.setdefault( - "easter_egg", - _make_image_upload(f"{first_name}_{last_name}_egg.gif"), - ) - return Person.objects.create( + return PersonFactory( first_name=first_name, last_name=last_name, **kwargs ) def make_publication(self, title="A Test Paper", year=2024, **kwargs): """ Create and return a Publication with sensible defaults: post-lab- - formation date, conference venue, a forum name, and a dummy PDF - (display_pub_snippet.html unconditionally renders pub.pdf_file.url, - so tests that go through the publications view need one to render). - Override via kwargs. + formation date (from ``year``), conference venue, a forum name, and a + dummy PDF (display_pub_snippet.html unconditionally renders + pub.pdf_file.url, so tests that go through the publications view need + one to render). Override via kwargs. """ - from datetime import date as _date - from website.models import Publication - from website.models.publication import PubType kwargs.setdefault("date", _date(year, 1, 1)) - kwargs.setdefault("forum_name", "CHI") - kwargs.setdefault("pub_venue_type", PubType.CONFERENCE) - kwargs.setdefault( - "pdf_file", - SimpleUploadedFile( - f"{title.replace(' ', '_')}.pdf", - b"%PDF-1.4 test", - content_type="application/pdf", - ), - ) - return Publication.objects.create(title=title, **kwargs) + return PublicationFactory(title=title, **kwargs) def make_talk(self, title="A Test Talk", year=2024, **kwargs): """ Create and return a Talk. Artifact.save() generates a thumbnail - from pdf_file (via ImageMagick) on every save, so we provide a - small valid PDF and let it run; tests that don't care about the + from pdf_file (via ImageMagick) on every save, so the factory provides + a small valid PDF and lets it run; tests that don't care about the thumbnail just ignore it. """ - from datetime import date as _date - from website.models import Talk - from website.models.talk import TalkType kwargs.setdefault("date", _date(year, 1, 1)) - kwargs.setdefault("forum_name", "CHI") - kwargs.setdefault("talk_type", TalkType.CONFERENCE_TALK) - kwargs.setdefault( - "pdf_file", - SimpleUploadedFile( - f"{title.replace(' ', '_')}.pdf", - b"%PDF-1.4 test", - content_type="application/pdf", - ), - ) - return Talk.objects.create(title=title, **kwargs) + return TalkFactory(title=title, **kwargs) def make_video(self, title="A Test Video", year=2024, **kwargs): """ @@ -117,11 +83,8 @@ def make_video(self, title="A Test Video", year=2024, **kwargs): would raise), and the video snippet embeds it. date is set so get_most_recent_artifact_date() has something to sort on. """ - from datetime import date as _date - from website.models import Video kwargs.setdefault("date", _date(year, 1, 1)) - kwargs.setdefault("video_url", "https://www.youtube.com/watch?v=dQw4w9WgXcQ") - return Video.objects.create(title=title, **kwargs) + return VideoFactory(title=title, **kwargs) def make_news_item(self, title="Test News", author=None, **kwargs): """ @@ -129,11 +92,9 @@ def make_news_item(self, title="Test News", author=None, **kwargs): (the FK is nullable with on_delete=SET_NULL) so tests can exercise the authorless code path that caused the original /news/158/ bug. """ - from datetime import date as _date - from website.models import News kwargs.setdefault("date", _date(2024, 1, 1)) kwargs.setdefault("content", "Test news body.") - return News.objects.create(title=title, author=author, **kwargs) + return NewsItemFactory(title=title, author=author, **kwargs) def make_project(self, name="A Test Project", short_name=None, with_thumbnail=False, **kwargs): @@ -149,12 +110,11 @@ def make_project(self, name="A Test Project", short_name=None, visibility backfill. Defaults to False to avoid touching the filesystem unnecessarily. """ - from website.models import Project if short_name is None: short_name = name.lower().replace(" ", "") if with_thumbnail: kwargs.setdefault( "gallery_image", - _make_image_upload(f"{short_name}_thumb.gif"), + image_upload(f"{short_name}_thumb.gif"), ) - return Project.objects.create(name=name, short_name=short_name, **kwargs) + return ProjectFactory(name=name, short_name=short_name, **kwargs) diff --git a/website/tests/factories.py b/website/tests/factories.py new file mode 100644 index 00000000..dfe36775 --- /dev/null +++ b/website/tests/factories.py @@ -0,0 +1,217 @@ +""" +factory_boy fixtures for the Makeability Lab models (#1272). + +These factories are the single source of truth for building model instances in +tests. The ``DatabaseTestCase.make_*`` helpers in :mod:`website.tests.base` +delegate to them, so all existing DB-backed tests run through this code too. + +Design notes +------------ +- **Filesystem-light by default.** Image and PDF fields are populated with tiny + in-memory uploads (a 1x1 GIF, a stub PDF) rather than real media. This matches + the original ``base.py`` philosophy: the DB suite stays fast and never touches + ``media/``. In particular, pre-setting ``Person.image`` / ``easter_egg`` skips + ``Person.save()``'s Star Wars fallback, which would otherwise read a real file + off disk. A later PR (#1272 layer 2) can add curated ``seed_media/`` files and + point a seed-data command at them when real thumbnail/crop/PDF code paths + actually need exercising. +- **No auto-authors.** ``PublicationFactory`` / ``TalkFactory`` / ``PosterFactory`` + do *not* invent authors. Callers pass ``authors=[...]`` (a list of ``Person`` + instances) when they want them; otherwise the artifact is created authorless. + This keeps the ~200 existing ``make_publication`` call sites green (they add + authors explicitly) and leaves authorless code paths testable. A seed-data + command can pass ``authors=PersonFactory.create_batch(n)`` for a populated graph. + +Usage +----- + from website.tests.factories import PersonFactory, PublicationFactory + + alice = PersonFactory(first_name="Alice", last_name="Ng") + pub = PublicationFactory(title="A Paper", authors=[alice]) + batch = PersonFactory.create_batch(5) # five people with Faker names +""" + +from datetime import date + +import factory +from django.core.files.uploadedfile import SimpleUploadedFile + +from website.models import ( + Award, + News, + Person, + Poster, + Project, + ProjectRole, + Publication, + Talk, + Video, +) +from website.models.award import AwardType +from website.models.publication import PubType +from website.models.talk import TalkType + +# Smallest possible valid GIF (1x1, transparent). Used for ImageFields so the +# upload validators (extension + magic-byte header) pass without reading disk. +_GIF_1PX = ( + b"GIF89a\x01\x00\x01\x00\x80\x00\x00\xff\xff\xff\x00\x00\x00" + b"!\xf9\x04\x01\x00\x00\x00\x00,\x00\x00\x00\x00\x01\x00\x01" + b"\x00\x00\x02\x02D\x01\x00;" +) + +# Minimal stub with a real %PDF magic header so validate_pdf_upload passes. +_PDF_STUB = b"%PDF-1.4 test" + + +def image_upload(name): + """Return a SimpleUploadedFile satisfying an ImageField (1x1 GIF).""" + return SimpleUploadedFile(name, _GIF_1PX, content_type="image/gif") + + +def pdf_upload(name): + """Return a SimpleUploadedFile satisfying a pdf_file FileField.""" + return SimpleUploadedFile(name, _PDF_STUB, content_type="application/pdf") + + +class PersonFactory(factory.django.DjangoModelFactory): + """A Person with both image fields pre-set (skips the Star Wars fallback).""" + + class Meta: + model = Person + + first_name = factory.Faker("first_name") + last_name = factory.Faker("last_name") + image = factory.LazyAttribute( + lambda o: image_upload(f"{o.first_name}_{o.last_name}.gif") + ) + easter_egg = factory.LazyAttribute( + lambda o: image_upload(f"{o.first_name}_{o.last_name}_egg.gif") + ) + + +class ProjectFactory(factory.django.DjangoModelFactory): + """ + A Project. Created exactly as Project.save() leaves it, i.e. + ``is_visible=False`` (private) unless you pass ``is_visible=True``. + ``short_name`` is derived from ``name`` (lowercased, no spaces) plus a + sequence suffix so factory-built projects get distinct URL slugs. + """ + + class Meta: + model = Project + + name = factory.Faker("catch_phrase") + short_name = factory.Sequence(lambda n: f"project{n}") + + +class VideoFactory(factory.django.DjangoModelFactory): + """ + A Video. ``video_url`` defaults to a real YouTube URL because + ``Video.get_video_host_str()`` substring-matches it (a None url would raise) + and the video snippet embeds it. + """ + + class Meta: + model = Video + + title = factory.Faker("sentence", nb_words=5) + date = factory.Faker("date_between", start_date="-3y", end_date="today") + video_url = "https://www.youtube.com/watch?v=dQw4w9WgXcQ" + + +class _ArtifactFactory(factory.django.DjangoModelFactory): + """ + Shared declarations for the Artifact subclasses (Publication/Talk/Poster). + Abstract: not a model itself, just a mixin of common fields plus the + ``authors`` post-generation hook. + """ + + class Meta: + abstract = True + + title = factory.Faker("sentence", nb_words=8) + date = factory.Faker("date_between", start_date="-3y", end_date="today") + forum_name = "CHI" + pdf_file = factory.LazyAttribute( + lambda o: pdf_upload(f"{o.title.replace(' ', '_')[:40]}.pdf") + ) + + @factory.post_generation + def authors(self, create, extracted, **kwargs): + """Set authors only when explicitly provided (see module docstring).""" + if create and extracted: + self.authors.set(extracted) + + +class PublicationFactory(_ArtifactFactory): + """A Publication (defaults to a peer-reviewed conference paper).""" + + class Meta: + model = Publication + + pub_venue_type = PubType.CONFERENCE + + +class TalkFactory(_ArtifactFactory): + """A Talk. Artifact.save() generates a thumbnail from pdf_file on save.""" + + class Meta: + model = Talk + + talk_type = TalkType.CONFERENCE_TALK + + +class PosterFactory(_ArtifactFactory): + """A Poster.""" + + class Meta: + model = Poster + + +class NewsItemFactory(factory.django.DjangoModelFactory): + """ + A News item. ``author`` is intentionally left null by default (the FK is + nullable, on_delete=SET_NULL) so the authorless code path stays exercised; + pass ``author=PersonFactory()`` for an authored item. + """ + + class Meta: + model = News + + title = factory.Faker("sentence", nb_words=6) + date = factory.Faker("date_between", start_date="-3y", end_date="today") + content = factory.Faker("paragraph") + + +class AwardFactory(factory.django.DjangoModelFactory): + """ + An external Award (the Awards-page kind, distinct from Publication.award). + Pass ``recipients=[person, ...]`` and/or ``projects=[...]`` to link them. + """ + + class Meta: + model = Award + + title = factory.Faker("sentence", nb_words=5) + date = factory.Faker("date_between", start_date="-3y", end_date="today") + award_type = AwardType.STUDENT_AWARD + + @factory.post_generation + def recipients(self, create, extracted, **kwargs): + if create and extracted: + self.recipients.set(extracted) + + +class ProjectRoleFactory(factory.django.DjangoModelFactory): + """ + A ProjectRole linking a Person to a Project. Builds its own Person and + Project via SubFactory unless you pass ``person=`` / ``project=``. + ``start_date`` is required by the model, so it always gets a value. + """ + + class Meta: + model = ProjectRole + + person = factory.SubFactory(PersonFactory) + project = factory.SubFactory(ProjectFactory) + start_date = factory.Faker("date_between", start_date="-3y", end_date="today") diff --git a/website/tests/test_easter_egg_picker.py b/website/tests/test_easter_egg_picker.py index 6a34d059..e5c04b82 100644 --- a/website/tests/test_easter_egg_picker.py +++ b/website/tests/test_easter_egg_picker.py @@ -26,7 +26,7 @@ from django.test import SimpleTestCase, TestCase import website.utils.fileutils as ml_fileutils -from website.tests.base import _GIF_1PX +from website.tests.factories import _GIF_1PX # --- figure listing (single source of truth) ------------------------------- diff --git a/website/tests/test_factories.py b/website/tests/test_factories.py new file mode 100644 index 00000000..cdc6ded1 --- /dev/null +++ b/website/tests/test_factories.py @@ -0,0 +1,115 @@ +""" +Tests for the factory_boy fixtures (#1272). + +These guard the factories themselves: that each builds a valid, saved instance +with the file fields populated (so the upload validators pass and Person skips +its Star Wars fallback), that the relationship graph composes, and that the +no-auto-authors contract the ``base.py`` helpers rely on holds. +""" + +from website.models import Person, Project, ProjectRole, Publication +from website.tests.base import DatabaseTestCase +from website.tests.factories import ( + AwardFactory, + NewsItemFactory, + PersonFactory, + PosterFactory, + ProjectFactory, + ProjectRoleFactory, + PublicationFactory, + TalkFactory, + VideoFactory, +) + + +class FactoryTests(DatabaseTestCase): + def test_each_factory_builds_a_saved_instance(self): + """Every factory persists a row and fills the required fields.""" + for factory_cls in ( + PersonFactory, + ProjectFactory, + VideoFactory, + PublicationFactory, + TalkFactory, + PosterFactory, + NewsItemFactory, + AwardFactory, + ProjectRoleFactory, + ): + with self.subTest(factory=factory_cls.__name__): + obj = factory_cls() + self.assertIsNotNone(obj.pk) + + def test_person_image_fields_are_prepopulated(self): + """ + PersonFactory sets image + easter_egg, so Person.save() never falls + back to opening a random Star Wars file off disk. + """ + person = PersonFactory() + self.assertTrue(person.image) + self.assertTrue(person.easter_egg) + + def test_factory_runs_through_url_name_collision_resolution(self): + """ + Person.save() resolves url_name collisions by appending a counter, and + the factory goes through save(), so two people with the *same* name + still get distinct url_names. Force a collision rather than relying on + Faker (whose values can't be counted on to repeat or to differ). + """ + a = PersonFactory(first_name="Sam", last_name="Lee") + b = PersonFactory(first_name="Sam", last_name="Lee") + self.assertNotEqual(a.url_name, b.url_name) + # A Faker-named batch also all persist (distinct names or not). + PersonFactory.create_batch(5) + self.assertEqual(Person.objects.count(), 7) + + def test_publication_has_no_authors_by_default(self): + """ + The no-auto-authors contract: artifacts are authorless unless the + caller passes authors=. The base.py make_* helpers depend on this. + """ + pub = PublicationFactory() + self.assertEqual(pub.authors.count(), 0) + + def test_authors_passed_explicitly_are_set_in_order(self): + """Passing authors= populates the SortedManyToManyField.""" + a, b = PersonFactory(), PersonFactory() + pub = PublicationFactory(authors=[a, b]) + self.assertEqual(list(pub.authors.all()), [a, b]) + + def test_project_role_subfactories_compose_the_graph(self): + """ + ProjectRoleFactory wires a Person to a Project through the role table + without the caller building either side by hand. + """ + role = ProjectRoleFactory() + self.assertIsInstance(role.person, Person) + self.assertIsInstance(role.project, Project) + self.assertEqual(role.project.projectrole_set.count(), 1) + + def test_project_role_accepts_explicit_person_and_project(self): + person = PersonFactory() + project = ProjectFactory() + role = ProjectRoleFactory(person=person, project=project) + self.assertEqual(role.person, person) + self.assertEqual(role.project, project) + self.assertEqual(ProjectRole.objects.count(), 1) + + def test_award_recipients_are_linked(self): + recipients = PersonFactory.create_batch(2) + award = AwardFactory(recipients=recipients) + self.assertEqual(award.recipients.count(), 2) + + def test_project_defaults_to_private(self): + """ProjectFactory leaves Project.save()'s private-by-default in place.""" + self.assertFalse(ProjectFactory().is_visible) + + def test_make_helpers_delegate_to_factories(self): + """ + The base.py helpers still honor their original keyword API — notably + the ``year`` shorthand — now that they delegate to the factories. + """ + pub = self.make_publication(title="Delegated", year=2021) + self.assertEqual(pub.title, "Delegated") + self.assertEqual(pub.date.year, 2021) + self.assertIsInstance(pub, Publication)