From 046d5bc3336010e094e282712c29405f5762d307 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Wed, 4 Sep 2024 17:09:35 +0200
Subject: [PATCH] Run nogo on internal and external tests libs, not testmain
 (#4082)

**What type of PR is this?**

Bug fix

**What does this PR do? Why is it needed?**

nogo should run on both the internal and external libraries compiled for
a `go_test`, but didn't. It also shouldn't run on the generated
`testmain.go` file, but did.
---
 go/private/rules/test.bzl           |  9 ++++-
 tests/core/nogo/tests/tests_test.go | 59 ++++++++++++++++++++++++++++-
 2 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/go/private/rules/test.bzl b/go/private/rules/test.bzl
index b5db446928..5e0fad37c3 100644
--- a/go/private/rules/test.bzl
+++ b/go/private/rules/test.bzl
@@ -60,10 +60,14 @@ def _go_test_impl(ctx):
 
     go = go_context(ctx)
 
+    validation_outputs = []
+
     # Compile the library to test with internal white box tests
     internal_library = go.new_library(go, testfilter = "exclude")
     internal_source = go.library_to_source(go, ctx.attr, internal_library, ctx.coverage_instrumented())
     internal_archive = go.archive(go, internal_source)
+    if internal_archive.data._validation_output:
+        validation_outputs.append(internal_archive.data._validation_output)
     go_srcs = split_srcs(internal_source.srcs).go
 
     # Compile the library with the external black box tests
@@ -82,6 +86,8 @@ def _go_test_impl(ctx):
     ), external_library, ctx.coverage_instrumented())
     external_source, internal_archive = _recompile_external_deps(go, external_source, internal_archive, [t.label for t in ctx.attr.embed])
     external_archive = go.archive(go, external_source, is_external_pkg = True)
+    if external_archive.data._validation_output:
+        validation_outputs.append(external_archive.data._validation_output)
 
     # now generate the main function
     repo_relative_rundir = ctx.attr.rundir or ctx.label.package or "."
@@ -165,7 +171,6 @@ def _go_test_impl(ctx):
         version_file = ctx.version_file,
         info_file = ctx.info_file,
     )
-    validation_output = test_archive.data._validation_output
 
     env = {}
     for k, v in ctx.attr.env.items():
@@ -187,7 +192,7 @@ def _go_test_impl(ctx):
         ),
         OutputGroupInfo(
             compilation_outputs = [internal_archive.data.file],
-            _validation = [validation_output] if validation_output else [],
+            _validation = validation_outputs,
         ),
         coverage_common.instrumented_files_info(
             ctx,
diff --git a/tests/core/nogo/tests/tests_test.go b/tests/core/nogo/tests/tests_test.go
index ed19dd41e2..801388b022 100644
--- a/tests/core/nogo/tests/tests_test.go
+++ b/tests/core/nogo/tests/tests_test.go
@@ -15,6 +15,7 @@
 package importpath_test
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
@@ -45,6 +46,18 @@ go_test(
     srcs = ["super_simple_test.go"],
 )
 
+go_test(
+    name = "diagnostic_external_test",
+    size = "small",
+    srcs = ["diagnostic_external_test.go"],
+)
+
+go_test(
+    name = "diagnostic_internal_test",
+    size = "small",
+    srcs = ["diagnostic_internal_test.go"],
+)
+
 nogo(
     name = "nogo",
     vet = True,
@@ -75,21 +88,63 @@ import (
 
 func TestFoo(t *testing.T) {
 }
+-- diagnostic_external_test.go --
+package diagnostic_test
+
+import (
+	"testing"
+)
+
+func TestFoo(t *testing.T) {
+    if TestFoo == nil {
+		t.Fatal("TestFoo is nil")
+    }
+}
+-- diagnostic_internal_test.go --
+package diagnostic
+
+import (
+	"testing"
+)
+
+func TestFoo(t *testing.T) {
+    if TestFoo == nil {
+		t.Fatal("TestFoo is nil")
+    }
+}
 `,
 		Nogo: `@//:nogo`,
 	})
 }
 
 func TestExternalTestWithFullImportpath(t *testing.T) {
-	if out, err := bazel_testing.BazelOutput("test", "//:all"); err != nil {
+	if out, err := bazel_testing.BazelOutput("test", "//:simple_test"); err != nil {
 		println(string(out))
 		t.Fatal(err)
 	}
 }
 
 func TestEmptyExternalTest(t *testing.T) {
-	if out, err := bazel_testing.BazelOutput("test", "//:all"); err != nil {
+	if out, err := bazel_testing.BazelOutput("test", "//:super_simple_test"); err != nil {
 		println(string(out))
 		t.Fatal(err)
 	}
 }
+
+func TestDiagnosticInExternalTest(t *testing.T) {
+	if _, err := bazel_testing.BazelOutput("test", "//:diagnostic_external_test"); err == nil {
+		t.Fatal("unexpected success")
+	} else if !strings.Contains(err.Error(), "diagnostic_external_test.go:8:8: comparison of function TestFoo == nil is always false (nilfunc)") {
+		println(err.Error())
+		t.Fatal("unexpected output")
+	}
+}
+
+func TestDiagnosticInInternalTest(t *testing.T) {
+	if _, err := bazel_testing.BazelOutput("test", "//:diagnostic_internal_test"); err == nil {
+		t.Fatal("unexpected success")
+	} else if !strings.Contains(err.Error(), "diagnostic_internal_test.go:8:8: comparison of function TestFoo == nil is always false (nilfunc)") {
+		println(err.Error())
+		t.Fatal("unexpected output")
+	}
+}