Run the entire test suite using pytest #109

chris48s · 2020-03-12T21:06:19Z

Main changes in this PR:

🔧 Configure pytest
👀 Get it to discover and run the full test suite
✔️ Get all the existing unit tests passing
📚 Update CI build and docs

Closes #107

load fixtures declaratively we don't need to call_command('loaddata') by hand remove unused imports

chris48s · 2020-03-12T21:21:59Z

project/config/settings/test.py

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


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.

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.

chris48s · 2020-03-12T22:34:15Z

Hrmm. This is close, but not quite ready for review. I've marked this WIP for now as I've run out of time to fiddle with it this evening and there's clearly a bit of a hole in my docker knowledge that needs plugging to finish this off and tidy up. I'll have to come back to it in a few days now.

Remaining tasks are:

Clean up the commands for invoking manage.py and pytest calls in the CI build
Reflect that solution in the docs
Rebase to clean up the history

@billglover - I think the issue I'm hitting is that essentially docker-compose run --rm manage [x] is running as a less privileged user than docker-compose run --rm app [x]. I just can't explain why, or tbh what is the logic behind manage as a separate service from app. Are you able to offer any insight on this that might help?

chris48s · 2020-03-16T21:58:34Z

Having fiddled with it a bit more, I think this is good for review now. I've ended up removing the manage container. I did have a look at turning it into a generic shell container for running shell commands, but it ended up being an exact clone of the app container so I decided we might as well have one less service and run stuff against the app container.

I think my original comment in #109 (comment) was wrong - everything is running as root. Simultaneously, I feel like I've worked around an underlying issue here which stems from the containers running as root and writing to the filesystem but I think in its current form this PR isn't making that any worse than when I started. I'd still like to understand it better though 🤔

BethanyG · 2020-03-21T17:41:13Z

Hi @chris48s - I am going to dig into this (and some testing) today. I don't think I feel comfortable removing the manage container without more eyes on this, so I've requested that @billglover take a look as well.

lpatmo

A belated HUGE 🙏thanks for this, @chris48s -- sorry it took me a while to look at this!

I checked out your branch and tested all the updated commands. docker-compose run --rm app pytest looks 😍! I also added a few questions/comments since I am approaching this from a newb perspective. I'm going to defer to @billglover and @BethanyG for final approval, since they've been more hands-on in Django and docker than I have.

README.md

lpatmo · 2020-03-22T07:53:35Z

project/config/settings/base.py

@@ -257,7 +257,10 @@
            "level": "DEBUG",
            "class": "logging.StreamHandler",
            "formatter": "verbose",
