Testing and quality upgrades #123
Conversation
- Also disables Python 3 checks as there are many issues that will need to be addressed when the time comes to upgrade.
bmedx
force-pushed
the
bmedx/test_and_quality_upgrades
branch
from
e532c5e
to
22d5509
Compare
| .tox | ||
| .pytest_cache |
There was a problem hiding this comment.
Nice, that's a recent improvement: pytest-dev/pytest#3138 . Used to be .cache, which made it unclear at a glance where it was coming from.
| known_first_party = | ||
| workbench | ||
| sample_xblocks | ||
| sections = FUTURE,STDLIB,THIRDPARTY,DJANGO,DJANGOAPP,EDX,FIRSTPARTY,LOCALFOLDER |
There was a problem hiding this comment.
We should probably standardize on an order for these pretty soon; I think the cookiecutter and edx-platform still conflict (this looks like the cookiecutter order).
There was a problem hiding this comment.
Agreed. This is the cookiecutter version, which I tend to use as the canonical way of doing things for these app / xblock repos (for better or worse, I just want things to be as consistent as they can be).
There was a problem hiding this comment.
Ugh - not looking forward to going back into edx-platform and running isorts again....
|
|
||
| script: | ||
| - tox | ||
|
|
||
| after_success: | ||
| - "pip install coveralls" | ||
| - "coveralls" | ||
| - codecov |
There was a problem hiding this comment.
Has devops enabled codecov integration for the repo yet?
There was a problem hiding this comment.
I was waiting to see if it fired off a hook, but never did so I'll file a ticket. Edit: filed as DEVOPS-7164
|
|
||
| install: | ||
| - "pip install tox-travis" | ||
| - pip install tox-travis | ||
| - pip install -r requirements/tox.txt |
There was a problem hiding this comment.
This doesn't need to be fixed right now, but the pip-tools PR should generate a travis.txt or such which includes tox-travis.
| .PHONY: cover | ||
| cover: | ||
| coverage run manage.py test | ||
| coverage report |
There was a problem hiding this comment.
Do we want to keep a target for generating the coverage report? The cookiecutter has one that launches the HTML report in a browser window.
There was a problem hiding this comment.
I had a version of it in the next round of changes, which pulls in most of the cookiecutter Makefile. Once this goes in I'll make a new branch of those changes and push it so whoever gets to this next has a starting place a little further along.
| from mock import Mock | ||
| from xblock.runtime import DictKeyValueStore, KvsFieldData | ||
| from xblock.test.tools import TestRuntime, assert_equals, assert_in | ||
| from xblock.test.tools import TestRuntime as TR |
There was a problem hiding this comment.
I'm curious, what prompted using this alias?
There was a problem hiding this comment.
Pytest was trying to collect it as a test due to the name and was throwing a warning about it having an init. It should really be renamed in xblock, but I'm already pretty far afield in doing this PR. I'll comment it to explain.
setup.cfg
Outdated
| @@ -9,8 +9,5 @@ rednose=1 | |||
| [pep8] | |||
| exclude=src/,.* | |||
There was a problem hiding this comment.
With the pycodestyle section in tox.ini, do we still need this here?
tox.ini
Outdated
| django111: Django>=1.11,<1.12 | ||
| passenv = | ||
| DISPLAY | ||
| commands = | ||
| make var/workbench.db | ||
| make test | ||
| py.test {posargs} |
There was a problem hiding this comment.
"pytest" has been recommended over "py.test" since the 3.0.0 release.
There was a problem hiding this comment.
I'll update it here, but this actually came in from the cookiecutter template so we should fix it there too.
|
|
||
| from workbench import scenarios | ||
| from workbench.runtime_util import reset_global_state | ||
|
|
||
| pytestmark = pytest.mark.django_db # pylint: disable=invalid-name |
There was a problem hiding this comment.
Is this actually being used? Same question on the other files defining it.
There was a problem hiding this comment.
Yes, from what I read it's the best practice way to get db access under pytest_django so that we're explicit about which tests hit the database. If we know of a better way of doing this I'm happy to change the pattern, though.
There was a problem hiding this comment.
Ah, I'd missed the docs on applying a mark to an entire module. Good to know.
workbench/wsgi.py
Outdated
| from django.core.wsgi import get_wsgi_application | ||
|
|
||
| os.environ.setdefault("DJANGO_SETTINGS_MODULE", "workbench.settings") |
There was a problem hiding this comment.
I think this actually does need to come before the get_wsgi_application import; there's a from django.conf import settings in one of the indirectly imported modules.
There was a problem hiding this comment.
Ah yes, an overzealous isort.
|
@jmbowman @doctoryes I think all of the applicable feedback has been addressed. Should be good for re-review. |
| known_first_party = | ||
| workbench | ||
| sample_xblocks | ||
| sections = FUTURE,STDLIB,THIRDPARTY,DJANGO,DJANGOAPP,EDX,FIRSTPARTY,LOCALFOLDER |
There was a problem hiding this comment.
Ugh - not looking forward to going back into edx-platform and running isorts again....
Still to do: Clean up requirements, implement pip-compile