Add a basic health check to the router #252
Conversation
Provide a health check endpoint which behaves in the same way as the health check on the gateway. At this point in time it always returns pass because we don't have a different health status to set.
Add test for content-type and ensure it is application/json.
|
The json() method adds the content-type header for you. I've added to the test to make sure this is happening. See: cc097df |
| let mut result = HashMap::new(); | ||
| result.insert("status", "pass"); | ||
| let reply = Box::new(warp::reply::json(&result)) as Box<dyn Reply>; | ||
| Ok::<_, Rejection>(reply) |
There was a problem hiding this comment.
[nit] You can move this allocation to a static Lazy
Add snapshots for expected responses and separate tests into header check and body check.
| let res = warp::test::request() | ||
| .path("/.well-known/apollo/server-health") | ||
| .reply(&filter) | ||
| .await; | ||
|
|
||
| let hdrs = res.headers(); | ||
|
|
||
| assert_eq!(res.status(), 200); | ||
| assert_snapshot!(hdrs["content-type"].to_str().unwrap()); |
There was a problem hiding this comment.
[nit] Try using assert_debug_snapshot on the entire res object. Much easier!!
There was a problem hiding this comment.
I only want to verify the content-type header is in place. If there is a different (new) header in the response, I don't want my test to start to fail.
There was a problem hiding this comment.
It's using warp::test::request() so I suspect it won't have too many changing headers. I have not tested though.
Even if something change in the future that make the response look different, the test will fail and display the difference and someone will just valid the new snapshot. No biggy.
| } | ||
|
|
||
| #[test(tokio::test)] | ||
| async fn it_provides_health_status_header() -> Result<(), FederatedServerError> { |
There was a problem hiding this comment.
[nit] Never return errors in test! That's actually a common mistake.
Test in rust are processes that either succeed with an exit status 0 or fails with an exit status non-zero. Therefore all the standard assertion macros are based on panicking: assert_eq!() panics when the test fails.
Pancking in test == test failing, this is fine and it is even better to panic because you can get a stacktrace of where it
went wrong.
So my rule of thumb is:
- use unwrap() a lot in test
- never use
?and return a result (otherwise you lose the stacktrace) - use expect() if it the test doesnt give a useful information to the developer (in custom macros for example)
There was a problem hiding this comment.
I agree with this. I don't return errors from tests usually. I copied this from the existing tests and assumed it was the preferred approach. Your rules of thumb are the same as mine! :)
| assert_eq!(res.status(), 200); | ||
| assert_json_snapshot!(String::from_utf8_lossy(res.body())); |
There was a problem hiding this comment.
[nit] Same here. You can probably assert the entire response using assert_debug_snapshot
|
I opened #260 to track the addition the documentation. |
Provide a health check endpoint which behaves in the same way as the
health check on the gateway.
https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-express/src/ApolloServer.ts#L85
At this point in time it always returns pass because we don't have a
different health status to set.
resolves: #54