testing: Helper's highest function call's information should be maintained for any subsequent/nested calls

[mattcary]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.9.2 android/arm64

Does this issue reproduce with the latest release?

Yes (I think 1.9.2 is the latest release)

What operating system and processor architecture are you using (go env)?

GOARCH="arm64"
GOHOSTARCH="arm64"
GOHOSTOS="android"
GOOS="android"

What did you do?

Run the following program with "go test". I would have hoped that the reported error line is in the 2nd call to checkFor3s() in TestHelper(). Instead, the error line is in forEach() (line 9 for me but I think some blank lines may have been eaten below.

I think the problem is that checkFor3s() uses the method forEach which is not marked with t.Helper(). In a more realistic case, forEach would be a non-testing utility method and so would not have the possibility of calling t.Helper().

Perhaps the semantics of t.Helper should be that the displayed error line is in the stack frame above the highest t.Helper() call? That way this example would be fixed, and also it would not be necessary to sprinkle t.Helper() calls everywhere if there are complex testing utility functions.

package helper                                                                                                                 

import(
        "testing"
)                                                                                                                               
func forEach(data []int, f func(int)) {
        for _, i := range data {
                f(i)
        }
}

func checkFor3s(tb testing.TB, data []int) {
        tb.Helper()
        forEach(data, func(i int) {
                tb.Helper()
                if i != 3 {
                        tb.Error("Saw", i, "instead of 3")
                }
        })
}

func TestHelper(t *testing.T) {
        checkFor3s(t, []int{3, 3, 3, 3})
        checkFor3s(t, []int{3, 3, 2, 3})
        checkFor3s(t, []int{3, 3, 3, 3})
}

[odeke-em]

Hello there @mattcary, thanks for reporting this. So if I understood your issue right, it is that the line number reporting is inaccurate right? That on Go1.9.2 you are getting the error reported on line 9

func forEach(data []int, f func(int)) {
        for _, i := range data {
                f(i)
        } // <--- right here

Please try with Go1.10 beta, the line numbers now seem to report on line 8

func forEach(data []int, f func(int)) {
        for _, i := range data {
                f(i) // <--- right here
        }

$ go test -v
=== RUN   TestHelper
--- FAIL: TestHelper (0.00s)
	tf_test.go:8: Saw 2 instead of 3
FAIL
exit status 1
FAIL	_/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/23249	0.006s

[mattcary]

Thanks for your reply Emmanuel.

Just downloaded the beta, it reports the error at the call to f(i) identically in 1.9.2 as in 1.10beta1.

However, the issue is that the line number is in the wrong function altogether: it should report an error in TestHelper(), not in forEach() at all. Putting a call to tb.Helper in forEach() makes the line number display correctly:

func forEach(tb testing.TB, data []int, f func(int)) {
tb.Helper()
for _, i := range data {
f(i)
}
}

But this requires plumbing tb through, which would not be possible if forEach were a non-testing utility method.

Happy New Year! :)

[odeke-em]

Thank you @mattcary for the clarification, and happy new year to you too!

Oh I see, so if I may paraphrase, your issue is that you'd like *testing.B.Helper() and *testing.T.Helper() to propagate/nest the behavior(using the parent function's file and line information), to any callers that are any call-level after the original marking?

I'll also page @cespare and @ianlancetaylor.

[mattcary]

Yes, something like that. I was thinking that the highest T.Helper() call in the stack should have as parent the testing line we want to display as an error.

Thx

[odeke-em]

Awesome, thank you for the clarification @mattcary, I'll re-amend the title for even more precision, please feel free to edit it.

[cespare]

With the suggested change, in order to decide whether or not to mark a method as a helper, you need to know whether there is a helper higher up in the call chain. In the given code, for instance, forEach will only be ignored as a helper if it is called from a helper; if you call forEach directly from TestHelper, then you'll have the same issue where the printed line number will be inside forEach.

With t.Helper's existing behavior, the rule is simple: if you want a function to be ignored when go test prints file:line, mark it as a helper. You can make this choice function-by-function without ever considering the full set of call paths that reach the function.

Additionally, I think it's too late to change this behavior because t.Helper is in a released version of Go.

[mattcary]

Happy New Year! On Dec 27, 2017 11:49 PM, "Caleb Spare" <notifications@github.com> wrote: With the suggested change, in order to decide whether or not to mark a method as a helper, you need to know whether there is a helper higher up in the call chain. In the given code, for instance, forEach will only be ignored as a helper if it is called from a helper; if you call forEach directly from TestHelper, then you'll have the same issue where the printed line number will be inside forEach. One could never print a line number from forEach, it doesn't have access to testing.T (in my real case it's from a different package entirely). For example, I think the same problem would occur if one used filepath.Walk in a test helper. With t.Helper's existing behavior, the rule is simple: if you want a function to be ignored when go test prints file:line, mark it as a helper. You can make this choice function-by-function without ever considering the full set of call paths that reach the function. But it's not possible to mark forEach as a helper. Anytime a helper is called from a non-test function, testing.T.Helper cannot be used. This restricts non-test code from being used in tests. Additionally, I think it's too late to change this behavior because t.Helper is in a released version of Go. I agree that semantic changes are tricky. However, this feels like a bug to me. Is there a legitimate use for this behavior? I briefly looked through the issue and proposal where t.Helper was discussed (#4899), and while on the bug 1 Nov 2016 robpike@ raised the concern this case hits, it was never really discussed. At any rate, there are multiple ways to solve this issue with various drawbacks. A new method could be added, for example, although that's messy for the API in general. More minimal changes seem to have already been discussed in proposal/4899 and found unsatisfactory. Maybe there's another way to fix this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23249 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AhIggHeU041toVdcKUTJoCeO4PrjUl-iks5tEslvgaJpZM4RNCHN> .

[mattcorbyeaglen]

I too am having this issue and it would be very useful for Helper() to do this.

[karlkfi]

An alternate way to work around this issue would be to do what Ginkgo does and allow Helper to specify how many stack frames to skip. This allows a helper function that knows it's going to be called by a non-helper function to specify that it wants to skip more frames. This would also allow *testing.B.Helper() and *testing.T.Helper() to be wrapped with abstractions without breaking Helper.

https://github.com/onsi/ginkgo/blob/master/types/code_location.go#L98

This also has the benefit that it can be added to Helper in a reverse compatible way to using a vararg.