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/require.Len doesn't print anything if the slice is too long #1525

Open
fgimian opened this issue Jan 23, 2024 · 7 comments · May be fixed by #1559
Open

assert/require.Len doesn't print anything if the slice is too long #1525

fgimian opened this issue Jan 23, 2024 · 7 comments · May be fixed by #1559
Labels

Comments

@fgimian
Copy link

fgimian commented Jan 23, 2024

Hey there, I encountered this when attempting to use require.Len as shown below.

package tester_test

import (
	"testing"

	"github.com/stretchr/testify/require"
)

func TestVeryLongSlice(t *testing.T) {
	var bigSlice []string
	for i := 0; i < 20000; i++ {
		bigSlice = append(bigSlice, "hello")
	}

	require.Len(t, bigSlice, 10)
}

Output of test:

=== RUN   TestVeryLongSlice
    tester_test.go:15:
                Error Trace:    C:/Users/Fots/source/tester/tester/tester_test.go:15
                Error:
                Test:           TestVeryLongSlice
--- FAIL: TestVeryLongSlice (0.00s)
FAIL
FAIL    github.com/fgimian/tester/tester        0.150s
FAIL

I haven't quite figured out why this happens, although I can imagine the output would be impractically long anyway. Worst of all, the main assertion message is not displayed which is quite confusing (especially to someone new using the library, like me 😄).

Personally, I think that it would be best not to display the slice at all for the Len function, and instead just display:

Should have X item(s), but has Y

However, I suspect my view on that may not be what some people want. The reason I think it is unwise to print the slice in any form, is that typically when many people are testing for the length of a slice, they may be comparing to a relatively large slice of items (e.g. the output of a mocked API call or similar).

An additional point that I believe has been discussed before is the inconsistency with argument ordering for the Len function which takes the actual first and expected second.

Cheers
Fotis

@fgimian
Copy link
Author

fgimian commented Jan 23, 2024

The easiest workaround for now seems to be:

package tester_test

import (
	"testing"

	"github.com/stretchr/testify/require"
)

func TestVeryLongSlice(t *testing.T) {
	var bigSlice []string
	for i := 0; i < 20000; i++ {
		bigSlice = append(bigSlice, "hello")
	}

	expected := 10
	require.Len(
		t,
		bigSlice,
		expected,
		"Should have %d item(s), but has %d",
		expected,
		len(bigSlice),
	)
}

@brackendawson
Copy link
Collaborator

brackendawson commented Feb 21, 2024

We use a scanner to get the text which should go there line by line (it can be multiline) to indent each line until after the field name:

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
outBuf.WriteString("\n\t" + strings.Repeat(" ", longestLabelLen+1) + "\t")
}
outBuf.WriteString(scanner.Text())
}
return outBuf.String()

This mishandles the scanner.Scan() call. If scanner.Scan() returns false then you need to check scanner.Err() in case it isn't io.EOF. If we did that we'd have found it is bufio.Scanner: token too long.

We need to think of a thing to do about very long lines in this code. We should also probably not produce such a long line from assert.Len, maybe we should truncate after a sane number of elements?

@fgimian
Copy link
Author

fgimian commented Feb 21, 2024

We use a scanner to get the text which should go there line by line (it can be multiline) to indent each line until after the field name:

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
outBuf.WriteString("\n\t" + strings.Repeat(" ", longestLabelLen+1) + "\t")
}
outBuf.WriteString(scanner.Text())
}
return outBuf.String()

This mishandles the scanner.Scan() call. If scanner.Scan() returns false then you need to check scanner.Err() in case it isn't io.EOF. If we did that we'd have found it is bufio.Scanner: token too long.

We need to think of a thing to do about very long lines in this code. We should also probably not produce such a long line from assert.Len, maybe we should truncate after a sane number of elements?

That sounds good to me. Personally I'd even be happy for the contents of the slice not to be printed at all, but I suppose this output can be useful for shorter slices when troubleshooting why a test is failing.

Cheers
Fotis

@hendrywiranto
Copy link
Contributor

hendrywiranto commented Feb 22, 2024

Hello
For the argument ordering, you can enable https://github.com/Antonboom/testifylint via golangci

@fgimian
Copy link
Author

fgimian commented Feb 22, 2024

Hello For the argument ordering, you can enable https://github.com/Antonboom/testifylint via golangci

Thanks, yep I'm using this already, but I think my point was more around the inconsistency of the API and how that particular function feels odd in the context of the rest.

Of course I appreciate that this would be a breaking change but just thought I'd bring it up for consideration in case a major new version of testify was being planned.

Cheers
Fotis

@brackendawson
Copy link
Collaborator

The argument order is covered by #146. Let's keep this issue about the missing message.

arjunmahishi added a commit to arjunmahishi/testify that referenced this issue Mar 3, 2024
… 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 arjunmahishi linked a pull request Mar 3, 2024 that will close this issue
@arjunmahishi
Copy link
Collaborator

This mishandles the scanner.Scan() call. If scanner.Scan() returns false then you need to check scanner.Err() in case it isn't io.EOF. If we did that we'd have found it is bufio.Scanner: token too long.

I've raised a PR that will make sure that the TokenTooLong never happens.

We should also probably not produce such a long line from assert.Len, maybe we should truncate after a sane number of elements?

Agreed. But regardless of the call we take about displaying long messages, I think this fix will be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants