Support resolving test_suite helpers from other targets

This only works if there was no non-test package providing the same
package.

We don't automatically set the visibility for the depended-on code -
this is an explicit decision that should be made by a human as to
whether that dependency should be allowed.

java/gazelle/README.md

@@ -79,7 +79,7 @@ Currently, the gazelle plugin makes the following assumptions about the code it'
 1. Packages only exist in one place. Two different directories or dependencies may not contain classes which belong in the same package. The exception to this is that for each package, there may be a single test directory which uses the same package as that package's non-test directory.
 1. There are no circular dependencies that extend beyond a single package. If these are present, and can't easily be removed, you may want to set `# gazelle:java_module_granularity module` in the BUILD file containing the parent-most class in the dependency cycle, which may fix the problem, but will slow down your builds. Ideally, remove dependency cycles.
 1. Non-test code doesn't depend on test code.
-1. Non-test code used by one package of tests either lives in the same directory as those tests, or lives in a non-test-code directory.
+1. Non-test code used by one package of tests either lives in the same directory as those tests, or lives in a non-test-code directory. We also detect non-test code used from another test package, if that other package doesn't have a corresponding non-test code directory, but require you to manually set the visibility on the depended-on target, because this is an unexpected set-up.
 
 If these assumptions are violated, the rest of the generation should still function properly, but the specific files which violate the assumptions (or depend on files which violate the assumptions) will not get complete results. We strive to emit warnings when this happens.
 

java/gazelle/generate.go

@@ -97,7 +97,7 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes
 		rjl.SetAttr("exports", append(exports, ":"+jplName))
 		packageName := types.NewPackageName(protoPackage.Options["java_package"])
 		log.Debug().Str("pkg", packageName.Name).Msg("adding the proto import statement")
-		rjl.SetPrivateAttr(packagesKey, []types.ResolvableJavaPackage{types.NewResolvableJavaPackage(packageName, false)})
+		rjl.SetPrivateAttr(packagesKey, []types.ResolvableJavaPackage{*types.NewResolvableJavaPackage(packageName, false, false)})
 		res.Gen = append(res.Gen, rjl)
 		res.Imports = append(res.Imports, types.ResolveInput{
 			PackageNames: sorted_set.NewSortedSetFn([]types.PackageName{packageName}, types.PackageNameLess),
@@ -289,6 +289,7 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes
 					packageNames,
 					testJavaImportsWithHelpers,
 					cfg.GetCustomJavaTestFileSuffixes(),
+					testHelperJavaFiles.Len() > 0,
 					&res,
 				)
 			}
@@ -412,7 +413,7 @@ func (l javaLang) generateJavaLibrary(file *rule.File, pathToPackageRelativeToBa
 
 	resolvablePackages := make([]types.ResolvableJavaPackage, 0, packages.Len())
 	for _, pkg := range packages.SortedSlice() {
-		resolvablePackages = append(resolvablePackages, types.NewResolvableJavaPackage(pkg, testonly))
+		resolvablePackages = append(resolvablePackages, *types.NewResolvableJavaPackage(pkg, testonly, false))
 	}
 	r.SetPrivateAttr(packagesKey, resolvablePackages)
 	res.Gen = append(res.Gen, r)
@@ -470,7 +471,7 @@ func (l javaLang) generateJavaTest(file *rule.File, pathToPackageRelativeToBazel
 	path := strings.TrimPrefix(f.pathRelativeToBazelWorkspaceRoot, pathToPackageRelativeToBazelWorkspace+"/")
 	r.SetAttr("srcs", []string{path})
 	r.SetAttr("test_class", fullyQualifiedTestClass)
-	r.SetPrivateAttr(packagesKey, []types.ResolvableJavaPackage{types.NewResolvableJavaPackage(f.pkg, true)})
+	r.SetPrivateAttr(packagesKey, []types.ResolvableJavaPackage{*types.NewResolvableJavaPackage(f.pkg, true, false)})
 
 	if runtimeDeps.Len() != 0 {
 		r.SetAttr("runtime_deps", labelsToStrings(runtimeDeps.SortedSlice()))
@@ -514,13 +515,15 @@ var junit5RuntimeDeps = []string{
 	"org.junit.platform:junit-platform-reporting",
 }
 
-func (l javaLang) generateJavaTestSuite(file *rule.File, name string, srcs []string, packageNames, imports *sorted_set.SortedSet[types.PackageName], customTestSuffixes *[]string, res *language.GenerateResult) {
+func (l javaLang) generateJavaTestSuite(file *rule.File, name string, srcs []string, packageNames, imports *sorted_set.SortedSet[types.PackageName], customTestSuffixes *[]string, hasHelpers bool, res *language.GenerateResult) {
 	const ruleKind = "java_test_suite"
 	r := rule.NewRule(ruleKind, name)
 	r.SetAttr("srcs", srcs)
 	resolvablePackages := make([]types.ResolvableJavaPackage, 0, packageNames.Len())
-	for _, packageName := range packageNames.SortedSlice() {
-		resolvablePackages = append(resolvablePackages, types.NewResolvableJavaPackage(packageName, true))
+	if hasHelpers {
+		for _, packageName := range packageNames.SortedSlice() {
+			resolvablePackages = append(resolvablePackages, *types.NewResolvableJavaPackage(packageName, true, true))
+		}
 	}
 	r.SetPrivateAttr(packagesKey, resolvablePackages)
 

java/gazelle/generate_test.go

@@ -227,7 +227,7 @@ func TestSuite(t *testing.T) {
 			var res language.GenerateResult
 
 			l := newTestJavaLang(t)
-			l.generateJavaTestSuite(nil, "blah", []string{src}, stringsToPackageNames([]string{pkg}), stringsToPackageNames(tc.importedPackages), nil, &res)
+			l.generateJavaTestSuite(nil, "blah", []string{src}, stringsToPackageNames([]string{pkg}), stringsToPackageNames(tc.importedPackages), nil, false, &res)
 
 			require.Len(t, res.Gen, 1, "want 1 generated rule")
 

java/gazelle/private/types/types.go

@@ -101,12 +101,14 @@ type ResolveInput struct {
 type ResolvableJavaPackage struct {
 	packageName PackageName
 	isTestOnly  bool
+	isTestSuite bool
 }
 
-func NewResolvableJavaPackage(packageName PackageName, isTestOnly bool) ResolvableJavaPackage {
-	return ResolvableJavaPackage{
+func NewResolvableJavaPackage(packageName PackageName, isTestOnly, isTestSuite bool) *ResolvableJavaPackage {
+	return &ResolvableJavaPackage{
 		packageName: packageName,
 		isTestOnly:  isTestOnly,
+		isTestSuite: isTestSuite,
 	}
 }
 
@@ -119,6 +121,9 @@ func (r *ResolvableJavaPackage) String() string {
 	if r.isTestOnly {
 		s += "!testonly"
 	}
+	if r.isTestSuite {
+		s += "!testsuite"
+	}
 	return s
 }
 
@@ -129,15 +134,19 @@ func ParseResolvableJavaPackage(s string) (*ResolvableJavaPackage, error) {
 	}
 	packageName := NewPackageName(parts[0])
 	isTestOnly := false
+	isTestSuite := false
 	for _, part := range parts[1:] {
 		if part == "testonly" {
 			isTestOnly = true
+		} else if part == "testsuite" {
+			isTestSuite = true
 		} else {
 			return nil, fmt.Errorf("saw unrecognized tag %s", part)
 		}
 	}
 	return &ResolvableJavaPackage{
 		packageName: packageName,
 		isTestOnly:  isTestOnly,
+		isTestSuite: isTestSuite,
 	}, nil
 }

java/gazelle/resolve.go

@@ -46,18 +46,18 @@ func (Resolver) Name() string {
 func (jr Resolver) Imports(c *config.Config, r *rule.Rule, f *rule.File) []resolve.ImportSpec {
 	log := jr.lang.logger.With().Str("step", "Imports").Str("rel", f.Pkg).Str("rule", r.Name()).Logger()
 
-	if !isJavaLibrary(r.Kind()) {
+	if !isJavaLibrary(r.Kind()) && r.Kind() != "java_test_suite" {
 		return nil
 	}
 
 	var out []resolve.ImportSpec
 	if pkgs := r.PrivateAttr(packagesKey); pkgs != nil {
 		for _, pkg := range pkgs.([]types.ResolvableJavaPackage) {
-			out = append(out, resolve.ImportSpec{Lang: languageName, Imp: pkg.PackageName().Name})
+			out = append(out, resolve.ImportSpec{Lang: languageName, Imp: pkg.String()})
 		}
 	}
 
-	log.Debug().Str("out", fmt.Sprintf("%#v", out)).Msg("return")
+	log.Debug().Str("out", fmt.Sprintf("%#v", out)).Str("label", label.New("", f.Pkg, r.Name()).String()).Msg("return")
 	return out
 }
 
@@ -149,7 +149,8 @@ func simplifyLabel(repoName string, l label.Label, from label.Label) label.Label
 }
 
 func (jr *Resolver) convertImport(c *config.Config, imp types.PackageName, ix *resolve.RuleIndex, from label.Label, isTestRule bool, ownPackageNames *sorted_set.SortedSet[types.PackageName]) (out label.Label, err error) {
-	importSpec := resolve.ImportSpec{Lang: languageName, Imp: imp.Name}
+	cacheKey := types.NewResolvableJavaPackage(imp, false, false)
+	importSpec := resolve.ImportSpec{Lang: languageName, Imp: cacheKey.String()}
 	if ol, found := resolve.FindRuleWithOverride(c, importSpec, languageName); found {
 		return ol, nil
 	}
@@ -172,15 +173,15 @@ func (jr *Resolver) convertImport(c *config.Config, imp types.PackageName, ix *r
 			Msg("convertImport found MULTIPLE results in rule index")
 	}
 
-	if v, ok := jr.internalCache.Get(imp); ok {
-		return v.(label.Label), nil
+	if v, ok := jr.internalCache.Get(cacheKey); ok {
+		return simplifyLabel(c.RepoName, v.(label.Label), from), nil
 	}
 
 	jr.lang.logger.Debug().Str("parsedImport", imp.Name).Stringer("from", from).Msg("not found yet")
 
 	defer func() {
-		if err == nil {
-			jr.internalCache.Add(imp, out)
+		if err == nil && out != label.NoLabel {
+			jr.internalCache.Add(cacheKey, out)
 		}
 	}()
 
@@ -192,6 +193,30 @@ func (jr *Resolver) convertImport(c *config.Config, imp types.PackageName, ix *r
 		return l, nil
 	}
 
+	if isTestRule {
+		// If there's exactly one testonly match, use it
+		testonlyCacheKey := types.NewResolvableJavaPackage(imp, true, false)
+		testonlyImportSpec := resolve.ImportSpec{Lang: languageName, Imp: testonlyCacheKey.String()}
+		testonlyMatches := ix.FindRulesByImportWithConfig(c, testonlyImportSpec, languageName)
+		if len(testonlyMatches) == 1 {
+			cacheKey = testonlyCacheKey
+			return simplifyLabel(c.RepoName, testonlyMatches[0].Label, from), nil
+		}
+
+		// If there's exactly one testonly match, use it
+		testsuiteCacheKey := types.NewResolvableJavaPackage(imp, true, true)
+		testsuiteImportSpec := resolve.ImportSpec{Lang: languageName, Imp: testsuiteCacheKey.String()}
+		testsuiteMatches := ix.FindRulesByImportWithConfig(c, testsuiteImportSpec, languageName)
+		if len(testsuiteMatches) == 1 {
+			cacheKey = testsuiteCacheKey
+			l := testsuiteMatches[0].Label
+			if l != from {
+				l.Name += "-test-lib"
+				return simplifyLabel(c.RepoName, l, from), nil
+			}
+		}
+	}
+
 	if isTestRule && ownPackageNames.Contains(imp) {
 		// Tests may have unique packages which don't exist outside of those tests - don't treat this as an error.
 		return label.NoLabel, nil

java/gazelle/testdata/custom_test_suffixes/expectedStderr.txt

@@ -1,2 +1,4 @@
 12:00AM WRN java/gazelle/private/maven/resolver.go:XXX > not loading maven dependencies error="open %WORKSPACEPATH%/maven_install.json: no such file or directory" _c=maven-resolver
 12:00AM WRN java/gazelle/resolve.go:XXX > Unable to find package for import in any dependency from rule=//src/test/com/example/child/grandchild package=org.junit
+12:00AM WRN java/gazelle/resolve.go:XXX > Unable to find package for import in any dependency from rule=//src/test/com/example/child package=org.junit
+12:00AM WRN java/gazelle/resolve.go:XXX > Unable to find package for import in any dependency from rule=//src/test/com/example package=org.junit

java/gazelle/testdata/java_test_per_file/src/test/java/com/example/myproject/AppTest.java

@@ -2,13 +2,14 @@
 
 import static org.junit.Assert.assertEquals;
 
+import com.example.justhelpers.Helper;
 import org.junit.Test;
 
 public class AppTest {
 
   @Test
   public void testCompare() throws Exception {
     App app = new App();
-    assertEquals("should return 0 when both numbers are equal", 0, app.compare(1, 1));
+    assertEquals("should return 0 when both numbers are equal", 0, app.compare(Helper.powerOfOne(1), 1));
   }
 }

java/gazelle/testdata/java_test_per_file/src/test/java/com/example/myproject/BUILD.out

@@ -6,6 +6,7 @@ java_test(
     test_class = "com.example.myproject.AppTest",
     deps = [
         "//src/main/java/com/example/myproject",
+        "//src/test/java/com/example/justhelpers",
         "@maven//:junit_junit",
         "@maven//:org_hamcrest_hamcrest_all",
     ],
@@ -17,6 +18,7 @@ java_test(
     test_class = "com.example.myproject.OtherAppTest",
     deps = [
         "//src/main/java/com/example/myproject",
+        "//src/test/java/com/example/justhelpers",
         "@maven//:junit_junit",
         "@maven//:org_hamcrest_hamcrest_all",
     ],

java/gazelle/testdata/module-granularity/module1/BUILD.out

@@ -34,6 +34,7 @@ java_test_suite(
     ],
     deps = [
         ":module1",
+        "//testmodule/src/test/java/com/example/hello/notworld/justhelpersinmodule:justhelpersinmodule-tests-test-lib",
         "@maven//:org_hamcrest_hamcrest_all",
         "@maven//:org_junit_jupiter_junit_jupiter_api",
     ],

java/gazelle/testdata/module-granularity/module1/src/test/java/com/example/hello/world/WorldTest.java

@@ -2,6 +2,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import com.example.hello.notworld.justhelpersinmodule.Helper;
 import org.junit.jupiter.api.Test;
 
 public class WorldTest {
@@ -10,4 +11,9 @@ public class WorldTest {
     public void testWorld() {
         assertEquals("", "World", World.World());
     }
+
+    @Test
+    public void testNotWorld() {
+        assertEquals("Not World!", Helper.getExpectation());
+    }
 }

java/gazelle/testdata/module-granularity/testmodule/src/test/java/com/example/hello/notworld/justhelpers/BUILD.out → java/gazelle/testdata/module-granularity/testmodule/src/test/java/com/example/hello/notworld/justhelpersinmodule/BUILD.out

@@ -3,7 +3,7 @@ load("@contrib_rules_jvm//java:defs.bzl", "java_test_suite")
 # gazelle:java_module_granularity module
 
 java_test_suite(
-    name = "justhelpers-tests",
+    name = "justhelpersinmodule-tests",
     srcs = [
         "Helper.java",
         "withdirectory/Helper.java",

java/gazelle/testdata/module-granularity/testmodule/src/test/java/com/example/hello/notworld/justhelpers/Helper.java → java/gazelle/testdata/module-granularity/testmodule/src/test/java/com/example/hello/notworld/justhelpersinmodule/Helper.java

@@ -1,4 +1,4 @@
-package com.example.hello.notworld.justhelpers;
+package com.example.hello.notworld.justhelpersinmodule;
 
 public class Helper {
   public String getExpectation() {

java/gazelle/testdata/module-granularity/testmodule/src/test/java/com/example/hello/notworld/justhelpers/withdirectory/Helper.java → java/gazelle/testdata/module-granularity/testmodule/src/test/java/com/example/hello/notworld/justhelpersinmodule/withdirectory/Helper.java

@@ -1,4 +1,4 @@
-package com.example.hello.notworld.justhelpers.withdirectory;
+package com.example.hello.notworld.justhelpersinmodule.withdirectory;
 
 public class Helper {
   public String toUpperCase(String input) {