-
Notifications
You must be signed in to change notification settings - Fork 41
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
[WIP][Update] Django Channels #8
Conversation
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.
It's looking promising @atb00ker 👍
74121e9
to
8050054
Compare
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.
I'm working to fix the outstanding issues, so you don't need to change this PR (please don't introduce changes or I won't be able to merge easily).
I'm leaving some comments about minor things only so that you can learn to avoid those in the future ok?
@@ -0,0 +1,3 @@ | |||
#!/bin/bash | |||
set -e | |||
pytest |
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.
Please note that the final new line at the end of the file is missing, you may need to configure your editor to add finalnew lines automatically
[pytest] | ||
DJANGO_SETTINGS_MODULE = tests.test_settings | ||
# -- read python files format: | ||
python_files = pytest_*.py |
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.
Please note that the final new line at the end of the file is missing, you may need to configure your editor to add finalnew lines automatically
try: | ||
from .local_settings import * | ||
except: #noqa | ||
pass |
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.
is this file really needed? local_settings
is already imported in settings.py
so why not use the main settings directly?
If we can avoid adding files we should do so, the less code we have to maintain, the better.
|
||
script: | ||
- coverage run --source=django_loci runtests.py | ||
- py.test --cov=django_loci | ||
- coverage run -a --source=django_loci runtests.py |
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.
so if you manage to call pytests
in runtests.py
we may be able to fix coverage
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 coverage from both the tests is added.
-a
option in coverage run -a --source=django_loci runtests.py
appends the results from tests to the results of py.test --cov=django_loci
.
The problem is only with one file routing.py
try: | ||
user = self.scope["user"] | ||
self.pk = self.scope['url_route']['kwargs']['pk'] | ||
except Exception: |
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.
catching the generic Exception
is generally a bad practice
''' | ||
In channels 2.x, Websockets can only be tested | ||
asynchronously, hence, pytest is used for these tests. | ||
''' |
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.
for consistency, use double quotes for docstrings
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.
## Do not Merge ##
Tasks remaining before Merge:
test_update_location()
with the new version of channels, i need help with it.routing.py
belong to.coveragerc
, under omit section? The file is called during the tests yet that doesn't reflect in the coverage!WIP: #7
PR Forwarded, Please Read: #9