assert: handle TokenTooLong error scenario #1559
Conversation
… messages As pointed out in stretchr#1525, when the assertion message is too long, it gets completely truncated in the final output. This is because, `bufio.Scanner.Scan()` has a default `MaxScanTokenSize` set to `65536` characters (64 * 1024). The `Scan()` function return false whenever the line being scanned exceeds that max limit. This leads to the final assertion message being truncated. This commit fixes that by manually setting the internal scan buffer size to `len(message) + 1` to make sure that above scenario never occurs. Fixes stretchr#1525
arjunmahishi
changed the title
dolmen
added
enhancement
pkg-assert
Instead of using an arbitrary value like 20000, we can just use the value defined by bufio package (MaxScanTokenSize).
|
@dolmen @brackendawson Can you please take one last look at this? |
|
@dolmen Are we good to merge? |
|
I'm not yet convinced that the tests provide enough coverage. I would like to see more clearly a check related to bufio boundaries. I feel that we are testing with strings much longer than the boundary, and not just below and at the boundary. |
assert/assertions_priv_test.go
Outdated
| }, | ||
| { | ||
| name: "single line - just under the bufio default limit", | ||
| msg: strings.Repeat("hello ", bufio.MaxScanTokenSize-10), |
There was a problem hiding this comment.
The boundary is not "hello" repeated bufio.MaxScanTokenSize times, but a string which has a line whose length is bufio.MaxScanTokenSize bytes.
We probably need a utility function to build such an input string. At least strings.Repeat alone doesn't help.
There was a problem hiding this comment.
Ohh! that's right.
Changed the approach a little bit. The new commit generates the input as you suggested. And instead of matching against an exact output, it asserts the pattern of the output. Like the leading white spaces etc. Because for generating the expected output, we will end up rewriting the actual function logic in the test.
Also, fix the test cases for this function. This commit generates the input based on parameters like bytes per line and number of lines. The assertion is made against the pattern of the output rather than the exact output.
assert/assertions.go
Outdated
| // than the length of the message to avoid exceeding the default | ||
| // MaxScanTokenSize while scanning lines. This can happen when there is a | ||
| // single very long line. Refer to issue #1525 | ||
| msgScanner.Buffer(nil, len(message)+1) |
There was a problem hiding this comment.
Is this safe? As we know from the assert.Len case this can contain a very large formatted slice. Having more than doubled the memory consumed by the large slice by creating a string containing the formatted version of it. Do we want to double the formatted version again? Plus all the intermediate buffers that were allocated on the heap by msgScanner before the next GC cycle?
We could at least pre-allocate the buffer for msgScanner rather than passing in nil. But I'd prefer that we either split or truncate lines that are unreasonably long, or we could just truncate the entire message to less than bufio.MaxScanTokenSize?
There was a problem hiding this comment.
We could at least pre-allocate the buffer for msgScanner rather than passing in nil.
Agreed. I've made this change.
But I'd prefer that we either split or truncate lines that are unreasonably long, or we could just truncate the entire message to less than
bufio.MaxScanTokenSize?
If we decide to truncate, we could probably truncate based on the dimensions of the terminal? This makes more sense from a UX point of view.
There was a problem hiding this comment.
Hey, @arjunmahishi. I really think we should truncate rather than allocate unknown amounts of memory. It turns out Equal actually already uses a function to truncate its values. Sorry to steal the issue but I've opened #1646 with how I think it could be done. What do you think?
There was a problem hiding this comment.
Cool. Better to reuse the existing functionality and keep it consistent. Closing this now.
Summary
As pointed out in #1525, when the assertion message is too long, it gets completely truncated in the final output. This is because
bufio.Scanner.Scan()has a defaultMaxScanTokenSizeset to65536characters (64 * 1024). TheScan()function returns false whenever the line being scanned exceeds that max limit. This leads to the final assertion message being truncated.Changes
This commit fixes that by manually setting the internal scan buffer size to
len(message) + 1to make sure that the above scenario never occurs.Related issues
Fixes #1525