search

hackoregon / civic-devops

Master collection point for issues, procedures, and code to manage the HackOregon Civic platform
MIT License
11 stars 4 forks source link

Remove some or all of the example test cases from API projects #169

Closed MikeTheCanuck closed 5 years ago

MikeTheCanuck commented 6 years ago

Disaster Resilience ran into a problem during Travis build that was traced to one of the example test cases that apparently requires write access to "reset the database": https://github.com/hackoregon/disaster-resilience-backend/pull/22

We should eliminate this test case from all the backend projects (and if they have at least one working, non-example test case, we can remove all the examples - which were only introduced to support the pytest testing approach, which will fail in Travis builds if there are no tests available to run):

MikeTheCanuck commented 6 years ago

Results of Analysis

nam20485 commented 6 years ago

@MikeTheCanuck Please see https://github.com/hackoregon/disaster-resilience-backend/pull/22#issuecomment-396411774. The offending TransactionTestCase in disaster-resilience-backend appearing to need write access to the DB is a red herring, it does not need write access and completes successfully without write access. The problem being reported during the unit tests run is only because a model was changed and merged in without new migrations being added after. If the Django migrate command is run and resulting migration files are checked in when the model updates are added, the test case functions correctly with only read-only DB access. See hackoregon/disaster-resilience-backend#20, comment https://github.com/hackoregon/disaster-resilience-backend/pull/20#issuecomment-396410771 for full explanation.

MikeTheCanuck commented 6 years ago

@nam20485 does this mean that you intend to keep the dummy tests in the repo? Or are you narrowly responding to the confusion over whether there's a real bug there? I don't see the value in the dummy tests - they aren't accomplishing anything meaningful, and aren't even necessary once there's at least one functional test that pytest can execute successfully.

nam20485 commented 6 years ago

I was narrowly responding to your belief that the dummy test case requires write access and thereby caused an error or bug.

The error was real but was caused by not creating migrations after modifying a model. This is resolved now (and the dummy test cases do not require write access).

As for leaving in the test cases, they do have nominal value in that they catch, as in this case, when the service would fail to start. However I do agree they need to be replaced by real test cases and then removed.

nam20485 commented 6 years ago

@MikeTheCanuck FYI- I removed the dummy example tests and replaced them with specific endpoint response unit test cases. See https://github.com/hackoregon/disaster-resilience-backend/blob/development/api/tests/tests.py.

It tests that each API endpoint responds with an HTTP 200 response. I plan on adding model and serialization unit test cases, but I think this is a good start to at least ensure the image that is about to be deployed responds correctly. Might be a good example to deploy the rest of the backends.

@znmeb @BrianHGrant