search

codebuddies / backend

CodeBuddies back-end
https://codebuddies.org
GNU General Public License v3.0
20 stars 25 forks source link

Run the entire test suite using pytest #109

Closed chris48s closed 4 years ago

chris48s commented 4 years ago

Main changes in this PR:

:wrench: Configure pytest :eyes: Get it to discover and run the full test suite :heavy_check_mark: Get all the existing unit tests passing :books: Update CI build and docs

Closes #107

chris48s commented 4 years ago

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:

@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 commented 4 years ago

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 https://github.com/codebuddies/backend/pull/109#issuecomment-598461820 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 :thinking:

BethanyG commented 4 years ago

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.

BethanyG commented 4 years ago

@billglover - any thoughts or concerns to add?

billglover commented 4 years ago

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.

chris48s commented 4 years ago

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 commented 4 years ago

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.