Api Logger Middleware - Unit tests #441
Conversation
| , sensitive :: Int | ||
| } deriving Generic | ||
|
|
||
| instance ToJSON MkJson where |
There was a problem hiding this comment.
I wonder if we have MkJson deriving Generic and we have
{-# LANGUAGE DeriveGeneric #-}
import GHC.Generics
aren't below instances equal to
instance ToJSON MkJson
instance FromJSON MkJson
???
There was a problem hiding this comment.
Indeed ^.^... I think I just like being explicit. Force of the habit.
| , whatever :: Int | ||
| } deriving Generic | ||
|
|
||
| instance ToJSON ResponseJson where |
There was a problem hiding this comment.
the same remark/question as above
| , (Error, "500 Internal Server Error") | ||
| ] | ||
|
|
||
| it "different request ids" $ \ctx -> do |
There was a problem hiding this comment.
we do not lose any request's log. but do we also have time order maintained in this setup? Maybe we do not have this kind of guarantee here? If we have, maybe it is worth showing it....
There was a problem hiding this comment.
Hmmm. Good point. There's actually no tests about the timing of request at all. It could be worth adding a handcrafted "long" endpoint (like, over a second or so) and check that the computed time is indeed over a certain amount of time.
For the concurrent test... 🤔.. Perhaps it'll be sufficient to also make a single request to that endpoint, and expect to have only one set of log lines with a time above 1s?
What do you think?
There was a problem hiding this comment.
(I mean, single request to the long endpoint in addition to the 100 requests to the other faster one)
There was a problem hiding this comment.
exactly, how the logging will behave when timeout, also crossed my mind!
There was a problem hiding this comment.
Hmmm.. Timeout is only a client concern here. The server does not timeout so to speak, only the client closes the connection.
I am not actually 100% sure how servant / warp behaves in that case. I suppose they leave the handler until it completes and throwd when trying to write the response back to the client.. In any case, that's kinda out of scope for the middleware here which has no control over this.
There was a problem hiding this comment.
I was thinking that maybe we can add one property to those unit tests. Here we can add long endpoint (this would strengthen the case) and we would generate 100 different endpoints queries. And in the property we would realize that combination of request twice and compare results of these two 100-request runs. With fast endpoints, then with long endpoints, or even better mix of short and long...Maybe it is overkill - but still it would give us more information what to expect from our logging engine just looking at the test suite.
There was a problem hiding this comment.
🤔 And what would we expect this property to be / demonstrate?
There was a problem hiding this comment.
that when we run the set of 100 different queries the logging is deterministic in terms of logging time order when something is satisfied?(for example, when the response time order from mock server is the same in those two 100-request runs). Some logging engines does not guarantee this for sure because they have buffers working strangely...Just speculating ....
There was a problem hiding this comment.
Actually, having something deterministic won't be that easy because we don't know what servant and the GHC runtimes does with the requests. Eventually, Servant creates one thread per request handler, but they get interleaved in a non deterministic order so I wouldn't rely too much on that. So, we can't be sure that if req#1 is received before req#2, it'll log req#1 before. Especially since there's a MVar read before the log entry, it can cause the thread to hang and pass on the focus to another thread. Plus, I don't think it really matters to have logs in order for this kind of scenarios. What matters is that all the logs are indeed there.
So, best we can do is really say that, after n requests, there should be entries regarding n different requests.
Both statements are actually equivalent: ``` instance ToJSON a where toJSON = genericToJSON Aeson.defaultOptions ``` ``` instance ToJSON a ``` (resp. for FromJSON) Slightly more concise.
KtorZ
force-pushed
the
KtorZ/api-logger-unit-tests
branch
from
ed834e0
to
3e843a4
Compare
|
Issue Number
#352
Overview
Comments
Covers pretty much everything but response streams that we don't use anyway.