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

Add Django 2.2 to test matrix and support using Django 2.2 as a dependency. #951

Merged
merged 11 commits into from Jul 3, 2019

Conversation

Projects
None yet
2 participants
@jchristgit
Copy link
Contributor

commented Apr 20, 2019

No description provided.

jchristgit added some commits Apr 20, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 20, 2019

Codecov Report

Merging #951 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #951   +/-   ##
=======================================
  Coverage   75.47%   75.47%           
=======================================
  Files         107      107           
  Lines        4358     4358           
=======================================
  Hits         3289     3289           
  Misses       1069     1069

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 621f608...5abe7e2. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Apr 20, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@1f6de54). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #951   +/-   ##
=========================================
  Coverage          ?   75.47%           
=========================================
  Files             ?      107           
  Lines             ?     4358           
  Branches          ?        0           
=========================================
  Hits              ?     3289           
  Misses            ?     1069           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f6de54...6d15f80. Read the comment docs.

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

I'm a bit puzzled as to what's going on in Travis as this works fine for me when testing locally (at least on 3.7) and Travis seems to just be running tox.. Any idea what might be going wrong there?

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@jchristgit I'm back after a break, sorry about the slow response. I released a new django-nyt with Django 2.2 support. I'm wondering why all these tests are failing with django.db.utils.OperationalError: no such table: auth_group - do you have time to investigate?

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

Seems like I confused something earlier when I said that tests pass locally, it seems I misread the output. I initially assumed this was caused by the following:

Tests will fail on SQLite if apps without migrations have relations to apps with migrations. This has been a documented restriction since migrations were added in Django 1.7, but it fails more reliably now. You’ll see tests failing with errors like no such table: <app_label>_. This was observed with several third-party apps that had models in tests without migrations. You must add migrations for such models.

(see https://docs.djangoproject.com/en/2.2/releases/2.2/), which kind of seems to be the cause for my local no such table: wiki_article__old error. However, this only seems to occur on Django 1.11 - for 2.0, 2.1 and 2.2 it seems to work just fine. This is somewhat contradictory to the changelog to me.

A quick grep links to django.db.backends.sqlite3.schema and some searching links me to this: https://code.djangoproject.com/ticket/29182 which sounds like it's a Django bug on 1.11. Travis CI is suggesting the exact opposite though :/

I'll try to somehow reproduce the issue on Travis locally but I'm not sure how. I'm curious, do you get the same error when running locally?

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Hi @jchristgit - are you running tests with tox, i.e. an isolated environment? For instance tox -e py36-django111 for Python 3.6 and Django 1.11..

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 12, 2019

However, this only seems to occur on Django 1.11

I can't reproduce this locally, nope. I'm also wondering which applications without migrations that would be referenced. There are applications (plugins) without models and migrations, but since they don't have models, they cannot be referenced from other migrations.

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

I am running with tox, yes, tests only run on 3.7 (system Python) though.

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@jchristgit if your system's Python is 3.7, then you can only run those tests, otherwise you'd have to install the other version of Python on your system.

Question is if you can reproduce the Travis errors?

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

It doesn't look like it, no. I was expecting some form of special test command being run on Travis but it seems like Travis is just running tox directly, so there shouldn't be any differences there either. What confuses me is that I get test failures for Django 1.11 locally due to the error mentioned above but 2.2 appears to work just fine. Maybe it's some issue with the pip cache?

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 19, 2019

@jchristgit Travis CI uses a plugin for tox, travis-tox which automatically selects the test environment (python version etc) from the Travis environment.

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Hmm, okay. That means the Travis tests should run similar to those locally, and looking at .travis.yml doesn't suggest anything special like an external database being used instead of SQLite. Maybe there is some variable configured on the Travis CI website that causes the test to run differently that is not visible in .travis.yml?

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 22, 2019

No, but they are run on Ubuntu 14.04 and 16.04, it's visible in the top of each build. They run different versions of the SQLite3 library, but apart from that I'm not sure what could cause the difference.

As I said, I couldn't reproduce your local build error. Do you have any?

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Sorry, then I misunderstood your earlier comment, I thought you had the same error as I did but couldn't reproduce the on on Travis.

I can still reproduce the local error every time. You saying that you can't reproduce my local build error seems to suggest that there must be something going on with the system packages here:

  • Travis CI runs on Ubuntu 14.04 and Ubuntu 16.04 and the tests fail on both on 2.2, suggesting the SQLite migration change might be the issue
  • I run Debian testing (buster) and get another issue pointing me towards a Django 1.11 bug regarding SQLite tables in tests, but 2.2 runs just fine

On which distribution & release are you running the tests? The SQLite library version could be the issue, mixed with the versions that seems to make some sense...

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Are you running on a clean tox environment? In your repo checkout, remove the .tox/ folder.

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Am running tox -e py36-django111 on Ubuntu 18.04 and that works fine. Which environment are you using?

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

From my understanding I'm testing on all environments just running tox. The relevant bits of the output:

ERROR:  py34-django111: InterpreterNotFound: python3.4
ERROR:  py34-django20: InterpreterNotFound: python3.4
ERROR:  py34-django21: InterpreterNotFound: python3.4
ERROR:  py34-django22: InterpreterNotFound: python3.4
ERROR:  py35-django111: InterpreterNotFound: python3.5
ERROR:  py35-django20: InterpreterNotFound: python3.5
ERROR:  py35-django21: InterpreterNotFound: python3.5
ERROR:  py35-django22: InterpreterNotFound: python3.5
ERROR:  py36-django111: InterpreterNotFound: python3.6
ERROR:  py36-django20: InterpreterNotFound: python3.6
ERROR:  py36-django21: InterpreterNotFound: python3.6
ERROR:  py36-django22: InterpreterNotFound: python3.6
ERROR:   py37-django111: commands failed
  py37-django20: commands succeeded
  py37-django21: commands succeeded
  py37-django22: commands succeeded
ERROR:  lint: InterpreterNotFound: python3.4

Clearing the tox environment by deleting .tox and re-running tox reports the exact same.

For what it's worth, I've installed 3.6.8 via pyenv and ran the tests on there, but that one results in the same error as on Python 3.7.

Which SQLite library version are you using? Via dpkg --list | grep sqlite, I see the following:

ii  libsqlite3-0:amd64                    3.27.2-2                             amd64        SQLite 3 shared library
ii  libsqlite3-dev:amd64                  3.27.2-2                             amd64        SQLite 3 development files
@benjaoming

This comment has been minimized.

Copy link
Member

commented May 23, 2019

InterpreterNotFound is because you don't have the Python installation on your system. Hence we use Travis for such things. This is not a problem.

What's the error(s) that you have when running py37-django111 on a clean .tox? The same?

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

I'm aware that InterpreterNotFound is not a problem, I was referring to the failed test on 1.1. Yes, running tox with a removed .tox yields the same results for py37-django111.

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Sorry, haha, I missed this: Django 1.11 doesn't support Python 3.7 or vice versa. It's not an issue :)

