Test buffered reader / Ensure EmptyBuffer does not get content in it #178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ensure it is not possible for downloadBody or other wrappers to accidently populate data into the emptyBuffer. This could be solved with an interface, but that interface would likely need to be quite complex as we rely on the methods on bytes.Buffer. While we could mirror Cap() Len(), etc, this seems like busy work. It is easier to broker access to br.buf.ReadFrom through br.readFrom to ensure we panic in the case of writing to a buffer that has had emptyBuffer assigned to it. This ensures we can make the assumption that .Read() will result in EOF in all scenarios when emptyBuffer has been set on the BufferedReader.
|
Ooof, bufferedReader sure is hard to test, isn't it? This PR made me feel bad enough about its interface that I have raised #180 to change it to have a better interface (purely on io abstractions with nothing http-specific) and also a better implementation (delegating more logic to bufio.Reader). If we do that, we don't have to test for some of the behaviour we're testing for here - there's no longer an emptyBuffer that we might write to, we don't have to implement checks for full buffers, bufio.Reader has a fixed-size buffer so we can't unintentionally grow it, bufio.Reader handles passing errors during fetch out on the Read() interface appropriately, etc. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces tests for bufferedReader to ensure there are no regressions, additionally, it adds a panic() case where we've attempted to use the downloadBody or a .readFrom() on the bufferedReader when emptyBuffer is the underlying bytes.Buffer.
The panic() is indicating a programming error, additionally it helps ensure the assumption that emptyBuffer will EOF is guaranteed. The only use of the br.buf.ReadFrom is from tests and/or downloadBody.