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: add Name to track file and line of test case declaration #52751

Open
dsnet opened this issue May 7, 2022 · 66 comments
Open

testing: add Name to track file and line of test case declaration #52751

dsnet opened this issue May 7, 2022 · 66 comments

Comments

@dsnet
Copy link
Member

dsnet commented May 7, 2022

In Go, it is very common to use table-driven tests:

tests := struct {
    name string
    input T
    ...
} {{
    name: "Foo",
    ...,
}, {
    name: "Bar",
    ...,
}
... // maybe dozens or hundreds more cases
}
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        got, err := fizz(tt.input)
        if err != nil {
            t.Fatalf("fizz error: %v", err) // my_test.go:1234
        }
    })
}

When this test fails, it prints with something like:

--- FAIL: Test/Bar (0.00s)
    my_test.go:1234: fizz error: the world blew up
  • The my_test.go:1234 tells us where in the test logic this failed.
  • The Test/Bar name tells us which test case failed.

Most code editors today identify source.go:1234 strings and automatically provide the ability to jump to that source code location. This is really helpful for jumping to the execution logic that failed, but is not helpful for jumping to the test data that caused the failure. It is impossible for editor tooling to automatically correlate the the test name (e.g., Test/Bar) with the test case in the code since the association between the two can be determined by arbitrary Turing-complete logic.

I propose the following API in the testing package:

// NameFileLine is a name combined with a file and line number.
type NameFileLine struct { ... }

// Name constructs a NameFileLine.
// It annotates the name with the file and line number of the caller.
func Name(name string) NameFileLine

// RunName runs f as a subtest of t called name.
func (t *T) RunName(name NameFileLine, f func(t *testing.T))

// RunName runs f as a subtest of b called name.
func (b *B) RunName(name NameFileLine, f func(b *testing.B))

