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.WriteString(msgScanner.Text())
arjunmahishi marked this conversation as resolved.
Show resolved Hide resolved
first = false
}

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

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

const (
// set maxScanTokenSize to 1 more than the default set by bufio. This will
// cover the case where a single line is longer than the default
// maxScanTokenSize
maxScanTokenSize = bufio.MaxScanTokenSize + 1
arjunmahishi marked this conversation as resolved.
Show resolved Hide resolved
)

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 - extremely long",
msg: strings.Repeat("hello ", maxScanTokenSize),
longestLabelLen: 11,
expected: strings.Repeat("hello ", maxScanTokenSize),
},
{
name: "multi line - extremely long",
msg: strings.Repeat("hello\n", maxScanTokenSize),
longestLabelLen: 3,
expected: strings.TrimSpace(
strings.TrimPrefix(strings.Repeat("\thello\n\t ", maxScanTokenSize), "\t"),
),
},
}

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