Refactors test_routes as a separate module #266
Conversation
Fixes failing algolia test where mocker was mocking the wrong method
Digbigpig
force-pushed
the
246-test_routes_module
branch
from
a67d11d to
b19e76c
Compare
| first_term = random_string() | ||
| apikey = get_api_key(client) | ||
|
|
||
| # Create resource and find it in the search results. |
There was a problem hiding this comment.
This is kind of a funky set of tests because it's not actually using Algolia... Like, having an integration test would make more sense than a unit test for this. I think we may want to have a chat about what we're really testing here and take a second look at whether this accomplishes that in the best way. I'm honestly not sure, but I could see this being improved
There was a problem hiding this comment.
Maybe we could start a test automation project to run integration tests on the different parts of OC...
| @@ -11,7 +11,7 @@ | |||
| def unauthorized_response(): | |||
| message = "The email or password you submitted is incorrect " \ | |||
| "or your account is not allowed api access" | |||
| payload = {'errors': {"invalid-credentials": {"message": message}}} | |||
| payload = {'errors': {"unauthorized": {"message": message}}} | |||
There was a problem hiding this comment.
The only status code that returns an "invalid-*" response is a 422. For all the rest of the routes, the error is the same as the status, which in the case of 401 is "Unauthorized". I feel this API change is necessary for consistency
| @@ -120,7 +120,7 @@ def standardize_response(payload={}, status_code=200, version=LATEST_API_VERSION | |||
| elif not data: | |||
| # 500 Error case -- Something went wrong. | |||
| message = msg_map.get(500) | |||
| resp["errors"] = {'errors': {"server-error": {"message": message}}} | |||
| resp["errors"] = {"server-error": {"message": message}} | |||
There was a problem hiding this comment.
Fixing these tests to use a standardized utility function to check the responses was really good, because it caught this issue. A frontend would not have been able to parse correctly
|
|
Added helper to modify the all resources created_at and last_updated timestamps in order to test the updated_after filter
The commit is expected to fail on build.
| "message")) | ||
|
|
||
| # Set it back to development for other tests | ||
| environ["FLASK_ENV"] = "development" |
There was a problem hiding this comment.
Isn't this test the same as the previous test? Should we separate the body into a separate function and just make each test call it with different parameters? Or would that be overdoing it?
There was a problem hiding this comment.
Yes, it's the same test except the mocked exception is fake_algolia_unreachable_host instead of fake_algolia_exception (which is two different errors we want to ensure we're catching in these 4 situations).
If you want to tighten this up and combine them somehow or break the logic out into a function, feel free, just so long as it's still readable
|
I documented the remaining problems in a new issue. I'm not going to touch that issue. If someone comes along and wants to mess with them, great. If not, that issue serves more as documentation so if someone comes along they can have some background as to why these things remain as problems. I think we're good to merge this. Please open additional PRs or issues for remaining problems that were not addressed in this PR. |
This refactors the test_routes file as a module. Addresses Issue #246
Let me know what you think about the file names, whether a test should be moved, or if we should further break down files.
The test_resource_crud.py file (
holds all CRUD tests rather than GET routes) is 550 lines long but doesn't have an unreasonable amount of tests. test_resource_retrieval is just under 200 and I think that is the second biggest file now..... I just googled CRUD to realize that the R is for Read, not Remove... So suggestions for a new name are appreciated too