Using this API, the example above would be changed as:

  tests := struct {
-     name string
+     name testing.NameFileLine
      input T
      ...
  } {{
-     name: "Foo",
+     name: testing.Name("Foo"),
      ...,
  }, {
-     name: "Bar",
+     name: testing.Name("Bar"), // my_test.go:321
      ...,
  }
  ... // maybe dozens or hundreds more cases
  }
  for _, tt := range tests {
-     t.Run(tt.name, func(t *testing.T) {
+     t.RunName(tt.name, func(t *testing.T) {
          got, err := fizz(tt.input)
          if err != nil {
              t.Fatalf("fizz error: %v", err) // my_test.go:1234
          }
      })
  }
  • We call testing.Name in every test case, which captures file and line information about where the test case was declared.
  • We call testing.T.RunName and pass it the testing.TestName so that the subtest knows what test case is associated with this subtest.

Thus, the test output would be something like:

--- FAIL: Test/Bar (0.00s)
    my_test.go:321: my_test.go:1234: fizz error: the world blew up
  • The my_test.go:321 tells us where the test data was declared.
  • The my_test.go:1234 tells us where in the test logic this failed.

Now, we can click on my_test.go:321 in our code editors and it will take us directly to the test case declaration.

@dsnet dsnet added the Proposal label May 7, 2022
@gopherbot gopherbot added this to the Proposal milestone May 7, 2022
@randall77
Copy link
Contributor

randall77 commented May 7, 2022

tests := struct {
    name string
    fileLine string
    input T
} {
    name: "foo",
    fileLine: fileLine(),
    ...
}

func fileLine() string {
    _, file, line, _ := runtime.Caller(1)
    return file + ":" + line
}

This is similar to the workaround we use in the stdlib, e.g., reflect.verifyGCBits.

@randall77
Copy link
Contributor

That said, it would be nice to have something ergonomic built in to testing. I wish testing.Run took an any instead of a string as the name, so we didn't have to introduce another method on Testing to enable this.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 7, 2022
@dsnet
Copy link
Member Author

dsnet commented May 9, 2022

To avoid declaring new RunName methods, we could do something like:

  for _, tt := range tests {
-     t.Run(tt.name, func(t *testing.T) {
+     t.Run(tt.name.String(), func(t *testing.T) {
          ...
      })
  }

where testing.NameFileLine.String prints in a special syntax recognized by testing.T.Run and testing.B.Run. However, this might be considered a breaking change.

@rsc
Copy link
Contributor

rsc commented May 11, 2022

It seems like there are two things going on here:

  1. The trick about testing.Name recording where it was called from.
  2. The ability to add file:line annotations to test failures.

It seems like we could keep t.Run the same, pass tt.name.String() to it, and then separately have a

t.AddFileLine(tt.name.FileLine())

call at the start of the function body. Then other sources of file:line (like testscript failures) can hook into this too.

@rsc
Copy link
Contributor

rsc commented May 11, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) May 11, 2022
@dnephin
Copy link
Contributor

dnephin commented May 12, 2022

I have encountered this problem a few times. I'll share the workaround I came up with in case it helps this proposal in some way.

Using t.Helper and a run function I was able to have the tests emit at least 2 file and line number outputs: one for the first line of the test case definition, and one from the test logic function where the failure happened.

To make this work, the traditional test case "table" has to be modified a bit.

  • the logic of the test is defined at the top in a run function, instead of at the end.
  • instead of a slice or map of test cases, each one calls run. It's a few extra characters per test case.

Using the example test from the description, it might look like this:

type testCase struct {
    name string
    input T
    ...
}

run := func(t *testing.T, tc testCase) {
    t.Helper()
    t.Log("case:", tc.name)
    t.Run(name, func(t *testing.T) {
        got, err := fizz(tt.input)
        if err != nil {
            t.Fatalf("fizz error: %v", err) // my_test.go:1234
        }
    })
}

run(t, testCase{
    name: "Foo",
    ...,
}
run(t, testCase{
    name: "Bar",
    ...,
}
... // maybe dozens or hundreds more cases

Definitely not as elegant as the fileLine workaround. t.AddFileLine would be a great addition!

@rsc
Copy link
Contributor

rsc commented May 25, 2022

I'd still like to understand better whether we can separate out the two different features being added, as I noted in #52751 (comment). Any thoughts, @dsnet?

@AlexanderYastrebov
Copy link
Contributor

Since new type is introduced maybe there is no need to have String and AddFileLine methods but just a Run wrapper:

package go52751

import (
	"fmt"
	"runtime"
	"testing"
)

type TC struct {
	name     string
	location string
}

func (tc *TC) Run(t *testing.T, tf func(t *testing.T)) bool {
	t.Helper()
	return t.Run(tc.name, func(t *testing.T) {
		t.Helper()
		// this should use internal undecorated logging to achieve desired output
		t.Logf("Test case %q defined at %s", tc.name, tc.location)
		tf(t)
	})
}

func testingCase(name string) *TC {
	_, file, line, _ := runtime.Caller(1)
	return &TC{name, fmt.Sprintf("%s:%d", file, line)}
}

func TestFileLine(t *testing.T) {
	tests := []struct {
		tc    *TC
		input string
	}{{
		tc:    testingCase("Foo"),
		input: "x",
	}, {
		tc:    testingCase("Bar"),
		input: "",
	},
	}
	for _, tt := range tests {
		tt.tc.Run(t, func(t *testing.T) {
			if tt.input == "" {
				t.Fatalf("input error")
			}
		})
	}
}
--- FAIL: TestFileLine (0.00s)
    --- FAIL: TestFileLine/Bar (0.00s)
        case_test.go:42: Test case "Bar" defined at /tmp/go52751/case_test.go:37
        case_test.go:44: input error
FAIL
FAIL	command-line-arguments	0.001s
FAIL

@dsnet
Copy link
Member Author

dsnet commented May 27, 2022

@AlexanderYastrebov, you would need two different Run methods since the method must taking in either a func(*testing.T) or a func(*testing.B). It's not clear that it's a cleaner API than what was originally proposed.

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented May 28, 2022

@dsnet

diff --git a/src/testing/testing.go b/src/testing/testing.go
index ec2d864822..dc1bfc301e 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -529,6 +529,8 @@ type common struct {
        tempDir    string
        tempDirErr error
        tempDirSeq int32
+
+       cases map[string]string
 }
 
 // Short reports whether the -test.short flag is set.
@@ -1451,6 +1453,27 @@ func tRunner(t *T, fn func(t *T)) {
        t.mu.Unlock()
 }
 
+func (t *T) Case(name string) string {
+       _, file, line, _ := runtime.Caller(1)
+       location := fmt.Sprintf("%s:%d\n", file, line)
+
+       t.mu.Lock()
+       if t.cases == nil {
+               t.cases = make(map[string]string)
+       }
+       uniqName := name
+       for i := 1; ; i++ {
+               if _, ok := t.cases[uniqName]; !ok {
+                       break
+               }
+               uniqName = fmt.Sprintf("%s#%d", name, i)
+       }
+       t.cases[uniqName] = location
+       t.mu.Unlock()
+
+       return uniqName
+}
+
 // Run runs f as a subtest of t called name. It runs f in a separate goroutine
 // and blocks until f returns or calls t.Parallel to become a parallel test.
 // Run reports whether f succeeded (or at least did not fail before calling t.Parallel).
@@ -1463,6 +1486,15 @@ func (t *T) Run(name string, f func(t *T)) bool {
        if !ok || shouldFailFast() {
                return true
        }
+
+       if loc, ok := t.cases[name]; ok {
+               fo := f
+               f = func(t *T) {
+                       t.Helper()
+                       t.Logf("case at %s", loc)
+                       fo(t)
+               }
+       }
        // Record the stack trace at the point of this call so that if the subtest
        // function - which runs in a separate stack - is marked as a helper, we can
        // continue walking the stack into the parent test.
package go52751

import (
	"testing"
)

func TestFileLine(t *testing.T) {
	tests := []struct {
		name  string
		input string
	}{{
		name:  t.Case("Foo"),
		input: "x",
	}, {
		name:  t.Case("Bar"),
		input: "",
	},
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			if tt.input == "" {
				t.Fatalf("input error")
			}
		})
	}
}
--- FAIL: TestFileLine (0.00s)
    --- FAIL: TestFileLine/Bar (0.00s)
        case_test.go:20: case at /tmp/go52751/case_test.go:15
        case_test.go:22: input error

@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

@dsnet, any thoughts on #52751 (comment) ?

@dsnet
Copy link
Member Author

dsnet commented Jun 8, 2022

In regards to #52751 (comment), it seems odd if the test name is annotated with file:line information if we can directly use the test name and file:line information together. There's a discrepancy between how creation and usage operates.

If usage is segregated, then creation should also be segregated:

  tests := struct {
      name string
+     location testing.SourceLocation
      input T
      ...
  } {{
      name: "Foo",
+     location: testing.FileLine(),
      ...,
  }, {
      name: "Bar",
+     location: testing.FileLine(), // my_test.go:321
      ...,
  }
  ... // maybe dozens or hundreds more cases
  }
  for _, tt := range tests {
      t.Run(tt.name, func(t *testing.T) {
+         t.Annotate(tt.location)
          got, err := fizz(tt.input)
          if err != nil {
              t.Fatalf("fizz error: %v", err) // my_test.go:1234
          }
      })
  }

This approach is more flexible as you can imagine using this for more purposes than just annotating test case names.
However, it is a little more typing than my original proposal in #52751 (comment).
I would be okay with something like this API.


In response to #52751 (comment), one significant detriment to func (t *T) Case(name string) string is that it assumes that we have a testing.T on hand when crafting the test cases. I have some tests that share a global testcases table. There is no single testing.T around at the time that the table was constructed.

@rsc
Copy link
Contributor

rsc commented Jun 8, 2022

@dsnet, you jumped to testing.SourceLocation, which I didn't really have in mind. I think testing.Name returning something that has a name and a file:line is fine. But there are other tests reading test data files that might also want to set the file:line, and I was suggesting that they could use that mechanism too if it were split out. I don't think we need a SourceLocation type though.

So only the t.Annotate would be added in the diff, not all the testing.FileLine() calls.

@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

Ping @dsnet

@dnephin
Copy link
Contributor

dnephin commented Jul 27, 2022

I tried out a version of this in one of my test cases, and it seems that at least GoLand IDE does not highlight multiple file:line on the same line. The first instance of file:line on each output line is a hyperlink to the file in the IDE, but subsequent ones are just plain text.

I'm less familiar with other environments, but I expect even in environments that don't provide hyperlinks to the source it wont be obvious to someone running the tests what each of the two file:line are supposed to reference.

The ability to add file:line annotations to test failures.

I agree this is what is missing, and I think this would be more valuable if it worked similar to #52751 (comment). Instead of a second file:line number on the line, the location would replace the one that came from t.Log.

As multiple people have mentioned on this thread, capturing the file and line number is pretty easy to do, and could be done in different ways depending on the test.

If testing.T had a function that logged without any decoration a caller could pass in the decoration themselves:

location := fileLine()
...
t.Println(fmt.Sprintf("%v: test case definition", location))

Where Println would be like Log without c.deocrate:

func (c *common) Println(args ...any) {
    ... // handle c.done, and c.chatty branches
    c.output = append(c.output, fmt.Sprint(args...))
}

The output from the original proposal would look something like this:

--- FAIL: Test/Bar (0.00s)
    my_test.go:321: test case definition
    my_test.go:1234: fizz error: the world blew up

Edit: it is possible to use fmt.Println to remove the decoration, but that output appears in a different place from t.Log output in the standard (non-verbose) go test format.

Edit 2: it seems as of go1.20 the stdout is now interleaved with t.Log output, but is not indented the same way.

@dsnet
Copy link
Member Author

dsnet commented Aug 3, 2022

it seems that at least GoLand IDE does not highlight multiple

It's unfortunate that GoLand doesn't adequately hyperlink file:line: strings. I use VSCode, which does a decent job at it. Regardless of IDE support, the testing package already outputs file:line: annotations for test error and log messages and so doing more of it should not be a surprise.

If testing.T had a function that logged without any decoration a caller could pass in the decoration themselves:

I'm starting to lean towards this approach. I haven't quite come to terms with how much this should be in testing versus how much this should be a helper in runtime.

(I'm going to go on a tangent now regarding logging but it is related)

The other day I was dealing with log functions and composing them together. In my use case, I had a log.Logger.Printf and also another logf-like function that wrote to a different sink. I wanted to compose these two together so a single logf would write to both:

func multiLogf(logfs ...func(string, ...any)) func(string, ...any) {
    return func(f string, a ...any) {
        for _, logf := range logfs {
            logf(f, a...)
        }
    }
}

The problem with this is that log.Lshortfile does not compose well and now always records the callsite in my multiLogf helper function, making it fairly useless.

One way to address this is to have logf-like functions never record file:line and have the caller do it manually. However, we would want this to be easy to do. Unfortunately, the runtime.Caller API is not particularly friendly because it returns multiple arguments, making it difficult to use in a Go expression.

Imagine there was a:

package runtime

// FileLine returns the file:line of the caller.
// The file is only the base file name.
func FileLine() string

then I could have done:

logf("%s: payload too large for %v", runtime.FileLine(), id)

Similarly, we can accomplish what this proposal was originally about with:

  tests := struct {
+     where string
      name  string
      input T
      ...
  } {{
+     where: runtime.FileLine(),
      name:  "Foo",
      ...,
  }, {
+     where: runtime.FileLine(),
      name:  "Bar",
      ...,
  }
  ... // maybe dozens or hundreds more cases
  }
  for _, tt := range tests {
      t.Run(tt.name, func(t *testing.T) {
+         t.Logf("%s: test case declaration", tc.where)
          got, err := fizz(tt.input)
          if err != nil {
              t.Fatalf("fizz error: %v", err)
          }
      })
  }

Having a helper function in runtime makes grabbing the "file:line" useful for more applications than just testing.
However, I'd be okay with a helper also in testing that combined creation of a name and also where it was called from (essentially point 1 in #52751 (comment)).

My concern with point 2 in #52751 (comment) is that I'm uncertain how this information should be presented in the terminal UI and also what happens when t.AddFileLine(tt.name.FileLine()) is called multiple times. Making it something we annotate manually with testing.TB.Logf allows for more experimentation.

That said, I'd be okay even with something like testing.TB.AddFileLine if we can nail down the semantics. Alternatively, a testing.TB.Println method that doesn't log the line like what @dnephin suggested could work. The downside with that approach is that some users will call testing.TB.Println when they really should be calling testing.TB.Log.

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

I don't understand why we'd add a Println etc that does not log the source file line. If the caller is going to add their own explicitly, there's no harm in having two file:line in the output. I have tests that do that (for example cmd/go TestScript) and it works just fine. You get the source line of the log message as well as the source line of the corresponding data and can click on either one depending on what you are looking for.

Suppose we had

func (t *T) Source(file string, line int) 

which adds that file:line to the list of file lines that get printed. For example if you do

t.Source("mydata.txt", 2)
t.Fatalf("oops")

the message would look like

mydata.txt:2: my_test.go:123: oops

That seems generally useful, and I would probably use it in TestScript instead of the manual prints that I have today.

Then the remaining question is how to make it easy to get a file,line to pass to Source for a table-driven test.

Suppose we defined

// A Marker records a name at a given Go source file position.
type Marker struct {
    Name string
    File string
    Line int
}

func (m Marker) Source() (string, int) { return m.File, m.Line }

// Mark returns a Marker with the given name
// that also records the Go source file position where it appears.
func Mark(name string) Marker

Then we could do something like:

var squareTests = []struct{
    m Marker
    in int
    out int
}{
    {testing.Mark("zero"), 0, 0},
    {testing.Mark("negative"), -2, 4},
    {testing.Mark("positive"), 3, 9},
    {testing.Mark("huge"), 1000, 1000_000},
}

for _, tt := range squareTests {
    t.Run(tt.m.Name, func(t *testing.T) {
        t.Source(tt.m.Source())
        if out := tt.in*tt.in; out != tt.out {
            t.Errorf("%d*%d = %d, want %d", tt.in, tt.in, out, tt.out)
        }
    })
}

It seems to me that this separation provides two generally useful concepts that work well apart as well as together, instead of tying them together into one specialized concept.

Thoughts?

@dsnet
Copy link
Member Author

dsnet commented Aug 25, 2022

Overall, SGTM.

Some questions about semantics:

  • What happens when Source is called multiple times? Do they stack up?
  • When you call testing.T.Run, does it preserve the source called on the parent T?
  • Does Source affect the output of a panic? If a test panics, I would expect the source to be printed somewhere since that's relevant information.

@rsc
Copy link
Contributor

rsc commented Aug 31, 2022

Let's try to answer those questions:

  1. Calling Source multiple times stacks.
  2. Calling Run starts the new T with the Source info from the old T, and then the new one can stack additional ones.
  3. We should probably print a log message on all panics, like t.Log("PANIC:", v), and then that will show the source lines for us.

Thoughts?

@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

It seems like people are mostly happy with #52751 (comment)?
Does anyone want to try to implement it and see if it makes any sense in the implementation?

@rsc
Copy link
Contributor

rsc commented Oct 5, 2022

On hold for an implementation to sanity check this idea.

@rsc
Copy link
Contributor

rsc commented Oct 6, 2022

Placed on hold.
— rsc for the proposal review group

@gopherbot
Copy link

Change https://go.dev/cl/444195 mentions this issue: testing: add TB.Source to support user-specified source files

@dsnet
Copy link
Member Author

dsnet commented Oct 19, 2022

I sent out CL 444195 as a prototype implementation.

Example usage:

func Test(t *testing.T) {
	tests := []struct {
		testing.Marker
		in   int
		want string
	}{
		{testing.Mark("Foo"), 1, "1"},
		{testing.Mark("Bar"), 2, "2"},
		{testing.Mark("Baz"), 3, "3"},
		{testing.Mark("Giz"), 4, "4"},
		{testing.Mark("Daz"), 5, "5"},
	}
	for _, tt := range tests {
		t.Run(tt.Name, func(t *testing.T) {
			t.Source(tt.Source())
			got := strconv.Itoa(2 * (tt.in / 2)) // faulty conversion
			if got != tt.want {
				t.Errorf("Itoa(%d) = %v, want %v", tt.in, got, tt.want)
			}
		})
	}
}

Example output:

--- FAIL: Test (0.00s)
    --- FAIL: Test/Foo (0.00s)
        main_test.go:16: main_test.go:27: Itoa(1) = 0, want 1
    --- FAIL: Test/Baz (0.00s)
        main_test.go:18: main_test.go:27: Itoa(3) = 2, want 3
    --- FAIL: Test/Daz (0.00s)
        main_test.go:20: main_test.go:27: Itoa(5) = 4, want 5

where lines 16, 18, 20 are where the test case was declared,
and line 27 is where a test error was reported.

Overall, I generally like the API except for the following:

  • The t.Source(tt.Source()) call stutters. I don't have any great ideas. Having TB.Source take in a testing.Marker seems wrong.

  • The API can be easy to hold wrong.

    Consider the following modification of the test above:

      for _, tt := range tests {
    + 	t.Source(tt.Source())
    	t.Run(tt.Name, func(t *testing.T) {
    - 		t.Source(tt.Source())
      		got := strconv.Itoa(2 * (tt.in / 2)) // faulty conversion
      		if got != tt.want {
      			t.Errorf("Itoa(%d) = %v, want %v", tt.in, got, tt.want)
      		}
      	})
      }

    where the call to t.Source(tt.Source()) was simply flipped,
    which would result in an output of:

    --- FAIL: Test (0.00s)
        --- FAIL: Test/Foo (0.00s)
            main_test.go:16: main_test.go:27: Itoa(1) = 0, want 1
        --- FAIL: Test/Baz (0.00s)
            main_test.go:16: main_test.go:17: main_test.go:18: main_test.go:27: Itoa(3) = 2, want 3
        --- FAIL: Test/Daz (0.00s)
            main_test.go:16: main_test.go:17: main_test.go:18: main_test.go:19: main_test.go:20: main_test.go:27: Itoa(5) = 4, want 5
    

    The multiple calls to Source is stacked up per the semantics suggested in testing: add Name to track file and line of test case declaration #52751 (comment).

    One way to address this is to have Source only remember the last provided source.
    This would generally work well for any non-parallel tests (which are the most common).

Alternatively, the originally proposed API of have a new Run method would solve both issues:

func (*T) RunMarker(Marker, func(*T))

since each instantiation of T may have at most one user-specified source location associated with it.
Furthermore, the source location is immutable once associated,
which avoids any strange business with it being unintentionally stacked up or replaced.
This would simplify the implementation since we can avoid grabbing locks for all the AP calls.

@dsnet
Copy link
Member Author

dsnet commented Oct 19, 2022

I think my ideal API for this continues to be something like:

type Label struct {
	Name string
	File string
	Line int
}
func Mark(name string) Label

func (*T) RunLabel(Label, func(*T))
func (*B) RunLabel(Label, func(*B))

As a bikeshed, renaming Marker to Label seems clearer to me as the testing.Marker type by itself doesn't really tell me anything. An alternative verb for Mark could be Annotate.

@dnephin
Copy link
Contributor

dnephin commented Feb 10, 2023

It's unlikely that anyone is currently defining a test name containing a newline, since I'm pretty sure test output breaks if you do.

I believe the newline is converted to a _ in the output (like other whitespace characters): https://go.dev/play/p/Ic4FH_ZKkzW, so it's possible somewhere there are test names with newlines, but most likely it's rare.

I like the approach of associating the marker with a name. Earlier suggestions like #52751 (comment) seemed like a nice way to make that work.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2023

It seemed like we were at a reasonable point with

type Pos struct {File string; Line int}
testing.Mark() Pos
t.AddPos(testCase.pos)

There was a concern raised about multiple file:line: on a given line, but that should be plenty legible, and at least the tools I use have no problem with linking to either one. If others know of specifically broken environments that don't support that, let's look at concrete cases.

The discussion moved over to trying to smuggle information into the t.Run argument, but that's a pretty major change, and a confusing one. We should try to use data types and data structures instead of appending things to the t.Run argument.

Other than the question of how to print two file:line associated with a message, are there other concerns remaining about this proposal?

@dsnet
Copy link
Member Author

dsnet commented Mar 12, 2023

There was a concern raised about multiple file:line: on a given line, but that should be plenty legible

When combined with the new -fullpath flag in #37708, I don't see how this will remain legible. I'd argue there's only enough space for a single full path to be comfortable printed per line.

I also commented in #52751 (comment) that the API was easy to accidentally misuse with a one-line mistake like:

  for _, tt := range tests {
+ 	t.Source(tt.Source())
	t.Run(tt.Name, func(t *testing.T) {
- 		t.Source(tt.Source())
  		...
  	})
  }

which would drastically change the way test output was formatted. The added line would (unintentionally) cause multiple source positions to be stacked up, while the removed line would (correctly) only print one source position per t.Run invocation.

In #52751 (comment), I concluded that I still believed the best solution was still a new RunLabel method, which solves both issues:

  1. it ensures that at most one path may be associated with a test name, avoiding unreadable stack-up of many source paths (especially if -fullpath is specified)
  2. Since the source information is provided at the same time to Run, there is no possibility of accidentally registering at the wrong time (i.e., too early or too late).

While I advocated for a new RunLabel API, @neild suggested hijacking the existing Run method and giving special semantics to certain strings to be treated as a file path. Personally, I'd be happy with either a new RunLabel method or modifying Run to detect file paths.

@rsc
Copy link
Contributor

rsc commented Mar 15, 2023

Mixing the source into Run seems like it will end up being a non-orthogonal issue and lead to more, and then we'll need all the possible combinations of arguments to Run. A helper on testing.T seems clearer, and mostly separates it from Run.

As for the mistake of calling t.Source in the outer loop, maybe Source should only be setting a single source instead of stacking up? Then it's not tied to Run. It would probably be t.SetPos then, and the test log messages would print the Go source line as well as the position set by SetPos.

@dnephin
Copy link
Contributor

dnephin commented Mar 28, 2023

If others know of specifically broken environments that don't support [multiple file:line: on a given line], let's look at concrete cases.

GoLand (the JetBrains IDE for Go) does not support this. Here's an example screenshot from the terminal window in GoLand:

goland-file-line

@dsnet
Copy link
Member Author

dsnet commented Mar 28, 2023

we'll need all the possible combinations of arguments to Run

Is this perhaps instead an argument that there's a variation of Run with variadic optional arguments? 🤔

maybe Source should only be setting a single source instead of stacking up

Quite possibly. I would support this semantic more than the stacking up semantic, but it could interact poorly with usages of T.Parallel.

@dnephin
Copy link
Contributor

dnephin commented Mar 28, 2023

maybe Source should only be setting a single source instead of stacking up? Then it's not tied to Run. It would probably be t.SetPos then, and the test log messages would print the Go source line as well as the position set by SetPos.

I think that would address half the problem. In the example above t.SetPos is still being called on the wrong testing.T. It's called on the root test case testing.T. Would a call to t.SetPos on the root test case properly add the position to the next t.Log in a sub-test case? Or only the next call to t.Log on the root test case?

More importantly, I think that behaviour raises a new question. There's already a much more intuitive way of adding that information to a t.Log or t.Fatal. For example:

Given this implementation of Pos and Mark:

type Pos = token.Position

func Mark() Pos {
	_, file, line, _ := runtime.Caller(1)
	return Pos{Filename: filepath.Base(file), Line: line}
}

The Pos can be used directly:

pos := testing.Mark()
...
t.Fatalf("%v: fizz error: %v", pos, err)
// or
t.Log(pos, "test case data")

I believe this would produce the same output as t.SetPos, and to me adding text to an existing output line is more obvious than calling a separate function. Why would someone use t.SetPos ?

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

@dnephin, I appreciate the GoLand example. I would suggest that GoLand should add support for that syntax, since existing Go tests already print it independent of this discussion.

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

As far as the pos stack issues, what if instead we do:

package testing

// A Pos is a file:line recording a test data position.
type Pos struct {File string; Line int}

// Mark reports the file:line position of the source file in which it appears.
func Mark() Pos

// SetPos sets the test data position associated with the test.
// Test output using the [T.Log] and [T.Error] method families 
// reports both the source file:line of the method call (as adjusted by [T.Helper])
// and the test data position, if any.
// Calling SetPos with the zero Pos clears the test data position.
func (*T) SetPos(pos Pos)

@gopherbot
Copy link

Change https://go.dev/cl/495857 mentions this issue: testing: add Pos, Mark, SetPos

@rsc
Copy link
Contributor

rsc commented May 17, 2023

I implemented the latest version of this proposal in CL 495857. Please try it out if you are interested. Note that Mark only records the basename of the file (the last element) in the File field. Otherwise you get very long full paths that you almost certainly don't want.

@dsnet, @AlexanderYastrebov, @flowchartsman, any thoughts? Thanks.

@dsnet
Copy link
Member Author

dsnet commented Jun 29, 2023

@rsc. It's not my preferred API, but I'll take any improvement over the status quo. Thanks for working on this.

@rsc
Copy link
Contributor

rsc commented Jul 5, 2023

Have all remaining concerns with this proposal been addressed?

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Jul 5, 2023

@rsc My preferred option is to add t.Case("foo") and/or testing.Case("foo") (to support static testcase tables) as outlined in #52751 (comment) and #52751 (comment)

@dnephin
Copy link
Contributor

dnephin commented Jul 8, 2023

The original proposal, and some others in the comments, seem like they might have benefited more from being part of the stdlib.

The proposed implementation of Mark and Pos can be implemented outside the stdlib in ~6 lines, and SetPos is equivalent to t.Logf("%v: ...", pos) (without having to unset the position). Is this common enough to add to the stdlib or (like table tests) could this be documented as a common Go testing pattern (possibly in the godoc for testing)?

@rsc
Copy link
Contributor

rsc commented Jul 12, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jul 19, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: testing: add Name to track file and line of test case declaration testing: add Name to track file and line of test case declaration Jul 19, 2023
@rsc rsc modified the milestones: Proposal, Backlog Jul 19, 2023
@prashantv
Copy link
Contributor

prashantv commented Jul 21, 2023

This is a great addition!

One comment on the latest approach -- it adds a fair bit of noise compared to the original proposal, based on my understanding.

Comparing the original:

tests := struct {
    name testing.NameFileLine
    input T
    ...
} {{
    name: testing.Name("Foo"),
    ...,
}, {
    name: testing.Name("Bar"),
    ...,
}
... // maybe dozens or hundreds more cases
}
for _, tt := range tests {
    t.RunName(tt.name, func(t *testing.T) {
        got, err := fizz(tt.input)
        if err != nil {
            t.Fatalf("fizz error: %v", err) // my_test.go:1234
        }
    })
}

With the current API, it it adds:

  • A pos testing.Pos to the definition of the struct, and having each test table entry repeat pos: testing.Mark(),
  • Calling t.SetPos within each t.Run.
tests := struct {
    name string
    pos testing.Pos
    input T
    ...
} {{
    name: "Foo",
    pos: testing.Mark(),
    ...,
}, {
    name: "Bar",
    pos: testing.Mark(),
    ...,
}
... // maybe dozens or hundreds more cases
}
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        t.SetPos(tt.pos)

        got, err := fizz(tt.input)
        if err != nil {
            t.Fatalf("fizz error: %v", err) // my_test.go:1234
        }
    })
}

The original proposal is more ergonomic, and I also like the solution proposed @AlexanderYastrebov, though it requires testing.T, that is available in almost all cases, and it ends up reducing the noise significantly.

@rogpeppe
Copy link
Contributor

rogpeppe commented Aug 8, 2023

I tend to agree with @prashantv that this seems noisier than necessary in table-driven tests, requiring the addition of a single identical line in every entry of every table. I'd prefer it if there was an easy way to associate the mark with a label, as the original proposal did. Of course, it's not hard to write something like that oneself:

type Label struct {
	testing.Pos
	Name string
}

func Label(name string) Label {
	_, file, line, _ := runtime.Caller(1)
	return Label{
		Pos: testing.Pos{
			File: file,
			Line: line,
		},
		Name: name,
	}
}

but I don't particularly like the idea of either copy/pasting that everywhere or of defining a tiny module just to import that code.

@neild
Copy link
Contributor

neild commented Aug 16, 2023

I've tried testing.Mark out in some real test code, and I have to say I'm not really enamored of it.

As @prashantv says, this is noisy: You need a new entry in the test case, a new identical line in every entry in the table, and a call to t.SetPos in the test.

The pos: testing.Mark(), lines are pure nose, and if you forget one nothing warns you. You also don't get any warning if you forget the t.SetPos(test.pos).

The position doesn't integrate with the test case name at all, so this doesn't help with naming if you have a table of undifferentiated test cases that you want to put into subtests. Perhaps that's not a problem; if you want arbitrary names for subtests, you can use t.Run("", ...) and just get a number.

I'm not particularly fond of the output, either, but perhaps I'd get used to it.

Overall, however, this feels like too much mechanism leaking into the test code for not enough return.

I think I'd prefer something along the lines of @AlexanderYastrebov's earlier suggestion, perhaps (with a small modification):

for _, test := range []struct{
  case testing.Case
}{{
  case: t.Case("test name"),
}}{
  test.case.Run(func(t *testing.T) {
  })
}

This adds no new lines to the test and you get an immediate warning if you forget a testing.Case call.

@dsnet
Copy link
Member Author

dsnet commented Aug 24, 2023

Regarding the proposed testing.Case type with a Run method: it would need to operate on both testing.B and testing.T. In @neild's comment, it seems that it only accepts testing.T.

@gopherbot
Copy link

Change https://go.dev/cl/522880 mentions this issue: encoding/json: modernize tests

gopherbot pushed a commit that referenced this issue Aug 25, 2023
There are no changes to what is being tested.
No test cases were removed or added.

Changes made:
* Use a local implementation of test case position marking. See #52751.
* Use consistent names for all test tables and variables.
* Generally speaking, follow modern Go style guide for tests.
* Move global tables local to the test function if possible.
* Make every table entry run in a distinct testing.T.Run.

The purpose of this change is to make it easier to perform
v1-to-v2 development where we want v2 to support close to
bug-for-bug compatibility when running in v1 mode.

Annotating each test case with the location of the test data
makes it easier to jump directly to the test data itself
and understand why this particular case is failing.

Having every test case run in its own t.Run makes it easier
to isolate a particular failing test and work on fixing the code
until that test case starts to pass again.

Unfortunately, many tests are annotated with an empty name.
An empty name is better than nothing, since the testing framework
auto assigns a numeric ID for duplicate names.
It is not worth the trouble to give descriptive names to each
of the thousands of test cases.

Change-Id: I43905f35249b3d77dfca234b9c7808d40e225de8
Reviewed-on: https://go-review.googlesource.com/c/go/+/522880
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

10 participants