Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Pytest CI mode to optimise migrations #3815

Merged
merged 6 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/.env-ci
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
DATABASE_URL=postgresql://postgres:postgres@localhost:5432/postgres
ANALYTICS_DATABASE_URL=postgres://postgres:postgres@localhost:5432/analytics
PYTEST_ADDOPTS=--cov . --cov-report xml -n auto --dist worksteal
PYTEST_ADDOPTS=--cov . --cov-report xml -n auto --dist worksteal --ci
GUNICORN_LOGGER_CLASS=util.logging.GunicornJsonCapableLogger
COVERAGE_CORE=sysmon
1 change: 0 additions & 1 deletion api/.env-local
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
DATABASE_URL=postgresql://postgres:password@localhost:5432/flagsmith
DJANGO_SETTINGS_MODULE=app.settings.local
PYTEST_ADDOPTS=--cov . --cov-report html -n auto
GUNICORN_LOGGER_CLASS=util.logging.GunicornJsonCapableLogger
2 changes: 2 additions & 0 deletions api/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ DOTENV_OVERRIDE_FILE ?= .env

POETRY_VERSION ?= 1.8.2

GUNICORN_LOGGER_CLASS ?= util.logging.GunicornJsonCapableLogger

-include .env-local
-include $(DOTENV_OVERRIDE_FILE)

Expand Down
50 changes: 50 additions & 0 deletions api/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@
import pytest
from django.contrib.contenttypes.models import ContentType
from django.core.cache import caches
from django.db.backends.base.creation import TEST_DATABASE_PREFIX
from django.test.utils import setup_databases
from flag_engine.segments.constants import EQUAL
from moto import mock_dynamodb
from mypy_boto3_dynamodb.service_resource import DynamoDBServiceResource, Table
from pytest_django.plugin import blocking_manager_key
from rest_framework.authtoken.models import Token
from rest_framework.test import APIClient
from xdist import get_xdist_worker_id

from api_keys.models import MasterAPIKey
from environments.identities.models import Identity
Expand Down Expand Up @@ -65,6 +69,52 @@
)
from users.models import FFAdminUser, UserPermissionGroup


def pytest_addoption(parser: pytest.Parser) -> None:
parser.addoption(
Comment on lines +72 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming that this function will be auto-invoked without a decorator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See coverage report :)

Dive into pytest hooks if you're curious about how this works.

"--ci",
action="store_true",
default=False,
help="Enable CI mode",
)


@pytest.hookimpl(trylast=True)
def pytest_configure(config: pytest.Config) -> None:
if (
config.option.ci
and config.option.dist != "no"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that the option's value comes out as "no" instead of False.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"no" is, in fact, one of xdist's distribution modes. See here.

and not hasattr(config, "workerinput")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems weird that if ci is present that if workerinput is set that this will silently fail. Don't we prefer an exception if ci is set and so is workerinput?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having ci and having workerinput is a legitimate case when we need this hook to be a noop.

):
with config.stash[blocking_manager_key].unblock():
setup_databases(
verbosity=config.option.verbose,
interactive=False,
parallel=config.option.numprocesses,
)


@pytest.fixture(scope="session")
def django_db_setup(request: pytest.FixtureRequest) -> None:
if (
request.config.option.ci
# xdist worker id is either `gw[0-9]+` or `master`
and (xdist_worker_id_suffix := get_xdist_worker_id(request)[2:]).isnumeric()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious why

get_xdist_worker_id(request)[2:]

That part of the logic ends with a [2:]. Can you add a comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

):
# Django's test database clone indices start at 1,
# Pytest's worker indices are 0-based
test_db_suffix = str(int(xdist_worker_id_suffix) + 1)
else:
# Tests are run on main node, which assumes -n0
return request.getfixturevalue("django_db_setup") # pragma: no cover

from django.conf import settings

for db_settings in settings.DATABASES.values():
test_db_name = f'{TEST_DATABASE_PREFIX}{db_settings["NAME"]}_{test_db_suffix}'
db_settings["NAME"] = test_db_name


trait_key = "key1"
trait_value = "value1"

Expand Down