@benjaoming

This comment has been minimized.

Copy link
Member

commented May 23, 2019

py37-django111 isn't in the Travis test matrix :)

@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Okay, that explains my local error 😄 - I've changed tox.ini to only test for Django 2.0+ on Python 3.7, and now (for the Python versions that tox runs with here) it works without complaints.

That doesn't explain the error on Travis though - auth_group is an internal table and from my understanding both Travis and I run the test using sqlite via tox (apart from the different test environments)..

benjaoming and others added some commits Jun 9, 2019

@benjaoming

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

From the errors, it looks like the way that the test model testdata.CustomUser is constructed doesn't work anymore (in Django 2.2).

self = <django.db.backends.sqlite3.base.SQLiteCursorWrapper object at 0x7f110f25b0d8>
query = '\n                            SELECT REFERRING.`id`, REFERRING.`group_id` FROM `testdata_customuser_groups` as REFERR...                        WHERE REFERRING.`group_id` IS NOT NULL AND REFERRED.`id` IS NULL\n                            '
@jchristgit

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

That sounds like an idea, but if it is a framework issue, why would it break only on Travis? I think the issue must be on the system somewhere.

@benjaoming benjaoming referenced this pull request Jun 11, 2019

Closed

Django 2.1.9 coverage #954

@benjaoming

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I'm still not sure why sqlite3 version 3.11 on Travis has this issue. Still cannot reproduce locally. There are several release notes for Django 2.2 about SQLite and tests, so definitely something has changed.

Addressing following note in 727343b

Tests will fail on SQLite if apps without migrations have relations to apps with migrations. This has been a documented restriction since migrations were added in Django 1.7, but it fails more reliably now. You’ll see tests failing with errors like no such table: <app_label>_. This was observed with several third-party apps that had models in tests without migrations. You must add migrations for such models.

@benjaoming

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Another interesting release note:

Deferrable database constraints are now checked at the end of each TestCase test on SQLite 3.20+, just like on other backends that support deferrable constraints. These checks aren’t implemented for older versions of SQLite because they would require expensive table introspection there.

@benjaoming

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

LOL... I missed that... the last commit just made things work ONLY on Django 2.2 :D

image

@benjaoming

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Of course:

django.db.migrations.exceptions.NodeNotFoundError: Migration testdata.0001_initial dependencies reference nonexistent parent node ('auth', '0011_update_proxy_permissions')

@benjaoming benjaoming force-pushed the jchristgit:add-django-2-2 branch from 727343b to 6d15f80 Jul 3, 2019

@benjaoming

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

🎉

It was because the test application testdata didn't have migrations.

@benjaoming

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I know my latest commit message "Add migrations for testdata app because of Django 2.2 release note" sounds weird... but hey, I did add the migrations because of the release notes, and NOT because of the super weird error message.

@benjaoming benjaoming merged commit 0fd6df9 into django-wiki:master Jul 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjaoming

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Hmm, I see now that targeting this for master was a mistake, we need a PR for releases/0.4.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.