Conversation
Yarik-Popov
changed the title
Yarik-Popov
left a comment
Yarik-Popov
left a comment
There was a problem hiding this comment.
The logic is overall correct. Some minor issues with the code that should be easy to fix.
Also, instead of hardcoding the status codes that in the future we will forget what the meaning of them are. You should import the HTTP status codes fastapi.status
Yarik-Popov
left a comment
Yarik-Popov
left a comment
There was a problem hiding this comment.
I added one suggestion to try to fix the previous issue that existed.
Also, can you write some unit tests for this. See the python_test directory for examples on existing tests. You can find some hints on how to test APIs in the gs-onboarding repo
|
The only thing that is left are the tests. Once those are finished and they are good, you should be done. Great job so far. |
Yarik-Popov
left a comment
Yarik-Popov
left a comment
There was a problem hiding this comment.
Overall the tests are good. There are some small improvements to them that are needed.
| raise HTTPException( | ||
| status_code=HTTP_400_BAD_REQUEST, | ||
| detail=exc.message, | ||
| ) |
There was a problem hiding this comment.
Wait is this one the only one is a HTTPException while all the other ones are JSONResponse?
| # Test BaseOrbitalError | ||
| response = fastapi_test_client.get("/test_base_orbital_exceptions") | ||
| assert response.status_code == HTTP_400_BAD_REQUEST | ||
| assert response.json().get("message") == "Test BaseOrbitalError" | ||
|
|
||
| # Test DatabaseError | ||
| response = fastapi_test_client.get("/test_database_exceptions") | ||
| assert response.status_code == HTTP_400_BAD_REQUEST | ||
| assert response.json().get("message") == "Test DatabaseError" | ||
|
|
||
| # Test InvalidArgumentError | ||
| response = fastapi_test_client.get("/test_invalid_argument_exceptions") | ||
| assert response.status_code == HTTP_422_UNPROCESSABLE_ENTITY | ||
| assert response.json().get("message") == "Test InvalidArgumentError" | ||
|
|
||
| # Test InvalidStateError | ||
| response = fastapi_test_client.get("/test_invalid_state_exceptions") | ||
| assert response.status_code == HTTP_409_CONFLICT | ||
| assert response.json().get("message") == "Test InvalidStateError" | ||
|
|
||
| # Test NotFoundError | ||
| response = fastapi_test_client.get("/test_not_found_exceptions") | ||
| assert response.status_code == HTTP_404_NOT_FOUND | ||
| assert response.json().get("message") == "Test NotFoundError" | ||
|
|
||
| # Test ServiceError | ||
| response = fastapi_test_client.get("/test_service_exceptions") | ||
| assert response.status_code == HTTP_400_BAD_REQUEST | ||
| assert response.json().get("message") == "Test ServiceError" | ||
|
|
||
| # Test SunPositionError | ||
| response = fastapi_test_client.get("/test_sun_position_exceptions") | ||
| assert response.status_code == HTTP_400_BAD_REQUEST | ||
| assert response.json().get("message") == "Test SunPositionError" | ||
|
|
||
| # Test UnauthorizedError | ||
| response = fastapi_test_client.get("/test_unauthorized_exceptions") | ||
| assert response.status_code == HTTP_401_UNAUTHORIZED | ||
| assert response.json().get("message") == "Test UnauthorizedError" | ||
|
|
||
| # Test UnknownError | ||
| response = fastapi_test_client.get("/test_unknown_exceptions") | ||
| assert response.status_code == HTTP_400_BAD_REQUEST | ||
| assert response.json().get("detail") == "Test UnknownError" |
There was a problem hiding this comment.
Generally its best not to have lots of different asserts in the same asserts in the same test. If one of the tests fail at the beginning, the tests after will not run and you will lose important contex. Have a look at the test_ephemeris.py for examples on how to use pytest.mark.parameterize
| app = FastAPI() | ||
| setup_exception_handlers(app) | ||
|
|
||
| @app.get("/test_base_orbital_exceptions") | ||
| async def test_custom_exceptions(): | ||
| raise BaseOrbitalError("Test BaseOrbitalError") | ||
|
|
||
| @app.get("/test_database_exceptions") | ||
| async def test_database_exceptions(): | ||
| raise DatabaseError("Test DatabaseError") | ||
|
|
||
| @app.get("/test_invalid_argument_exceptions") | ||
| async def test_invalid_argument_exceptions(): | ||
| raise InvalidArgumentError("Test InvalidArgumentError") | ||
|
|
||
| @app.get("/test_invalid_state_exceptions") | ||
| async def test_invalid_state_exceptions(): | ||
| raise InvalidStateError("Test InvalidStateError") | ||
|
|
||
| @app.get("/test_not_found_exceptions") | ||
| async def test_not_found_exceptions(): | ||
| raise NotFoundError("Test NotFoundError") | ||
|
|
||
| @app.get("/test_service_exceptions") | ||
| async def test_service_exceptions(): | ||
| raise ServiceError("Test ServiceError") | ||
|
|
||
| @app.get("/test_sun_position_exceptions") | ||
| async def test_sun_position_exceptions(): | ||
| raise SunPositionError("Test SunPositionError") | ||
|
|
||
| @app.get("/test_unauthorized_exceptions") | ||
| async def test_unauthorized_exceptions(): | ||
| raise UnauthorizedError("Test UnauthorizedError") | ||
|
|
||
| @app.get("/test_unknown_exceptions") | ||
| async def test_unknown_exceptions(): | ||
| raise UnknownError("Test UnknownError") | ||
|
|
||
| fastapi_test_client = TestClient(app) |
There was a problem hiding this comment.
Can you extract this out to a function that creates the needed exception handler test client? Something such as create_exception_handler_client just above it would be great
2 participants
Purpose
Closes #371
New Changes
Added exception handlers for both custom and common exceptions.
Testing
Added unit testing for all the exception handling.
Outstanding Changes
N/A