st4lk / django-rest-social-auth

OAuth signin with django rest framework
MIT License
521 stars 122 forks source link

Freeze knox version in tests #89

Closed st4lk closed 5 years ago

jayvdb commented 5 years ago

Why is django-rest-knox 4 not acceptable? I've packaged v4, but I can choose a lower version if necessary

st4lk commented 5 years ago

Because current tests are failing with it. Therefore, I'm not sure that current version of 'django-rest-social-auth' will work with django-rest-knox 4 (I suppose some backwards incompatible changes where made in knox 4).

https://travis-ci.org/st4lk/django-rest-social-auth/builds/518225099

st4lk commented 5 years ago

Just need more time to investigate it.

jayvdb commented 5 years ago

ok, I'll try knox 3.6. I'll ping you if I run into troubles with that.

jayvdb commented 5 years ago

@st4lk I do have a problem, and I've ruled out the basics, but likely my problem is very stupid.

The following occurs on python 2 & 3, against Django 2.2, but I've mostly ruled out dj22 as the problem.

My .spec is at https://build.opensuse.org/package/show/home:jayvdb:django/python-rest-social-auth

It is using the github release tarball, and I patch it with https://github.com/st4lk/django-rest-social-auth/commit/648d1c44f463a4896de26a061df6e104bf553bcc.patch

My %check block is

export PYTHONPATH=${PWD}/example_project
export DJANGO_SETTINGS_MODULE=config.settings

# RuntimeError: Model class knox.models.AuthToken doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS.
# Uncomment the next line for all tests to pass
# rm tests/test_knox.py

# simple-jwt is Python 3 only
python2 -m pytest --ignore tests/test_simple_jwt.py
python3 -m pytest

One of the logs is https://build.opensuse.org/package/live_build_log/home:jayvdb:django/python-rest-social-auth/openSUSE_Factory_zSystems/s390x , but you'll need to create an account to access that, as they hide logs from the public. The intel jobs are backed up, so their logs usually disappear, but I see the same on my intel laptop.

platform linux2 -- Python 2.7.15, pytest-3.10.1, py-1.8.0, pluggy-0.9.0
Django settings: config.settings (from environment variable)
rootdir: /home/abuild/rpmbuild/BUILD/django-rest-social-auth-2.1.0, inifile: pytest.ini
plugins: django-3.4.8
collected 22 items / 1 errors

==================================== ERRORS ====================================
_____________________ ERROR collecting tests/test_knox.py ______________________
tests/test_knox.py:2: in <module>
    from knox.auth import TokenAuthentication as KnoxTokenAuthentication
/usr/lib/python2.7/site-packages/knox/auth.py:20: in <module>
    from knox.models import AuthToken
/usr/lib/python2.7/site-packages/knox/models.py:27: in <module>
    class AuthToken(models.Model):
/usr/lib/python2.7/site-packages/django/db/models/base.py:118: in __new__
    "INSTALLED_APPS." % (module, name)
E   RuntimeError: Model class knox.models.AuthToken doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS.
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
st4lk commented 5 years ago

@jayvdb Have you added 'knox' to INSTALLED_APPS of your django settings? Like here (it is commented in example project, need to uncomment if you need knox): https://github.com/st4lk/django-rest-social-auth/blob/master/example_project/config/settings.py#L45

st4lk commented 5 years ago

Ah ok, some tests are failing. Will check

st4lk commented 5 years ago

@jayvdb Can you confirm, that https://github.com/st4lk/django-rest-social-auth/blob/master/pytest.ini is taken into account? I.e. problem is with this:

Django settings: config.settings (from environment variable)

It should be config.settings_test, not config.settings

jayvdb commented 5 years ago

Ah, well I am setting the DJANGO_SETTINGS_MODULE as mentioned above.

anyways, I add the following and builds are green

# Enable knox in tests
sed -i "s/# 'knox'/'knox'/" example_project/config/settings.py
st4lk commented 5 years ago

It may be ok as quick solution, but looks dangerous (and hard to support in future)... If it is possible to find a way to specify settings_test - that will be safer. settings_test will use sqlite-in-memory database as well, instead of creating db.sqlite3 file every time. I'm not sure that it will always work correctly (I mean tests).

jayvdb commented 5 years ago

Since I have you here, and it is relevant to the above, one way to improve the packaging is to add the LICENSE and all the test 'stuff' to the sdist which is upload to PyPI, and then the wheels are shipped without the tests.

The tests, test data, and extra files needed, will make the sdist a bit larger, but given that real users are fetching wheels, the main consumers of the sdist are people wanting to play with the source, and they need the extra bits like pytest.ini which you mention above :-). I also love to see a tox.ini in an sdist. It would ideally be almost the same as the github archive. And then to get extra distro customers, python setup.py test from a clean extra of the sdist should run all tests - when that works, maintaining the package is easy, because then setuptools checks the dependency versions whenever the package is upgraded, so it isnt possible to forget to increase a minimum version.

st4lk commented 5 years ago

Thanks for feedback, that makes sense.

jayvdb commented 5 years ago

If you like, and dont object to having the sdist increase in size a bit, I can send a PR which does all that.

st4lk commented 5 years ago

PRs are always welcome :)