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

Add support for testing.T.Run() metadata to ParseError() #1262

Closed
ghost opened this issue Apr 16, 2017 · 4 comments
Closed

Add support for testing.T.Run() metadata to ParseError() #1262

ghost opened this issue Apr 16, 2017 · 4 comments
Assignees

Comments

@ghost
Copy link

ghost commented Apr 16, 2017

Table-driven tests iterate over a table of inputs to permute a single test to cover multiple sub-cases. To accomplish table-drive tests, tests use t.Run() to execute tests; the pattern is:

t.Run(subcase_name, func(t *testing.T) {
   t.Errorf("sample error")
})

In this pattern, testing produces output that includes subcase_name in the FAIL: message:

--- FAIL: TestEven/test_even (0.00s)

The output of these tests causes ParseError() to misbehave in interesting ways.

Behavior

On the command line:

➜  go test .
--- FAIL: TestEven (0.00s)
    --- FAIL: TestEven/sub-case_a (0.00s)
        foobar_test.go:8: this is an error
    --- FAIL: TestEven/sub-case_b (0.00s)
        foobar_test.go:8: this is an error
FAIL
FAIL    _/home/ser/test/src/github.com/foobar   0.002s

Note that testing.T.Run() is adding the test name to the FAIL message.

ParseError() adds the context for every error except for the first, including when there is only a single error. It may be undesirable that the context is added in the first case, but exclusion of the test name metadata makes errors untraceable. Copied from within nvim:

foobar_test.go|8| this is an error
||     --- FAIL: TestEven/sub-case_b (0.00s)
foobar_test.go|8| this is an error

If there are many sub-cases, it is not possible to tell which sub-case caused the first failure -- including when only a single sub-case fails.

Ideally, ParseErrors() would:

  1. preserve the test name (e.g., "sub-case_b")
  2. strip the FAIL messages (as it does with common test error messages)
  3. include the name in the errors. E.g.:
    foobar_test.go|8|sub-case_b this is an error

Steps to reproduce:

  1. Copy this into a file in an empty directory:
package foobar
import "testing"
func TestEven(t *testing.T) {
   for _, v := range []string{"a", "b"} {
      t.Run("sub-case "+v, func(t *testing.T) {
         t.Errorf("this is an error")
      })
   }
}
  1. Open this in vim
  2. Run :GoTest

Configuration

I can provide if necessary; however, I'm pretty sure this is because ParseErrors() was not written with this use case of testing in mind.

@fatih
Copy link
Owner

fatih commented Jun 1, 2017

The issue is that the line number is always the same for all sub tests. It'll not change. So I'm not sure how helpful the fix will be. For example if I add more strings to the test table it'll be like:

➜  demo go test
--- FAIL: TestEven (0.00s)
    --- FAIL: TestEven/sub-case_a (0.00s)
    	demo_test.go:8: this is an error
    --- FAIL: TestEven/sub-case_b (0.00s)
    	demo_test.go:8: this is an error
    --- FAIL: TestEven/sub-case_c (0.00s)
    	demo_test.go:8: this is an error
    --- FAIL: TestEven/sub-case_d (0.00s)
    	demo_test.go:8: this is an error
FAIL
exit status 1
FAIL	demo	0.006s

They are all on the same line and therefore quickfix will not jump between these cases. I assume in your case each sub case has a different error message, so you wan't to see which case fails, rather then jumping to the error itself ?

@fatih
Copy link
Owner

fatih commented Jun 1, 2017

So I've looked into this more, but it's not easy and it's a limitation of the go test command. For example let's assume we have the following file:

package m

import "testing"

func TestEven(t *testing.T) {
	for _, v := range []string{"a", "b"} {
		t.Run("sub-case "+v, func(t *testing.T) {
			t.Errorf("this is a sub error")
			t.Errorf("this is a sub error 2")
		})
	}

	t.Errorf("this is normal error")
}

Here if you run go test you'll get :

--- FAIL: TestEven (0.00s)
    --- FAIL: TestEven/sub-case_a (0.00s)
    	demo_test.go:8: this is a sub error
    	demo_test.go:9: this is a sub error 2
    --- FAIL: TestEven/sub-case_b (0.00s)
    	demo_test.go:8: this is a sub error
    	demo_test.go:9: this is a sub error 2
	demo_test.go:13: this is normal error
FAIL
exit status 1
FAIL	demo	0.007s

So now a question. How are you going to distinguish the line demo_test.go:13: this is normal error ? How can you be sure it depends on the sub-case_b or not? In our example that is not the case unfortunately. I have a fix already that prepends the test case, but for the above file we get the following output:

screen shot 2017-06-02 at 2 04 01 am

Here you can clearly see that the line demo_test.go:13: this is normal error is treated as pat of the sub test b.

I'm not sure how to fix without changing the way go test outputs the output for us.

@fatih
Copy link
Owner

fatih commented Jun 1, 2017

An improvement would be this proposal which is not implemented yet. It would solve a lot of our problems: golang/go#2981

@tomnomnom
Copy link

tomnomnom commented Jun 1, 2017

I've posted this on twitter already but will post here too for visibility.

It looks like the sub errors are prefixed with 4 spaces and a tab, whereas the normal errors are prefixed with just a tab. My terminal seems to line the text up, but when viewing it in a vim buffer with :set list it becomes clear.

screenshot from 2017-06-02 00-26-58

It might be a fairly hacky way to tell the difference, but it's at least possible.

@bhcleek bhcleek self-assigned this Nov 21, 2017
bhcleek added a commit to bhcleek/vim-go that referenced this issue Nov 21, 2017
bhcleek added a commit to bhcleek/vim-go that referenced this issue Nov 21, 2017
Add an option to prepend the test name to test errors and log messages.

Fixes fatih#1262
bhcleek added a commit to bhcleek/vim-go that referenced this issue Nov 21, 2017
Add an option to prepend the test name to test errors and log messages.

Fixes fatih#1262
bhcleek added a commit to bhcleek/vim-go that referenced this issue Nov 21, 2017
Add an option to prepend the test name to test errors and log messages.

Fixes fatih#1262
arp242 pushed a commit to bhcleek/vim-go that referenced this issue Nov 23, 2017
Add an option to prepend the test name to test errors and log messages.

Fixes fatih#1262
arp242 pushed a commit that referenced this issue Nov 23, 2017
prepend test name to test errors

Add an option to prepend the test name to test errors and log messages.

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

No branches or pull requests

3 participants