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

testing: t.Fail should report the line on which the failure occurred #26720

Closed
nhooyr opened this issue Jul 31, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@nhooyr
Copy link
Contributor

commented Jul 31, 2018

I had some code that looked like this:

package main

import (
	"testing"
	"time"
)

func TestFoo(t *testing.T) {
	t.Run("fail", func(t *testing.T) {
		go func() {
			defer func() {
				r := recover()
				if r != nil {
					t.Logf("panic: %+v", r)
				}
			}()

			time.Sleep(time.Second)

			t.Error("meow")
		}()
	})

	time.Sleep(time.Second*2)
}

I know the above code is buggy because its calling t.Error from a goroutine that lasts longer than the test goroutine and so t.Error will panic in t.Fail. However, the original test was complex and no one saw this coming.

The issue I was having is that the error call is never reported. t.Error panics and then this is recovered and logged but the individual test within which this happened is not marked as failed. Only the parent test is marked as failed. This made the test very hard to debug as there would be no line on which the failure occurred.

So if you run this code, you'll get:

$ go test
--- FAIL: TestFoo (0.00s)
FAIL
FAIL	github.com/nhooyr/scratch	1.006s

Which made me scratch my head for quite a while.

I want to repeat that I know this is a violation of the docs and so maybe shouldn't be fixed but at the same time, I think that it would be much easier to detect such failures if t.Fail reported the line number at which it was called. This would have made the above issue easy to debug and I can imagine a line number reported by t.Fail to be useful in other scenarios as well.

@nhooyr nhooyr changed the title t.Fail should report the line on which the failure occurred testing: t.Fail should report the line on which the failure occurred Jul 31, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

It would be nice to do better here but what you seem to suggest seems impossible. At the time that the goroutine calls t.Error the subtest started by t.Run is complete and has passed. There isn't any way for us to retroactively decide that it has failed.

But it does seem possible to have t.Fail detect that the test has completed, and to act differently in that case.

@wselwood

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

Taking a look at this at the contributor workshop in London... Will update again with more info

@gopherbot

This comment has been minimized.

Copy link

commented Aug 2, 2018

Change https://golang.org/cl/127596 mentions this issue: testing: report line of failure even if test has already completed

@wselwood

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

Hello @ianlancetaylor is there a chance of a code review on the cl in https://go-review.googlesource.com/c/127596/ or pointing who would be the right person to do a code review? This being my first one I'd like to know I've got it "right"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.