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

pass testing.T explicitly to the anonymous func #414

Closed
wants to merge 1 commit into from

Conversation

hidetatz
Copy link

@hidetatz hidetatz commented Jan 14, 2022

What does this pull request do?

Passes testing.T explicitly to the anonymous function run as a goroutine. By this change, go vet succeeds.
Fixes #412

Where should the reviewer start?

Read #412 to understand the problem and make sure testing.T is correctly used.

How should this be manually tested?

Run go vet locally.

@hidetatz hidetatz marked this pull request as ready for review January 14, 2022 13:46
Copy link
Member

@shogo82148 shogo82148 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, but it just deceives go vet and is not an essential solution.
We need to wait for all the goroutines to finish before calling (*T).Fatalf.

@nathany
Copy link
Contributor

nathany commented Jan 14, 2022

I did see some example solutions when I searched the Internet for the vet error... there's a better way to do it, I'm certain.

Little point in running go vet if we're going to trick it.

@nathany nathany closed this Jan 14, 2022
@hidetatz
Copy link
Author

Does it make sense to use errgroup? I'm writing patch like this:

diff --git a/inotify_poller_test.go b/inotify_poller_test.go
index 0570057..80a2bb5 100644
--- a/inotify_poller_test.go
+++ b/inotify_poller_test.go
@@ -11,6 +11,7 @@ import (
 	"testing"
 	"time"
 
+	"golang.org/x/sync/errgroup"
 	"golang.org/x/sys/unix"
 )
 
@@ -174,19 +175,23 @@ func TestPollerConcurrent(t *testing.T) {
 	oks := make(chan bool)
 	live := make(chan bool)
 	defer close(live)
-	go func(tb testing.TB) {
+	eg := errgroup.Group{}
+	eg.Go(func() error {
 		defer close(oks)
 		for {
 			ok, err := poller.wait()
 			if err != nil {
-				tb.Fatalf("poller failed: %v", err)
+				return err
 			}
 			oks <- ok
 			if !<-live {
-				return
+				return nil
 			}
 		}
-	}(t)
+	})
+	if err := eg.Wait(); err != nil {
+		t.Fatalf("poller failed: %v", err)
+	}
 
 	// Try a write
 	select {

@nathany
Copy link
Contributor

nathany commented Jan 15, 2022

That looks like a very nice solution

It does mean adding another dependency, but only for tests. 🤔

I'm not sure what other people think, but if you're willing to open a PR for that, it'd be a good start. There are multiple platforms where the vet error occurs.

@hidetatz
Copy link
Author

Another way is using sync.WaitGroup instead of errgroup if introducing another dependency is unacceptable, but using errgroup is clearer way for me.
OK, I'll open another PR.

There are multiple platforms where the vet error occurs.

Got it, I think something like GOOS=windows GOARCH=amd64 go vet ./... will work.

@nathany
Copy link
Contributor

nathany commented Jan 15, 2022

👍🏼 I'll let you decide -- if you can make sync.WaitGroup work nicely (maybe adding a helper function or what not), that may be preferred.

Kind've need a way to trigger the error case to make sure it all works, because normally Fatalf shouldn't be called. :-)

@shogo82148
Copy link
Member

yeah, errgroup is a good choice.
however I think sync.WaitGroup is enough to resolve it.

Here is my patch: #416

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

Successfully merging this pull request may close these issues.

go vet: call to (*T).Fatalf from a non-test goroutine
3 participants