-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Comments
This is similar to the workaround we use in the stdlib, e.g., |
That said, it would be nice to have something ergonomic built in to |
To avoid declaring new for _, tt := range tests {
- t.Run(tt.name, func(t *testing.T) {
+ t.Run(tt.name.String(), func(t *testing.T) {
...
})
} where |
It seems like there are two things going on here:
It seems like we could keep t.Run the same, pass tt.name.String() to it, and then separately have a
call at the start of the function body. Then other sources of file:line (like testscript failures) can hook into this too. |
This proposal has been added to the active column of the proposals project |
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 To make this work, the traditional test case "table" has to be modified a bit.
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 |
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? |
Since new type is introduced maybe there is no need to have 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")
}
})
}
}
|
@AlexanderYastrebov, you would need two different |
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")
}
})
}
}
|
@dsnet, any thoughts on #52751 (comment) ? |
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. In response to #52751 (comment), one significant detriment to |
@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. |
Ping @dsnet |
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 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
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 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 location := fileLine()
...
t.Println(fmt.Sprintf("%v: test case definition", location)) Where 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:
Edit: it is possible to use Edit 2: it seems as of go1.20 the stdout is now interleaved with |
It's unfortunate that GoLand doesn't adequately hyperlink
I'm starting to lean towards this approach. I haven't quite come to terms with how much this should be in (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 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 One way to address this is to have 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 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 That said, I'd be okay even with something like |
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
which adds that file:line to the list of file lines that get printed. For example if you do
the message would look like
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
Then we could do something like:
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? |
Overall, SGTM. Some questions about semantics:
|
Let's try to answer those questions:
Thoughts? |
It seems like people are mostly happy with #52751 (comment)? |
On hold for an implementation to sanity check this idea. |
Placed on hold. |
When combined with the new 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 In #52751 (comment), I concluded that I still believed the best solution was still a new
While I advocated for a new |
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. |
GoLand (the JetBrains IDE for Go) does not support this. Here's an example screenshot from the terminal window in GoLand: |
Is this perhaps instead an argument that there's a variation of
Quite possibly. I would support this semantic more than the stacking up semantic, but it could interact poorly with usages of |
I think that would address half the problem. In the example above More importantly, I think that behaviour raises a new question. There's already a much more intuitive way of adding that information to a Given this implementation of type Pos = token.Position
func Mark() Pos {
_, file, line, _ := runtime.Caller(1)
return Pos{Filename: filepath.Base(file), Line: line}
} The 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 |
@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. |
As far as the pos stack issues, what if instead we do:
|
Change https://go.dev/cl/495857 mentions this issue: |
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. |
@rsc. It's not my preferred API, but I'll take any improvement over the status quo. Thanks for working on this. |
Have all remaining concerns with this proposal been addressed? |
@rsc My preferred option is to add |
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 |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
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:
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 |
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:
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. |
I've tried 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 The 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 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):
This adds no new lines to the test and you get an immediate warning if you forget a |
Regarding the proposed |
Change https://go.dev/cl/522880 mentions this issue: |
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>
In Go, it is very common to use table-driven tests:
When this test fails, it prints with something like:
my_test.go:1234
tells us where in the test logic this failed.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:Using this API, the example above would be changed as:
testing.Name
in every test case, which captures file and line information about where the test case was declared.testing.T.RunName
and pass it thetesting.TestName
so that the subtest knows what test case is associated with this subtest.Thus, the test output would be something like:
my_test.go:321
tells us where the test data was declared.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.The text was updated successfully, but these errors were encountered: