fix: Double compression. #1714
fix: Double compression. #1714
Conversation
ldez
force-pushed
the
fix/double-gzip
branch
2 times, most recently
from
85eb873 to
1c8fd24
Compare
middlewares/compress.go
Outdated
|
|
||
| func isEncoded(headers http.Header) bool { | ||
| header := headers.Get(contentEncodingHeader) | ||
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding |
There was a problem hiding this comment.
Why do we add this link here? I suppose it's to reference what constitutes an encoding?
If so, could we add an explaining comment? Something like "According to http://..., content is encoded if ...".
middlewares/compress_test.go
Outdated
| assert.Equal(t, acceptEncodingHeader, rw.Header().Get(varyHeader)) | ||
|
|
||
| if bytes.Compare(rw.Body.Bytes(), baseBody) == 0 { | ||
| t.Errorf("got %v,\n expected a compressed body.", rw.Body.Bytes()) |
There was a problem hiding this comment.
Mixture of testing and testify doesn't seem ideal. How about
assert.True(t, bytes.Compare(rw.Body.Bytes(), baseBody) == 0)
There was a problem hiding this comment.
If want to see the diff and if I do that I just see: false is different of true.
Testify doesn't provide assert.NotEqualValues, this is why I do that.
It's just a pragmatic approach
There was a problem hiding this comment.
That's what the msgAndArgs final arguments to every testify assertion method are supposed to be used for. From the docs:
Every assertion function also takes an optional string message as the final argument, allowing custom error messages to be appended to the message the assertion method outputs.
There was a problem hiding this comment.
yes but I will see this:
Expected: True
Actual: False
Message: got XXX...
expected a compressed body.
And I don't think it's a good test output.
I prefer mix stdlib and Testify, it's a better compromise for me.
| assert.Equal(t, "", rw.Header().Get(varyHeader)) | ||
|
|
||
| assert.EqualValues(t, rw.Body.Bytes(), baseBody) | ||
| } |
There was a problem hiding this comment.
The test seems structurally identical to TestShouldCompressWhenNoContentEncodingHeader. How about combining both in a table-driven test?
There was a problem hiding this comment.
I don't really want to do that.
There was a problem hiding this comment.
because it's not my way to test:
- I follow a Kent Beck rule about triangulation: I begin to thinking about a possible factorization if I repeat 3 times the same thing (the number 3, it's my limit, not a KB limit, but it's a very classic number for that with TDD)
- the tests are more flexible and more resistant to changes of the implementation.
- each test is independent and can be "recycle" (it's a testing strategy)
- the code in the test provide some limitation to be clearly factorize. (readability)
- table-driven test are not the only way to write good tests: each test case are different and we can use a lot of technique or approach.
There was a problem hiding this comment.
Hmm sorry, I find most of these arguments too general and abstract.
The flexibility point sounds like a bet into the future that only some parts of the tests will need to be adjusted. It could just as well be that a part common to both tests will need to be changed, so separate tests could be more costly. We don't know what part might change; we do know though that we can save lines and improve readability by taking a table-driven approach now, possibly changing again in the future.
So I don't agree but I neither bother too much as it's just about two tests. :-)
|
still not work as expected |
|
I'm expericing this issue too with 1.3.3. Bug report here #1851 |
Description
Don't compress when
Content-Encodingheader is already present.Fixes #1060