Closed thesujai closed 1 month ago
The only major refactor it does is in the app.py, where I moved all the blueprint import and their registration to create_app
. This doesn't have any regression with my testing.
The only reason for doing this is getting the app
object, which if I imported without doing the refactoring would have resulted in the test sharing the same database as the main app(Which ig should be separated)
This is only for the initial review, if my approach for writing test is okay to you then only I will move forward with more tests.
@thesujai Thank you for the proposal. :)
I have a few concerns. We use Postgres as our primary database and I am not sure if sqlite provides all the functionalities we use in our database layer. SqlAlchemy might provide an abstraction layer over this, but we can't be sure if all the functions behave the same in sqlite and postgres, especially advanced datatypes like json and arrays, which we use quite heavily throughout our codebase. It would make the testing and prod environment behave differently which would be much harder to catch.
You can look into testcontainers for spawning postgres containers wherever needed. It is a very convenient library and allows to mimic your production environment.
Speaking of mimicing production environment, I think we can take this opportunity to introduce integration tests at either the API layer or at the Service layer. We can have a ton of unit tests as they are easier to write with mocked behaviour, or we can have a fewer "integration tests" to catch any bad assumptions unit tests were making. For example, the test of resolved incidents api in this pr, doesn't really test for much, except how the response is being adapted with the assumption the underlying business logic is correct. And this would be a fair assumption if the underlying logic/modules were well tested for how they behave with the database. But unfortunately, that's not the case.
PS: When I mention "integration tests" i mean tests interacting with a real database instance, instead of mocking out that behaviour. Not sure what the exact term for that is.
@jayantbh @samad-yar-khan Would like your input in this as well.
I would love to add some functional testing as well where the test DB is actually(I wrote tests like those for a different OSS, but that was easier for me as I was very familiar with the Database Design there).
I will try to use less mocking here
(I don't like it either!) (I didn't put much effort into writing this test, as it was for initial review) so I will be setting up mock data into the table before running tests, but complex e2e tests involving setting up 3-4 tables before hitting an API can be dealt with as a follow-up issue
, where we will have the base for API tests configured already? This will make writing integration tests much easier imo.
You can look into testcontainers for spawning postgres containers wherever needed. It is a very convenient library and allows to mimic your production environment.
I will check this out
As discussed in slack, not a requirement right now, might be in future!
Linked Issue(s)
Related to #506
Acceptance Criteria fulfillment
Proposed changes (including videos or screenshots)
create_app
which will return the app object. Done this so I can use the same flask app object in the test_client also(reducing the need to duplicate the code). This will not affect the current working of the app in anyways.get_resolved_incidents
Further comments