python-discord / api

A FastAPI-based service that allows our services to communicatie with our database.
MIT License
10 stars 1 forks source link

Migrate reminders endpoint from the site API, introduce model improvements #25

Closed D0rs4n closed 2 years ago

D0rs4n commented 2 years ago
D0rs4n commented 2 years ago

Using a mocked test database in the tests is a very huge stepback from the existing Django API and renders all of the added tests far less effective. Please re-add the test database and integrate it with our CI properly such that we can validate that the endpoints works with the database as wanted. @jchristgit

Alright, I was gonna do this initially however @HassanAbouelela mentioned that it would make the possibility of implementing features such as parallelization impossible, and that I shouldn't use a test database. I think it's worth a discussion.

HassanAbouelela commented 2 years ago

@jchristgit My reasoning for not adding the test database is that we don't really need it for our tests. I've looked over a few of the current site tests, and for the api ones, it's mostly just "Put data in DB, pull data from DB, call functions with that data." If you notice there, that whole putting into DB, and taking out is completely redundant. In both cases you're creating a predetermined instance of a model, and injecting it into a route. We don't need a DB to do that.

On the other hand, having a DB does make it more difficult to do certain things (such as parallelization as mentioned, but there are others). For instance, a test DB that is shared throughout the entire test suite makes for a more fragile suite, as your tests can have unexpected run conditions from other tests.

TL;DR: Imo, having a DB in our test suites doesn't solve much, but does make testing more complex.

jchristgit commented 2 years ago

@jchristgit My reasoning for not adding the test database is that we don't really need it for our tests. I've looked over a few of the current site tests, and for the api ones, it's mostly just "Put data in DB, pull data from DB, call functions with that data." If you notice there, that whole putting into DB, and taking out is completely redundant. In both cases you're creating a predetermined instance of a model, and injecting it into a route. We don't need a DB to do that.

I can understand your reasoning. For the majority of tests, it's true that this is redundant. However, by using a mock database, we could miss big problems such as broken inserts due to failed constraints, wrong types (or issues such as character truncation errors), foreign key problems, and more. For simple routes that are just essentially CRUD of routes, we could perhaps create some helper functions to perform the "create, test read, update, test updated, delete, test deleted" sequence of events whilst saving us from setting up mocks.

On the other hand, having a DB does make it more difficult to do certain things (such as parallelization as mentioned, but there are others). For instance, a test DB that is shared throughout the entire test suite makes for a more fragile suite, as your tests can have unexpected run conditions from other tests.

We can deal with this just fine as we are luckily using a sane database. Allowing parallel execution in our tests would basically just require:

D0rs4n commented 2 years ago

So, I see valid points on both sides. What's the consensus? I can push a draft solution how the testing using a test DB would look like. (Or, how it would not )
@HassanAbouelela @jchristgit

D0rs4n commented 2 years ago

Because of recent discussions and changes, I decided to close this pull request, and rewrite the changes from (almost) scratch.