Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: handle TokenTooLong error scenario #1559

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
21 changes: 16 additions & 5 deletions assert/assertions.go
Expand Up @@ -301,13 +301,24 @@ func messageFromMsgAndArgs(msgAndArgs ...interface{}) string {
func indentMessageLines(message string, longestLabelLen int) string {
outBuf := new(bytes.Buffer)

for i, scanner := 0, bufio.NewScanner(strings.NewReader(message)); scanner.Scan(); i++ {
// no need to align first line because it starts at the correct location (after the label)
if i != 0 {
// append alignLen+1 spaces to align with "{{longestLabel}}:" before adding tab
msgScanner := bufio.NewScanner(strings.NewReader(message))

// a buffer is set manually to store the tokenization of the message string
// so that we can re-use the same buffer for each line of the message without
// any additional allocations. We set the maximum buffer length to 1 more
// 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


first := true
for msgScanner.Scan() {
if !first {
outBuf.WriteString("\n\t" + strings.Repeat(" ", longestLabelLen+1) + "\t")
arjunmahishi marked this conversation as resolved.
Show resolved Hide resolved
}
outBuf.WriteString(scanner.Text())

outBuf.Write(msgScanner.Bytes())
first = false
}

return outBuf.String()
Expand Down
77 changes: 77 additions & 0 deletions assert/assertions_priv_test.go
@@ -0,0 +1,77 @@
package assert

import (
"bufio"
"strings"
"testing"
)

func Test_indentMessageLines(t *testing.T) {
tt := []struct {
name string
msg string
longestLabelLen int
expected string
}{
{
name: "single line",
msg: "Hello World\n",
longestLabelLen: 11,
expected: "Hello World",
},
{
name: "multi line",
msg: "Hello\nWorld\n",
longestLabelLen: 11,
expected: "Hello\n\t \tWorld",
},
{
name: "single line - over the bufio default limit",
msg: strings.Repeat("hello ", bufio.MaxScanTokenSize+10),
longestLabelLen: 11,
expected: strings.Repeat("hello ", bufio.MaxScanTokenSize+10),
},
{
name: "multi line - over the bufio default limit",
msg: strings.Repeat("hello\n", bufio.MaxScanTokenSize+10),
longestLabelLen: 3,
expected: strings.TrimSpace(
strings.TrimPrefix(strings.Repeat("\thello\n\t ", bufio.MaxScanTokenSize+10), "\t"),
),
},
{
name: "single line - just under the bufio default limit",
msg: strings.Repeat("hello ", bufio.MaxScanTokenSize-10),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

longestLabelLen: 11,
expected: strings.Repeat("hello ", bufio.MaxScanTokenSize-10),
},
{
name: "multi line - just under the bufio default limit",
msg: strings.Repeat("hello\n", bufio.MaxScanTokenSize-10),
longestLabelLen: 3,
expected: strings.TrimSpace(
strings.TrimPrefix(strings.Repeat("\thello\n\t ", bufio.MaxScanTokenSize-10), "\t"),
),
},
{
name: "single line - equal to the bufio default limit",
msg: strings.Repeat("hello ", bufio.MaxScanTokenSize),
longestLabelLen: 11,
expected: strings.Repeat("hello ", bufio.MaxScanTokenSize),
},
{
name: "multi line - equal to the bufio default limit",
msg: strings.Repeat("hello\n", bufio.MaxScanTokenSize),
longestLabelLen: 3,
expected: strings.TrimSpace(
strings.TrimPrefix(strings.Repeat("\thello\n\t ", bufio.MaxScanTokenSize), "\t"),
),
},
}

for _, tc := range tt {
t.Run(tc.name, func(t *testing.T) {
Equal(t, tc.expected, indentMessageLines(tc.msg, tc.longestLabelLen))
})
}
}