-
Notifications
You must be signed in to change notification settings - Fork 143
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 testing for current Django versions #200
Conversation
Codecov Report
@@ Coverage Diff @@
## master #200 +/- ##
==========================================
- Coverage 83.92% 78.26% -5.66%
==========================================
Files 77 77
Lines 3004 3004
==========================================
- Hits 2521 2351 -170
- Misses 483 653 +170
Continue to review full report at Codecov.
|
All the Django tests have the following failure:
I'm seeing this even on environments that should be supported (Python 3.6, Django 1.10 and 1.11). I'm familiar with Django, but not with the xray sdk. If a maintainer can point me in the right direction, I'm happy to try to fix this issue. |
Hi @ipmb, |
Hi @ipmb , Is this the(#200 (comment)) snippet of your local machine error? I think Travis CI is passing fine for other PRs (#204) . So it means Django tests passes with Django (1.10 and 1.11 and python 3.6) |
Thanks for the tip @bhautikpip. I'm seeing the errors both locally and on Travis. It looks like the issue is that the tests fail when only run against coverage run --source aws_xray_sdk -m py.test tests/ext/django I see this on both this branch and master. In order to test against the different Django versions, I'm trying to isolate the Django-specific tests so we can run them in their own tox environments. Could you suggest a way to run just the django tests? |
I don't see any reason why tests fails when running against tests/ext/django. I think the command |
Here's a minimal FROM python:3.7
WORKDIR /app
ENV DJANGO_SETTINGS_MODULE=tests.ext.django.app.settings
COPY . .
RUN pip install -e . pytest 'django>=1.10,<2.0' django-fake-model
CMD pytest tests/ext/django If you run this, you'll get the same error:
|
329ab77
to
fef67e2
Compare
Previously the patch was global which caused the django tests to fail if the sqlite3 tests weren't also run.
tests/ext/django/app/settings.py
Outdated
@@ -44,7 +44,6 @@ | |||
'django.middleware.common.CommonMiddleware', | |||
'django.middleware.csrf.CsrfViewMiddleware', | |||
'django.contrib.auth.middleware.AuthenticationMiddleware', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove this 'django.contrib.auth.middleware.SessionAuthenticationMiddleware' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SessionAuthenticationMiddleware class is removed. It provided no functionality since session authentication is unconditionally enabled in Django 1.10.
https://docs.djangoproject.com/en/2.0/releases/2.0/#miscellaneous
Would you mind resolving the conflict ? This should be good to go! |
@bhautikpip The merge conflict is resolved. |
#206 adds support in the code. |
Okay. I see. Thanks for clarifying! |
refs #10 #85
Description of changes:
Update tox so it will test against currently supported versions of Django.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.