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

Run Django checks (inc. missing migrations) as part of CI #7558

Closed
wants to merge 3 commits into from
Closed

Run Django checks (inc. missing migrations) as part of CI #7558

wants to merge 3 commits into from

Conversation

hugorodgerbrown
Copy link
Contributor

@hugorodgerbrown hugorodgerbrown commented Sep 28, 2020

Description

Refs #7554/#7557

This PR adds a script called runchecks.py which is analagous to runtests.py, but used to run Django admin checks as part of CI. This will fail if any errors are raised. In addition to a straight ./manage.py check command, it will run the makemigrations command in "dry-run" mode, failing if any missing migrations are detected.

It is equivalent to running these two commands:

$ python manage.py check --fail-level ERROR
$ python manage.py makemigrations --dry-run --check --verbosity 3

Running this against the 3.12 release (missing a migration) gives this:

django-rest-framework ❯ ./runchecks.py
Running basic Django system checks
System check identified no issues (0 silenced).

Checking for missing Django migrations
Migrations for 'authtoken':
  rest_framework/authtoken/migrations/0003_tokenproxy.py
    - Create proxy model TokenProxy
Full migrations file '0003_tokenproxy.py':
# Generated by Django 3.1.1 on 2020-09-28 16:42

from django.db import migrations


class Migration(migrations.Migration):

    dependencies = [
        ('authtoken', '0002_auto_20160226_1747'),
    ]

    operations = [
        migrations.CreateModel(
            name='TokenProxy',
            fields=[
            ],
            options={
                'verbose_name': 'token',
                'proxy': True,
                'indexes': [],
                'constraints': [],
            },
            bases=('authtoken.token',),
        ),
    ]

And returns with a non-zero exit code, thereby failing the Travis build.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

good stuff for CI!

@adamchainz
Copy link
Contributor

Django's test framework runs checks at the start of the test run. pytest-django is missing this. Could you merge this with runtests.py to avoid an extra CI action? Both checks and 'makemigrations' are pretty fast so the overhead shouldn't be noticeable.

@hugorodgerbrown
Copy link
Contributor Author

@adamchainz If I move the checks into runtests.py, they will run on every travis test run - which means 14 times (every combination of Django/Python). As a separate check they only get run once, as a sort of a backstop (to prevent the issue with the missing migration). Is that what you want?

@adamchainz
Copy link
Contributor

Yes. Running them multiple times is not really an issue since the overhead is measured in milliseconds. I think it's worth it for a simpler test structure, and it also means they'll be run locally without needing an extra environment, or thought about it. As I said, this is the default behaviour with Django's test framework and it should be pytest-django's behvaiour too - I just never quite figured out a good way of doing it - pytest-dev/pytest-django#414 .

@stale
Copy link

stale bot commented May 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 1, 2022
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

rebase please

@stale stale bot removed the stale label May 1, 2022
@stale
Copy link

stale bot commented Jul 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants