fix: fix kafka proxy tests #14
Conversation
kafka/proxy_test.go
Outdated
| @@ -53,12 +63,12 @@ func TestRequestKeyHandler_Handle(t *testing.T) { | |||
| }{ | |||
| { | |||
| name: "Valid message", | |||
| request: []byte{0, 0, 0, 7, 0, 16, 99, 111, 110, 115, 111, 108, 101, 45, 112, 114, 111, 100, 117, 99, 101, 114, 255, 255, 0, 1}, // payload: 'valid message' | |||
| request: []byte{0, 0, 5, 220, 0, 0, 0, 1, 0, 4, 100, 101, 109, 111, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 81, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 69, 255, 255, 255, 255, 2, 42, 190, 231, 201, 0, 0, 0, 0, 0, 0, 0, 0, 1, 122, 129, 58, 51, 194, 0, 0, 1, 122, 129, 58, 51, 194, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 0, 1, 38, 0, 0, 0, 1, 26, 118, 97, 108, 105, 100, 32, 109, 101, 115, 115, 97, 103, 101, 0}, // payload: 'valid message' | |||
There was a problem hiding this comment.
Could we provide the exact request body in tests and then convert it to bytes? At the moment I don't see which message is valid/invalid.
There was a problem hiding this comment.
We could, but at this moment we do not have a proper encoder.
Encoder/Decoder code comes from Sarama library and at this stage I only copied (yes, I had to copy because their interfaces are not public) the decoder logic, which is the only thing we need in our app by now.
I would postpone the encoding logic for later once the project turns to a more mature stage. There will be several request headers and message versions to support.
There was a problem hiding this comment.
Ok, I understand, but you can extend the request body as comment with a "shape" of valid/invalid message? Then it will be easy to check if tests passed or not by changes or by bug in changes 😅
There was a problem hiding this comment.
I dont get what you mean.
There was a problem hiding this comment.
@magicmatatjahu Look at my last commit. Not sure if this is what you meant but I hope it looks more clear now.
Ideally, we would be doing this through the ProduceRequest struct, but the cons are all of the ones I mentioned in my previous message.
There was a problem hiding this comment.
Oops, have an error in the test, but fixing anyway
There was a problem hiding this comment.
Sorry, I had this situation 😅
So yeah, the comment was for describing the shape of bytes. Ok, we can stay with the last commit :)
There was a problem hiding this comment.
So please fix the lint error and I will accept.
There was a problem hiding this comment.
Nah, I ended up creating a very handmade encoder that only works for tests. Enjoy checking the bytes back and forth dance 😆 . Btw, I ended up using some "magic" bytes so I simplify the work, otherwise, I will be spending a lot of time.
There was a problem hiding this comment.
Now I feel responsible for your efforts. I only had problem with these bytes and I don't like stop you (because it's only a test), so I accept changes 🚀 Good job!
Description
Tests were failing silently as no error happens when decoding goes wrong. This is on purpose so we do not break the basic proxy functionality (clients can still communicate to the brokers).
This change fixes the payloads in the tests and changes log levels to rely by now on them, so tests will fail if decoding goes wrong.