From dd07e6278a63f03203716200618e66d0a3504d4c Mon Sep 17 00:00:00 2001 From: Graham Beckley Date: Fri, 17 Feb 2023 11:50:43 -0500 Subject: [PATCH 1/6] Install factory-boy --- poetry.lock | 45 ++++++++++++++++++++++++++++++++++++--------- pyproject.toml | 1 + 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/poetry.lock b/poetry.lock index 4fbc1241..36c145a8 100644 --- a/poetry.lock +++ b/poetry.lock @@ -607,6 +607,40 @@ files = [ dnspython = ">=1.15.0" idna = ">=2.0.0" +[[package]] +name = "factory-boy" +version = "3.2.1" +description = "A versatile test fixtures replacement based on thoughtbot's factory_bot for Ruby." +category = "dev" +optional = false +python-versions = ">=3.6" +files = [ + {file = "factory_boy-3.2.1-py2.py3-none-any.whl", hash = "sha256:eb02a7dd1b577ef606b75a253b9818e6f9eaf996d94449c9d5ebb124f90dc795"}, + {file = "factory_boy-3.2.1.tar.gz", hash = "sha256:a98d277b0c047c75eb6e4ab8508a7f81fb03d2cb21986f627913546ef7a2a55e"}, +] + +[package.dependencies] +Faker = ">=0.7.0" + +[package.extras] +dev = ["Django", "Pillow", "SQLAlchemy", "coverage", "flake8", "isort", "mongoengine", "tox", "wheel (>=0.32.0)", "zest.releaser[recommended]"] +doc = ["Sphinx", "sphinx-rtd-theme", "sphinxcontrib-spelling"] + +[[package]] +name = "faker" +version = "17.0.0" +description = "Faker is a Python package that generates fake data for you." +category = "dev" +optional = false +python-versions = ">=3.7" +files = [ + {file = "Faker-17.0.0-py3-none-any.whl", hash = "sha256:21c3c6c45183308151c14f62afe59bf54ace68f663e0180973698ba2a9a3b2c4"}, + {file = "Faker-17.0.0.tar.gz", hash = "sha256:17cf85aeb0363a3384ccd4c1f52b52ec8f414c7afaab74ae1f4c3e09a06e14de"}, +] + +[package.dependencies] +python-dateutil = ">=2.4" + [[package]] name = "fastapi" version = "0.84.0" @@ -1666,13 +1700,6 @@ files = [ {file = "PyYAML-6.0-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_12_x86_64.manylinux2010_x86_64.whl", hash = "sha256:f84fbc98b019fef2ee9a1cb3ce93e3187a6df0b2538a651bfb890254ba9f90b5"}, {file = "PyYAML-6.0-cp310-cp310-win32.whl", hash = "sha256:2cd5df3de48857ed0544b34e2d40e9fac445930039f3cfe4bcc592a1f836d513"}, {file = "PyYAML-6.0-cp310-cp310-win_amd64.whl", hash = "sha256:daf496c58a8c52083df09b80c860005194014c3698698d1a57cbcfa182142a3a"}, - {file = "PyYAML-6.0-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:d4b0ba9512519522b118090257be113b9468d804b19d63c71dbcf4a48fa32358"}, - {file = "PyYAML-6.0-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:81957921f441d50af23654aa6c5e5eaf9b06aba7f0a19c18a538dc7ef291c5a1"}, - {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:afa17f5bc4d1b10afd4466fd3a44dc0e245382deca5b3c353d8b757f9e3ecb8d"}, - {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:dbad0e9d368bb989f4515da330b88a057617d16b6a8245084f1b05400f24609f"}, - {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:432557aa2c09802be39460360ddffd48156e30721f5e8d917f01d31694216782"}, - {file = "PyYAML-6.0-cp311-cp311-win32.whl", hash = "sha256:bfaef573a63ba8923503d27530362590ff4f576c626d86a9fed95822a8255fd7"}, - {file = "PyYAML-6.0-cp311-cp311-win_amd64.whl", hash = "sha256:01b45c0191e6d66c470b6cf1b9531a771a83c1c4208272ead47a3ae4f2f603bf"}, {file = "PyYAML-6.0-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:897b80890765f037df3403d22bab41627ca8811ae55e9a722fd0392850ec4d86"}, {file = "PyYAML-6.0-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:50602afada6d6cbfad699b0c7bb50d5ccffa7e46a3d738092afddc1f9758427f"}, {file = "PyYAML-6.0-cp36-cp36m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:48c346915c114f5fdb3ead70312bd042a953a8ce5c7106d5bfb1a5254e47da92"}, @@ -2488,5 +2515,5 @@ files = [ [metadata] lock-version = "2.0" -python-versions = "~3.11" -content-hash = "09bf8c1928d7413e98e5a4577127b33c49c817bcc33cf2badaf8408d29bdbf1d" +python-versions = "3.11.1" +content-hash = "ffc5d77397d5c5c8b20bc4cffd7518db4e001886c0a5e11320dbb4eb57995b64" diff --git a/pyproject.toml b/pyproject.toml index b8623c57..022ac0b9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,6 +49,7 @@ SQLAlchemy-Utils = "^0.40.0" pylint = "^2.16.0" pylint-pytest = "^1.1.2" types-requests = "^2.28.11" +factory-boy = "^3.2.1" [tool.pytest.ini_options] testpaths = [ From 1592f450a5fbb49e90af60c490e117947cb4a179 Mon Sep 17 00:00:00 2001 From: Graham Beckley Date: Thu, 23 Feb 2023 08:12:59 +0100 Subject: [PATCH 2/6] Add pytest-factoryboy --- poetry.lock | 32 +++++++++++++++++++++++++++++++- pyproject.toml | 1 + 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/poetry.lock b/poetry.lock index 36c145a8..2cd9986d 100644 --- a/poetry.lock +++ b/poetry.lock @@ -913,6 +913,18 @@ files = [ {file = "imagesize-1.3.0.tar.gz", hash = "sha256:cd1750d452385ca327479d45b64d9c7729ecf0b3969a58148298c77092261f9d"}, ] +[[package]] +name = "inflection" +version = "0.5.1" +description = "A port of Ruby on Rails inflector to Python" +category = "dev" +optional = false +python-versions = ">=3.5" +files = [ + {file = "inflection-0.5.1-py2.py3-none-any.whl", hash = "sha256:f38b2b640938a4f35ade69ac3d053042959b62a0f1076a5bbaa1b9526605a8a2"}, + {file = "inflection-0.5.1.tar.gz", hash = "sha256:1a29730d366e996aaacffb2f1f1cb9593dc38e2ddd30c91250c6dde09ea9b417"}, +] + [[package]] name = "iniconfig" version = "1.1.1" @@ -1606,6 +1618,24 @@ pluggy = ">=0.12,<2.0" [package.extras] testing = ["argcomplete", "hypothesis (>=3.56)", "mock", "nose", "pygments (>=2.7.2)", "requests", "xmlschema"] +[[package]] +name = "pytest-factoryboy" +version = "2.5.1" +description = "Factory Boy support for pytest." +category = "dev" +optional = false +python-versions = ">=3.7" +files = [ + {file = "pytest_factoryboy-2.5.1-py3-none-any.whl", hash = "sha256:41e3465935322188daefc8720f83cebb16bf3d3a430356dc91151c55f31d99c7"}, + {file = "pytest_factoryboy-2.5.1.tar.gz", hash = "sha256:7275a52299b20c0f58b63fdf7326b3fd2b7cbefbdaa90fdcfc776bbe92197484"}, +] + +[package.dependencies] +factory_boy = ">=2.10.0" +inflection = "*" +pytest = ">=5.0.0" +typing_extensions = "*" + [[package]] name = "python-dateutil" version = "2.8.2" @@ -2516,4 +2546,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "3.11.1" -content-hash = "ffc5d77397d5c5c8b20bc4cffd7518db4e001886c0a5e11320dbb4eb57995b64" +content-hash = "1f51b427581390f3ae2bff557ab5345c0ea3f22ec5e120351f7c5835c6d81638" diff --git a/pyproject.toml b/pyproject.toml index 022ac0b9..d0720864 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,6 +50,7 @@ pylint = "^2.16.0" pylint-pytest = "^1.1.2" types-requests = "^2.28.11" factory-boy = "^3.2.1" +pytest-factoryboy = "^2.5.1" [tool.pytest.ini_options] testpaths = [ From e8579ba3ef0a5d2029e08d3e2b244fb95112d65d Mon Sep 17 00:00:00 2001 From: Graham Beckley Date: Thu, 23 Feb 2023 09:02:58 +0100 Subject: [PATCH 3/6] Make unit tests a python package --- tests/unit/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/unit/__init__.py diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py new file mode 100644 index 00000000..e69de29b From 450b803f805b289e7b03ebbab75b96905f736bc4 Mon Sep 17 00:00:00 2001 From: Graham Beckley Date: Thu, 23 Feb 2023 09:03:29 +0100 Subject: [PATCH 4/6] Add scoped session for dbtesting, add email and newsletter factories --- ctms/database.py | 4 ++- tests/unit/conftest.py | 25 +++++++++++++------ tests/unit/factories.py | 54 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 tests/unit/factories.py diff --git a/ctms/database.py b/ctms/database.py index b51d215e..eed8cece 100644 --- a/ctms/database.py +++ b/ctms/database.py @@ -1,5 +1,5 @@ from sqlalchemy import create_engine, engine -from sqlalchemy.orm import declarative_base, sessionmaker +from sqlalchemy.orm import declarative_base, scoped_session, sessionmaker from . import config @@ -17,5 +17,7 @@ def engine_factory(settings): engine = engine_factory(config.Settings()) SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) +# Used for testing +ScopedSessionLocal = scoped_session(SessionLocal) Base = declarative_base() diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index f93b3279..e0e3605d 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -12,8 +12,8 @@ from fastapi.testclient import TestClient from prometheus_client import CollectorRegistry from pydantic import PostgresDsn +from pytest_factoryboy import register from sqlalchemy import create_engine, event -from sqlalchemy.orm import Session from sqlalchemy_utils.functions import create_database, database_exists, drop_database from ctms.app import app, get_api_client, get_db, get_metrics @@ -37,6 +37,7 @@ get_stripe_products, get_waitlists_by_email_id, ) +from ctms.database import ScopedSessionLocal, SessionLocal from ctms.schemas import ( ApiClientSchema, ContactSchema, @@ -51,6 +52,8 @@ SAMPLE_STRIPE_DATA, ) +from . import factories + MY_FOLDER = os.path.dirname(__file__) TEST_FOLDER = os.path.dirname(MY_FOLDER) APP_FOLDER = os.path.dirname(TEST_FOLDER) @@ -100,10 +103,11 @@ def engine(pytestconfig): drop_database(test_db_url) -@pytest.fixture +@pytest.fixture(scope="session") def connection(engine): """Return a connection to the database that rolls back automatically.""" conn = engine.connect() + SessionLocal.configure(bind=conn) yield conn conn.close() @@ -115,16 +119,17 @@ def dbsession(connection): Adapted from https://docs.sqlalchemy.org/en/14/orm/session_transaction.html#joining-a-session-into-an-external-transaction-such-as-for-test-suites """ transaction = connection.begin() - session = Session(autocommit=False, autoflush=False, bind=connection) - nested = connection.begin_nested() + session = ScopedSessionLocal() + nested = session.begin_nested() # If the application code calls session.commit, it will end the nested # transaction. Need to start a new one when that happens. - @event.listens_for(session, "after_transaction_end") - def end_savepoint(session, transaction): + @event.listens_for(ScopedSessionLocal, "after_transaction_end") + def end_savepoint(*args): nonlocal nested - if not nested.is_active: - nested = connection.begin_nested() + if nested.is_active: + session.expire_all() + nested = session.begin_nested() yield session session.close() @@ -158,6 +163,10 @@ def minimal_contact(dbsession): return contact +register(factories.EmailFactory) +register(factories.NewsletterFactory) + + @pytest.fixture def maximal_contact(dbsession): email_id = UUID("67e52c77-950f-4f28-accb-bb3ea1a2c51a") diff --git a/tests/unit/factories.py b/tests/unit/factories.py new file mode 100644 index 00000000..0ee462aa --- /dev/null +++ b/tests/unit/factories.py @@ -0,0 +1,54 @@ +import factory +from factory.alchemy import SQLAlchemyModelFactory + +from ctms import models +from ctms.database import ScopedSessionLocal + + +class BaseSQLAlchemyModelFactory(SQLAlchemyModelFactory): + class Meta: + abstract = True + sqlalchemy_session = ScopedSessionLocal + + +class NewsletterFactory(BaseSQLAlchemyModelFactory): + class Meta: + model = models.Newsletter + + name = factory.Sequence(lambda n: f"newsletter-{n}") + subscribed = True + format = "T" + lang = factory.Faker("language_code") + source = factory.Faker("url") + + email = factory.SubFactory(factory="tests.unit.factories.EmailFactory") + + +class EmailFactory(BaseSQLAlchemyModelFactory): + class Meta: + model = models.Email + + email_id = factory.Faker("uuid4") + primary_email = factory.Faker("email") + basket_token = factory.Faker("uuid4") + first_name = factory.Faker("first_name") + last_name = factory.Faker("last_name") + mailing_country = factory.Faker("country_code") + email_format = "T" + email_lang = factory.Faker("language_code") + double_opt_in = False + has_opted_out_of_email = False + + @factory.post_generation + def newsletters(self, create, extracted, **kwargs): + if not create: + return + if extracted: + for _ in range(extracted): + NewsletterFactory(email=self, **kwargs) + + +__all__ = ( + "NewsletterFactory", + "EmailFactory", +) From 7f5dd92cf829360d2bfa88b086003854c8975cca Mon Sep 17 00:00:00 2001 From: Graham Beckley Date: Thu, 23 Feb 2023 09:04:00 +0100 Subject: [PATCH 5/6] Create example tests with factories --- tests/unit/test_api_get.py | 73 +++++++++++++----------------------- tests/unit/test_api_patch.py | 31 ++++++--------- 2 files changed, 38 insertions(+), 66 deletions(-) diff --git a/tests/unit/test_api_get.py b/tests/unit/test_api_get.py index 1788d28c..b90e3f15 100644 --- a/tests/unit/test_api_get.py +++ b/tests/unit/test_api_get.py @@ -6,9 +6,12 @@ from ctms.models import Email -def test_get_ctms_for_minimal_contact(client, minimal_contact): +def test_get_ctms_for_minimal_contact(client, dbsession, email_factory): """GET /ctms/{email_id} returns a contact with most fields unset.""" - email_id = minimal_contact.email.email_id + contact = email_factory(newsletters=1) + newsletter = contact.newsletters[0] + dbsession.commit() + email_id = str(contact.email_id) resp = client.get(f"/ctms/{email_id}") assert resp.status_code == 200 assert resp.json() == { @@ -27,20 +30,20 @@ def test_get_ctms_for_minimal_contact(client, minimal_contact): "username": None, }, "email": { - "basket_token": "142e20b6-1ef5-43d8-b5f4-597430e956d7", - "create_timestamp": "2014-01-22T15:24:00+00:00", - "double_opt_in": False, - "email_format": "H", - "email_id": "93db83d4-4119-4e0c-af87-a713786fa81d", - "email_lang": "en", - "first_name": None, - "has_opted_out_of_email": False, - "last_name": None, - "mailing_country": "us", - "primary_email": "ctms-user@example.com", - "sfdc_id": "001A000001aABcDEFG", - "unsubscribe_reason": None, - "update_timestamp": "2020-01-22T15:24:00+00:00", + "basket_token": str(contact.basket_token), + "create_timestamp": contact.create_timestamp.isoformat(), + "double_opt_in": contact.double_opt_in, + "email_format": contact.email_format, + "email_id": str(contact.email_id), + "email_lang": contact.email_lang, + "first_name": contact.first_name, + "has_opted_out_of_email": contact.has_opted_out_of_email, + "last_name": contact.last_name, + "mailing_country": contact.mailing_country, + "primary_email": contact.primary_email, + "sfdc_id": None, + "unsubscribe_reason": contact.unsubscribe_reason, + "update_timestamp": contact.update_timestamp.isoformat(), }, "fxa": { "created_date": None, @@ -57,37 +60,13 @@ def test_get_ctms_for_minimal_contact(client, minimal_contact): }, "newsletters": [ { - "format": "H", - "lang": "en", - "name": "app-dev", - "source": None, - "subscribed": True, - "unsub_reason": None, - }, - { - "format": "H", - "lang": "en", - "name": "maker-party", - "source": None, - "subscribed": True, - "unsub_reason": None, - }, - { - "format": "H", - "lang": "en", - "name": "mozilla-foundation", - "source": None, - "subscribed": True, - "unsub_reason": None, - }, - { - "format": "H", - "lang": "en", - "name": "mozilla-learning-network", - "source": None, - "subscribed": True, - "unsub_reason": None, - }, + "format": newsletter.format, + "lang": newsletter.lang, + "name": newsletter.name, + "source": newsletter.source, + "subscribed": newsletter.subscribed, + "unsub_reason": newsletter.unsub_reason, + } ], "status": "ok", "vpn_waitlist": {"geo": None, "platform": None}, diff --git a/tests/unit/test_api_patch.py b/tests/unit/test_api_patch.py index 419900aa..9cbe3e7e 100644 --- a/tests/unit/test_api_patch.py +++ b/tests/unit/test_api_patch.py @@ -321,32 +321,25 @@ def test_patch_to_subscribe(client, maximal_contact): } -def test_patch_to_update_subscription(client, maximal_contact): +def test_patch_to_update_subscription(client, dbsession, newsletter_factory): """PATCH can update an existing newsletter subscription.""" - email_id = maximal_contact.email.email_id - existing_news_data = maximal_contact.newsletters[1].dict() - assert existing_news_data == { - "format": "T", - "lang": "fr", - "name": "common-voice", - "source": "https://commonvoice.mozilla.org/fr", - "subscribed": True, - "unsub_reason": None, - } + existing_newsletter = newsletter_factory() + dbsession.commit() + email_id = str(existing_newsletter.email.email_id) patch_data = { - "newsletters": [{"name": "common-voice", "format": "H", "lang": "en"}] + "newsletters": [{"name": existing_newsletter.name, "format": "H", "lang": "XX"}] } resp = client.patch(f"/ctms/{email_id}", json=patch_data, allow_redirects=True) assert resp.status_code == 200 actual = resp.json() - assert len(actual["newsletters"]) == len(maximal_contact.newsletters) - assert actual["newsletters"][1] == { + assert len(actual["newsletters"]) == 1 + assert actual["newsletters"][0] == { "format": "H", - "lang": "en", - "name": "common-voice", - "source": "https://commonvoice.mozilla.org/fr", - "subscribed": True, - "unsub_reason": None, + "lang": "XX", + "name": existing_newsletter.name, + "source": existing_newsletter.source, + "subscribed": existing_newsletter.subscribed, + "unsub_reason": existing_newsletter.unsub_reason, } From b331d5b51ebc7f0cb641310626d193fca146c2f1 Mon Sep 17 00:00:00 2001 From: Graham Beckley Date: Wed, 8 Mar 2023 13:37:35 -0500 Subject: [PATCH 6/6] Move session setup closer to documentation example --- tests/unit/conftest.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 1ff2dd92..6e59ba6e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -124,18 +124,22 @@ def dbsession(connection): """ transaction = connection.begin() session = ScopedSessionLocal() - nested = session.begin_nested() + nested = connection.begin_nested() # If the application code calls session.commit, it will end the nested # transaction. Need to start a new one when that happens. - @event.listens_for(ScopedSessionLocal, "after_transaction_end") + @event.listens_for(session, "after_transaction_end") def end_savepoint(*args): nonlocal nested - if nested.is_active: - session.expire_all() - nested = session.begin_nested() + if not nested.is_active: + nested = connection.begin_nested() yield session + # a nescessary addition to the example in the documentation linked above. + # Without this, the listener is not removed after each test ends and + # SQLAlchemy emits warnings: + # `SAWarning: nested transaction already deassociated from connection` + event.remove(session, "after_transaction_end", end_savepoint) session.close() transaction.rollback()