wagtail / wagtail-localize

Translation plugin for Wagtail CMS
https://wagtail-localize.org/
Other
222 stars 84 forks source link

Tests fail when using pytest with SQLite and TranslationSource.get_or_create_from_instance #706

Closed urlsangel closed 12 months ago

urlsangel commented 1 year ago

wagtail = "4.2" wagtail-localize = "^1.4"

We are running tests using pytest with the --nomigrations flag and SQLite as the db backend.

When running tests involving TranslationSource.get_or_create_from_instance, the tests fail with the following error:

/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/wagtail_localize/models.py:381: in get_or_create_from_instance
    "schema_version": get_schema_version(instance._meta.app_label) or "",
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/wagtail_localize/models.py:136: in get_schema_version
    .last()
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/cacheops/query.py:363: in last
    return self._no_monkey.last(self)
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/models/query.py:1055: in last
    for obj in (self.reverse() if self.ordered else self.order_by("-pk"))[:1]:
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/models/query.py:394: in __iter__
    self._fetch_all()
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/cacheops/query.py:273: in _fetch_all
    return self._no_monkey._fetch_all(self)
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/models/query.py:1867: in _fetch_all
    self._result_cache = list(self._iterable_class(self))
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/models/query.py:87: in __iter__
    results = compiler.execute_sql(
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/models/sql/compiler.py:1398: in execute_sql
    cursor.execute(sql, params)
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/cacheops/transaction.py:98: in execute
    result = self._no_monkey.execute(self, sql, params)
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/backends/utils.py:67: in execute
    return self._execute_with_wrappers(
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/backends/utils.py:80: in _execute_with_wrappers
    return executor(sql, params, many, context)
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/backends/utils.py:84: in _execute
    with self.db.wrap_database_errors:
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/utils.py:91: in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
/root/.cache/pypoetry/virtualenvs/.venv/lib/python3.10/site-packages/django/db/backends/utils.py:89: in _execute
    return self.cursor.execute(sql, params)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <django.db.backends.sqlite3.base.SQLiteCursorWrapper object at 0xffffb63171c0>
query = 'SELECT "django_migrations"."id", "django_migrations"."app", "django_migrations"."name", "django_migrations"."applied" FROM "django_migrations" WHERE "django_migrations"."app" = ? ORDER BY "django_migrations"."applied" DESC LIMIT 1'
params = ('core',)

    def execute(self, query, params=None):
        if params is None:
            return Database.Cursor.execute(self, query)
        query = self.convert_query(query)
>       return Database.Cursor.execute(self, query, params)
E       django.db.utils.OperationalError: no such table: django_migrations

This looks like it's because MigrationRecorder is being used here:

https://github.com/wagtail/wagtail-localize/blob/v1.4/wagtail_localize/models.py#L139

Is there a way around this for testing TranslationSource using pytest and SQLite?

zerolab commented 1 year ago

Hey @urlsangel,

To be honest, I do not know šŸ™ˆ as I do not use pytest. Will try to have a read in the coming days.

urlsangel commented 1 year ago

Thanks @zerolab! šŸ™

urlsangel commented 1 year ago

Hi @zerolab - just getting back onto this now, did you get a chance to have a look at this issue?

zerolab commented 1 year ago

Heh, did not get any breathing time to look into pytest. But I do have some dedicated Wagtail time this Friday and I aim to play with it

urlsangel commented 1 year ago

Awesome, Thanks @zerolab! šŸ‘ šŸ˜€

zerolab commented 1 year ago

@urlsangel well.. Friday was more meetings-heavy than anticipated so did not get to this. Do you have a minimal test case that would help speed things up on this side?

urlsangel commented 1 year ago

Hi @zerolab

No problem - here's one of the tests that was failing, it's because of the MigrationRecorder being called via TranslationSource by the looks (see previous comment here: https://github.com/wagtail/wagtail-localize/issues/706#issue-1822513281):

import pytest
from wagtail_localize.models import (
    Translation,
    TranslationSource,
)
from ..service import _languages

from helpers.util import dummy_request

@pytest.mark.trans
def test_translate_pt_to_es(benchmark, client, tagged_article_pt):
    og = tagged_article_pt
    og.body = og.footer = og.excerpt = ''

    new_lang = _languages.first(language_code='es')
    source, created = TranslationSource.get_or_create_from_instance(og)
    translation = Translation.objects.create(
        source=source,
        target_locale=new_lang,
    )
    translation.save_target()
    np = og.get_translation(new_lang)
    np.title = "Spanish translation of Portuguese page"
    np.save_revision().publish()

    res = benchmark(client.get, np.url)
    assert res.status_code == 200
    assert res is not None
    assert "Spanish translation of Portuguese page" in res.rendered_content
zerolab commented 1 year ago

The quick fix would be to wrap https://github.com/wagtail/wagtail-localize/blob/v1.4/wagtail_localize/models.py#L139 in a try/except.

But I most certainly want to understand a bit more as to how pytest does things and how it gets away with not running Django migrations

chris48s commented 1 year ago

--nomigrations is a feature of pytest-django, not pytest

https://pytest-django.readthedocs.io/en/latest/database.html#no-migrations-disable-django-migrations

Basically it just ignores all your migrations and generates the DB based on the current state of the DB model classes. I guess the theory is you don't need to re-play the entire migration history (including hand-written data backfills, etc) to just build an empty DB.

urlsangel commented 1 year ago

Thanks for the clarification @chris48s, I should have been clearer!

@zerolab - I don't think we can run our tests with migrations, so not sure if we can test code that uses TranslationSource from wagtail-localize with the current codebase?

chris48s commented 1 year ago

Also, TIL this is something django can do out of the box, independent of pytest using the settings['TEST']['MIGRATE'] setting: https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-TEST_MIGRATE

Given that, it doesn't seem ridiculous to handle this.

drcongo commented 12 months ago

For anyone finding this through search, pytest lets you monkeypatch stuff inside packages, and reading through the code that's accessing the table here it looks like in places it's happy to handle an empty string, so we're working around it for now by monkeypatching the get_schema_version function in the tests that it breaks...


def _no_migrations_access(*args, **kwargs):
    return ""

def test_article_share_header(benchmark, article_page, languages, monkeypatch):
    # Monkey patch because wagtail_localize.models.get_schema_version tries to access
    # the django migrations table which isn't guaranteed to exist
    import wagtail_localize
    monkeypatch.setattr(wagtail_localize.models, "get_schema_version", _no_migrations_access)
    [...]

https://docs.pytest.org/en/7.1.x/how-to/monkeypatch.html#monkeypatching-functions

edit to add: A longer term, fairly simple fix, as @zerolab mentioned, would be to just pop a try / except inside that function, returning an empty string if table access fails.

def get_schema_version(app_label):
    """
    Returns the name of the last applied migration for the given app label.
    """
    try:
        migration = (
            MigrationRecorder.Migration.objects.filter(app=app_label)
            .order_by("applied")
            .last()
        )
        if migration:
            return migration.name
    except OperationalError:
        return ""
    return ""

however, I only use pytest, so I'm not sure if this fix would break something in UnitTest.

zerolab commented 12 months ago

712 if you have the time to ~browse~ test

drcongo commented 12 months ago

Amazing, thanks @zerolab