-        }
+        },
+        "null": {


Can you explain what this does? (Sorry if it is obvious and I missed it)

In general, f6eddc6 is declares a logger that throws away log messages and then says "use this logger for everything under test"

I originally put that there when I was running tests with the django runner to capture log messages under test because they were all getting printed to stdout, but didn't clock that once I switched them over to running with pytest, pytest does this for us in a more configurable way. I've backed this change out in cb766dd. We should rely on pytest's capturing: https://docs.pytest.org/en/latest/capture.html
pytest will capture the logs by default and pytest --capture=no (or pytest -s) won't, which is more useful anyway.

I've done cb766dd as a revert commit to make review easier, but I can tidy up history before merge

lpatmo · 2020-03-22T07:57:04Z

project/users/models.py

@@ -10,6 +10,5 @@ class User(AbstractUser):
    # around the globe.
    name = CharField(_("Name of User"), blank=True, max_length=255)

-    @property


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

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

backend/project/core/contrib/sites/migrations/0003_set_site_domain_and_name.py

Line 16 in 2683d9a

"domain": "django.codebuddies.org",

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.

Thanks for the explanation here.

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.

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

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

BethanyG

Interestingly, I was unable to make a call to fixtures work with my conda setup for these tests - hence the call_command. However, the fixture trick works in docker...go figure. Much nicer as a list of fixtures.

Still not really wanting to give up the manage.py container here, but figure we could add it back in if needed, along with a test container, if we are feeling fancy/ are presented with a strong argument for having them separate.

I think we can approve and merge this, and if we later change our minds, it won't be a terrible thing.

BethanyG · 2020-03-25T07:28:49Z

@billglover - any thoughts or concerns to add?

billglover · 2020-03-25T11:29:53Z

No concerns with removing the manage container. The tests all seem to pass and so I'm happy with this.

Not much to add regarding pytest or the changes to configuration and application tests as I'm still a long way behind on Python / Django familiarity.

Django's ORM internally expects this to be a method, not a property https://docs.djangoproject.com/en/3.0/ref/models/instances/#get-absolute-url Making it a property causes django to throw `TypeError: 'str' object is not callable` when creating a link to a user object

- run pytest in CI - scrap the standalone manage container - run console commands against app container - update docs

chris48s · 2020-03-25T19:50:53Z

Cheers for the reviews all.

@billglover - do you have any thoughts on the issue of files created inside the container being owned by root? Should we be chown-ing everything from outside the container, should the containers be running as not-root? Bit of both?

billglover · 2020-03-25T20:10:34Z

I’m not too fussed at the moment, but would like to revisit how the containers are built and see if we can reduce the number of things that need to be installed. Perhaps this is would be a good time to look at file ownership.

chris48s added 3 commits March 12, 2020 19:29

move conftest

7fafb35

test config

360fd74

cleanup tests

6269bf0

load fixtures declaratively we don't need to call_command('loaddata') by hand remove unused imports

chris48s changed the title ~~Run the entire test suite using pytest~~ WIP: Run the entire test suite using pytest Mar 12, 2020

chris48s commented Mar 12, 2020

View reviewed changes

chris48s force-pushed the issue107 branch from bb552d6 to 60012fd Compare March 12, 2020 22:28

chris48s force-pushed the issue107 branch 2 times, most recently from 4806ba7 to 675dc91 Compare March 16, 2020 21:48

chris48s changed the title ~~WIP: Run the entire test suite using pytest~~ Run the entire test suite using pytest Mar 16, 2020

BethanyG requested review from billglover and BethanyG March 21, 2020 17:36

lpatmo reviewed Mar 22, 2020

View reviewed changes

This was referenced Mar 22, 2020

Issue 111 - make email required on backend chris48s/backend#1

Closed

[User Registration] Make email required on backend #111

Closed

BethanyG approved these changes Mar 25, 2020

View reviewed changes

billglover approved these changes Mar 25, 2020

View reviewed changes

chris48s added 2 commits March 25, 2020 19:42

fix user model

408ea66

Django's ORM internally expects this to be a method, not a property https://docs.djangoproject.com/en/3.0/ref/models/instances/#get-absolute-url Making it a property causes django to throw `TypeError: 'str' object is not callable` when creating a link to a user object

run pytest in CI, remove standalone manage container

699fec3

- run pytest in CI - scrap the standalone manage container - run console commands against app container - update docs

chris48s force-pushed the issue107 branch from c6c9687 to 699fec3 Compare March 25, 2020 19:43

chris48s merged commit 5fca719 into codebuddies:master Mar 25, 2020

chris48s mentioned this pull request Apr 8, 2020

use MD5 password hasher under test #130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run the entire test suite using pytest #109

Run the entire test suite using pytest #109

chris48s commented Mar 12, 2020

chris48s Mar 12, 2020

BethanyG Mar 25, 2020

chris48s commented Mar 12, 2020

chris48s commented Mar 16, 2020

BethanyG commented Mar 21, 2020

lpatmo left a comment

lpatmo Mar 22, 2020

chris48s Mar 22, 2020 •

edited

Loading

lpatmo Mar 22, 2020

chris48s Mar 22, 2020

lpatmo Mar 23, 2020

BethanyG Mar 25, 2020

BethanyG Mar 25, 2020

chris48s Mar 25, 2020

BethanyG left a comment

BethanyG commented Mar 25, 2020

billglover commented Mar 25, 2020

chris48s commented Mar 25, 2020 •

edited

Loading

billglover commented Mar 25, 2020

Run the entire test suite using pytest #109

Run the entire test suite using pytest #109

Conversation

chris48s commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris48s commented Mar 12, 2020

chris48s commented Mar 16, 2020

BethanyG commented Mar 21, 2020

lpatmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris48s Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BethanyG left a comment

Choose a reason for hiding this comment

BethanyG commented Mar 25, 2020

billglover commented Mar 25, 2020

chris48s commented Mar 25, 2020 • edited Loading

billglover commented Mar 25, 2020

chris48s Mar 22, 2020 •

edited

Loading

chris48s commented Mar 25, 2020 •

edited

Loading