Skip to content

Run the entire test suite using pytest #109

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

Merged
merged 5 commits into from
Mar 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,10 @@ jobs:
- name: Run application
run: docker-compose up -d web

- name: Test Resources
run: docker-compose run --rm manage test resources
if: always()

- name: Test Users
run: docker-compose run --rm manage test users
if: always()

- name: Test UserAuth
run: docker-compose run --rm manage test userauth
- name: Run test suite
run: docker-compose run -T --rm app pytest -v
if: always()

- name: Check Migrations are up-to-date
run: docker-compose run --rm manage makemigrations --check
run: docker-compose run -T --rm app ./manage.py makemigrations --check
if: always()
17 changes: 11 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ To stop the application and remove all containers, run the following.
docker-compose down
```

5. Create a superuser so that you can log into http://localhost:8000/admin by running the following in your terminal: `$ docker-compose run --rm manage createsuperuser`
5. Create a superuser so that you can log into http://localhost:8000/admin by running the following in your terminal: `$ docker-compose run --rm app ./manage.py createsuperuser`

## Editing Code

Expand Down Expand Up @@ -89,17 +89,22 @@ If you would like to tail the logs in the console then you remove the detach fla

The following are examples of some common Django management commands that you may need to run.

* Make Migrations: `docker-compose run --rm manage makemigrations`
* Merge Migrations: `docker-compose run --rm manage makemigrations --merge`
* Run Migrations: `docker-compose run --rm manage`
* Test: `docker-compose run --rm manage test`
* Make Migrations: `docker-compose run --rm app ./manage.py makemigrations`
* Merge Migrations: `docker-compose run --rm app ./manage.py makemigrations --merge`
* Run Migrations: `docker-compose run --rm app ./manage.py migrate`

To see the full list of management commands use `help`.

```plain
docker-compose run --rm manage help
docker-compose run --rm app ./manage.py help
```

### Automated Tests

* We use [pytest](https://docs.pytest.org/en/latest/contents.html) with the [pytest-django](https://pytest-django.readthedocs.io/en/latest/) plugin for running tests.
* Please add tests for your code when contributing.
* Run the test suite using `docker-compose run --rm app pytest`

### Import Postman collection
Postman is a free interactive tool for verifying the APIs of your project. You can download it at postman.com/downloads.

Expand Down
16 changes: 1 addition & 15 deletions docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ services:
sh -c "python /opt/codebuddies/manage.py collectstatic --clear --no-input &&
python /opt/codebuddies/manage.py migrate &&
uwsgi --ini /opt/codebuddies/uwsgi.ini"
working_dir: /opt/codebuddies
volumes:
- ./project:/opt/codebuddies
environment:
Expand Down Expand Up @@ -76,18 +77,3 @@ services:
restart: on-failure
ports:
- 8025:8025

# Manager allows you to run Django management tasks on the application.
manage:
container_name: manage
restart: on-failure
build: ./project
command: shell
entrypoint: /usr/local/bin/python3 /opt/codebuddies/manage.py
volumes:
- ./project:/opt/codebuddies
environment:
- DATABASE_URL=postgres://babyyoda:mysecretpassword@db:5432/codebuddies
- EMAIL_HOST=mailhog
depends_on:
- db
7 changes: 0 additions & 7 deletions project/config/settings/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
"DJANGO_SECRET_KEY",
default="AcnZMguCngDeMgbwXf6l9d2arow2lU9ZkDweNnoCZmZ2qH6pinB4tLhEYI4Fgf6Y",
)
# https://docs.djangoproject.com/en/dev/ref/settings/#test-runner
TEST_RUNNER = "django.test.runner.DiscoverRunner"

# CACHES
# ------------------------------------------------------------------------------
Expand All @@ -27,11 +25,6 @@
}
}

# PASSWORDS
# ------------------------------------------------------------------------------
# https://docs.djangoproject.com/en/dev/ref/settings/#password-hashers
PASSWORD_HASHERS = ["django.contrib.auth.hashers.MD5PasswordHasher"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I've done this is because the passwords in the fixture files were encrypted using the default hasher, so this was causing all the auth-based tests to fail (the hashes didn't match). This didn't manifest when running the tests with django's own runner because it wasn't set up to use this file at all - it was using local.py.

Another approach would have been to rebuild all the fixtures, but this was easier (and makes a minimal difference to test suite speed). If you end up switching all your tests to create test data on-the-fly using FactoryBoy or whatever, you could re-enable this micro-optimisation at a later date.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a community discussion about which hashing method we want to use for passwords in the DB - we're using Argon2 ATM (recommended in the Django docs - but not the default, which is PBKDF2), which is why the fixutres have the hashes they do. So I recommend using django.contrib.auth.hashers.Argon2PasswordHasher so that the two match.


# TEMPLATES
# ------------------------------------------------------------------------------
TEMPLATES[0]["OPTIONS"]["loaders"] = [ # noqa F405
Expand Down
File renamed without changes.
8 changes: 4 additions & 4 deletions project/core/templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -140,19 +140,19 @@ <h2>Application urls</h2>
<h2>docker-compose commands</h2>
<ul>
<li>
<code>docker-compose run --rm manage createsuperuser</code> - Command
<code>docker-compose run --rm app ./manage.py createsuperuser</code> - Command
to create a superuser
</li>
<li>
<code>docker-compose run --rm manage tests</code>- Command to run
<code>docker-compose run --rm app pytest</code> - Command to run
tests
</li>
<li>
<code>docker-compose run --rm makemigrations</code> - Command to make
<code>docker-compose run --rm app ./manage.py makemigrations</code> - Command to make
migrations (necessary if you change the models)
</li>
<li>
<code>docker-compose run --rm migrate</code> - Command to migrate the
<code>docker-compose run --rm app ./manage.py migrate</code> - Command to migrate the
migrations (necessary if you ran the previous makemigrations step)
</li>
</ul>
Expand Down
5 changes: 5 additions & 0 deletions project/pytest.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[pytest]
python_files = tests.py test_*.py *_tests.py
norecursedirs = staticfiles .git templates __pycache__
DJANGO_SETTINGS_MODULE = config.settings.test
FAIL_INVALID_TEMPLATE_VARS = 1
15 changes: 8 additions & 7 deletions project/resources/tests.py
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
from rest_framework import status, serializers
from rest_framework import status
from rest_framework.test import APITestCase
from rest_framework_jwt.settings import api_settings
from django.core.management import call_command

jwt_payload_handler = api_settings.JWT_PAYLOAD_HANDLER
jwt_encode_handler = api_settings.JWT_ENCODE_HANDLER

class ResourcesTests(APITestCase):

def setUp(self):
call_command('loaddata', 'users.json', verbosity=0)
call_command('loaddata', 'resources.json', verbosity=0)
call_command('loaddata', 'tagging.json', verbosity=0)
call_command('loaddata', 'taggeditems.json', verbosity=0)
fixtures = [
'users',
'resources',
'tagging',
'taggeditems'
]

def setUp(self):
url = '/auth/obtain_token/'
#to do: choose a user at random from loaded users.json
data = {"username": "JuJu", "password": "codebuddies"}
Expand Down
16 changes: 8 additions & 8 deletions project/userauth/tests.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import unittest
from unittest.mock import patch
from rest_framework import status, serializers
from rest_framework.test import APITestCase
from rest_framework_jwt.settings import api_settings
from django.core.management import call_command
from django.contrib.auth import get_user_model


Expand All @@ -12,14 +10,16 @@

class UserauthTests(APITestCase):

fixtures = ['users']

def setUp(self):
"""
Loads users.json fixture into test DB and directly creates a new user.
"""
call_command('loaddata', 'users.json', verbosity=0)
# create a new user
model = get_user_model()
self.person = model.objects.create_user(username='PetuniaPig', email='[email protected]',
password='codebuddies')
self.person = model.objects.create_user(
username='PetuniaPig',
email='[email protected]',
password='codebuddies'
)


def test_jwt_not_authed(self):
Expand Down
1 change: 0 additions & 1 deletion project/users/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,5 @@ class User(AbstractUser):
# around the globe.
name = CharField(_("Name of User"), blank=True, max_length=255)

@property
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we need to remove this decorator, out of curiosity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is explained in the commit message for 03770f4

To work through a more concrete example of the problem this causes:

property:

  • Declare get_absolute_url() as a property
  • Go to http://127.0.0.1:8000/admin/users/user/
  • Click on any user
  • Click the "view on site" link in the top-right
  • It will throw 'str' object is not callable: you can't invoke a string.

method:

  • Now switch it to a method
  • Click the "view on site" link in the top-right - it will issue a redirect.

Side note: It doesn't issue a redirect to a useful URL because of

but lets deal with that as another issue. I'm trying to avoid scope-creep too much in this PR and just focus on the tests. The reason I fixed the signature in this PR which addresses testing (rather than treating it as a seperate bug) is because it was causing a test to fail. Its just that nobody has noticed because you weren't running all the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On closer inspection, we should remove the redirect method altogether, & also remove the [view on site] button from the Admin. (see this for how: https://stackoverflow.com/questions/8662359/django-remove-view-on-site-button-in-the-admin-user-change-form). I think this decorator + method was either left from Coookiecutter or cargo-culted in by myself when piecing together userauth. Since we are mostly building an API, the redirect for viewing user detail/user profile on the site won't ever work -- unless we point it at a react URL. If we do provide a user profile or user detail, it will be as an endpoint, and deliver JSON -- not an HTML template/form.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open an issue for that (and probably chuck it into the PR for our testing re-write)...unless there are any objections??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'view on site' button is probably not that helpful for an API project, but I picked it as an example of an internal call that expects get_absolute_url to be a method which exists at the moment, but I think there are other internal calls in django (and possibly DRF) that expect that signature.

If you reeeally want a property version, we could also declare

@property
def absolute_url(self):
    return self.get_absolute_url()

and probably chuck it into the PR for our testing re-write

It can be done as its own PR - should be no dependency/merge conflict there

def get_absolute_url(self):
return reverse("users:detail", kwargs={"username": self.username})