feat!: Following generation mode when generating test targets (#2044)

When `python_generation_mode` is `project` or `file` , the generated
`py_test` targets are consistent with `py_library`. However, when
`python_generation_mode` is `package`, it becomes inconsistent: it
requires either `__test__` target or `__test__.py` file to generate
`py_test` in package mode, otherwise it will fall back to file mode.

This PR relaxes this requirement with a new directive
`gazelle:python_generation_mode_per_package_require_test_entry_point`.
Whent it's set to false, Gazelle and generates one `py_test` target per
package in package mode even without entry points.

This allows people to use `gazelle:map_kind` to map `py_test` to a macro
that sets a default test runner, such as
[rules_python_pytest](https://github.com/caseyduquettesc/rules_python_pytest)
or [pytest-bazel](https://pypi.org/project/pytest-bazel/), and generate
one test target per package. The behavior when
`gazelle:python_generation_mode` is "file" or "project" remains the
same.

This fixes #1972 for supporting pytest from Gazelle. With this approach,
people can define a thin macro like this to use py_pytest_main:

```
load("@aspect_rules_py//py:defs.bzl", "py_pytest_main")

def py_test(name, **kwargs):
    py_pytest_main(
        name = "__test__",
        deps = ["@pip_pytest//:pkg"],  # change this to the pytest target in your repo.
    )

    deps = kwargs.pop("deps", [])
    deps.append(":__test__")

    py_test(
        name = name,
        main = ":__test__.py",
        deps = deps,
        **kwargs,
)
```

BREAKING CHANGES:
Without `gazelle:map_kind` or `__test__` target or `__test__.py`, the
package mode will now generate `py_test` without `main` attribute, which
may not be runnable. However, this is already an issue with
"python_generation_mode:project" before this PR.

The default value of
`gazelle:python_generation_mode_per_package_require_test_entry_point` is
true to preserve the current behavior. We will flip that default value
in the future.

---------

Co-authored-by: aignas <[email protected]>

CHANGELOG.md

@@ -39,7 +39,16 @@ A brief description of the categories of changes:
   containing ">" sign
 
 ### Added
-* Nothing yet
+* (gazelle) Added `python_generation_mode_per_package_require_test_entry_point`
+  in order to better accommodate users who use a custom macro,
+  [`pytest-bazel`][pytest_bazel], [rules_python_pytest] or `rules_py`
+  [py_test_main] in order to integrate with `pytest`. Currently the default
+  flag value is set to `true` for backwards compatible behaviour, but in the
+  future the flag will be flipped be `false` by default.
+
+[rules_python_pytest]: https://github.com/caseyduquettesc/rules_python_pytest
+[py_test_main]: https://docs.aspect.build/rulesets/aspect_rules_py/docs/rules/#py_pytest_main
+[pytest_bazel]: https://pypi.org/project/pytest-bazel
 
 ### Removed
 * Nothing yet

gazelle/python/configure.go

@@ -61,6 +61,7 @@ func (py *Configurer) KnownDirectives() []string {
 		pythonconfig.ValidateImportStatementsDirective,
 		pythonconfig.GenerationMode,
 		pythonconfig.GenerationModePerFileIncludeInit,
+		pythonconfig.GenerationModePerPackageRequireTestEntryPoint,
 		pythonconfig.LibraryNamingConvention,
 		pythonconfig.BinaryNamingConvention,
 		pythonconfig.TestNamingConvention,
@@ -163,6 +164,14 @@ func (py *Configurer) Configure(c *config.Config, rel string, f *rule.File) {
 				log.Fatal(err)
 			}
 			config.SetPerFileGenerationIncludeInit(v)
+		case pythonconfig.GenerationModePerPackageRequireTestEntryPoint:
+			v, err := strconv.ParseBool(strings.TrimSpace(d.Value))
+			if err != nil {
+				log.Printf("invalid value for gazelle:%s in %q: %q",
+					pythonconfig.GenerationModePerPackageRequireTestEntryPoint, rel, d.Value)
+			} else {
+				config.SetPerPackageGenerationRequireTestEntryPoint(v)
+			}
 		case pythonconfig.LibraryNamingConvention:
 			config.SetLibraryNamingConvention(strings.TrimSpace(d.Value))
 		case pythonconfig.BinaryNamingConvention:

gazelle/python/generate.go

@@ -421,7 +421,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			addResolvedDependencies(annotations.includeDeps).
 			generateImportsAttribute()
 	}
-	if (hasPyTestEntryPointFile || hasPyTestEntryPointTarget || cfg.CoarseGrainedGeneration()) && !cfg.PerFileGeneration() {
+	if (!cfg.PerPackageGenerationRequireTestEntryPoint() || hasPyTestEntryPointFile || hasPyTestEntryPointTarget || cfg.CoarseGrainedGeneration()) && !cfg.PerFileGeneration() {
 		// Create one py_test target per package
 		if hasPyTestEntryPointFile {
 			// Only add the pyTestEntrypointFilename to the pyTestFilenames if
@@ -441,7 +441,10 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 					setMain(main)
 			} else if hasPyTestEntryPointFile {
 				pyTestTarget.setMain(pyTestEntrypointFilename)
-			}
+			} /* else:
+			main is not set, assuming there is a test file with the same name
+			as the target name, or there is a macro wrapping py_test and setting its main attribute.
+			*/
 			pyTestTargets = append(pyTestTargets, pyTestTarget)
 		}
 	} else {

gazelle/python/testdata/per_package_test_target_without_entry_point/BUILD.in

@@ -0,0 +1,2 @@
+# gazelle:python_generation_mode package
+# gazelle:python_generation_mode_per_package_require_test_entry_point false

gazelle/python/testdata/per_package_test_target_without_entry_point/BUILD.out

@@ -0,0 +1,18 @@
+load("@rules_python//python:defs.bzl", "py_library", "py_test")
+
+# gazelle:python_generation_mode package
+# gazelle:python_generation_mode_per_package_require_test_entry_point false
+
+py_library(
+    name = "per_package_test_target_without_entry_point",
+    srcs = ["__init__.py"],
+    visibility = ["//:__subpackages__"],
+)
+
+py_test(
+    name = "per_package_test_target_without_entry_point_test",
+    srcs = [
+        "bar_test.py",
+        "foo_test.py",
+    ],
+)

gazelle/python/testdata/per_package_test_target_without_entry_point/README.md

@@ -0,0 +1,3 @@
+# One test target per package without entry point
+
+This test case asserts that one test target is generated per package without entry point when `gazelle:python_generation_mode_per_package_require_test_entry_point false`

gazelle/python/testdata/per_package_test_target_without_entry_point/WORKSPACE

@@ -0,0 +1 @@
+# This is a Bazel workspace for the Gazelle test data.

gazelle/python/testdata/per_package_test_target_without_entry_point/bar_test.py

@@ -0,0 +1,24 @@
+# Copyright 2023 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import unittest
+
+
+class BarTest(unittest.TestCase):
+    def test_foo(self):
+        pass
+
+
+if __name__ == "__main__":
+    unittest.main()

gazelle/python/testdata/per_package_test_target_without_entry_point/foo_test.py

@@ -0,0 +1,24 @@
+# Copyright 2023 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import unittest
+
+
+class FooTest(unittest.TestCase):
+    def test_foo(self):
+        pass
+
+
+if __name__ == "__main__":
+    unittest.main()

gazelle/python/testdata/per_package_test_target_without_entry_point/test.yaml

@@ -0,0 +1,17 @@
+# Copyright 2023 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+---
+expect:
+  exit_code: 0

gazelle/pythonconfig/pythonconfig.go

@@ -55,6 +55,10 @@ const (
 	// the "per_file" GenerationMode by including the package's __init__.py file.
 	// This is a boolean directive.
 	GenerationModePerFileIncludeInit = "python_generation_mode_per_file_include_init"
+	// GenerationModePerPackageRequireTestEntryPoint represents the directive that
+	// requires a test entry point to generate test targets in "package" GenerationMode.
+	// This is a boolean directive.
+	GenerationModePerPackageRequireTestEntryPoint = "python_generation_mode_per_package_require_test_entry_point"
 	// LibraryNamingConvention represents the directive that controls the
 	// py_library naming convention. It interpolates $package_name$ with the
 	// Bazel package name. E.g. if the Bazel package name is `foo`, setting this
@@ -148,21 +152,22 @@ type Config struct {
 	pythonProjectRoot string
 	gazelleManifest   *manifest.Manifest
 
-	excludedPatterns             *singlylinkedlist.List
-	ignoreFiles                  map[string]struct{}
-	ignoreDependencies           map[string]struct{}
-	validateImportStatements     bool
-	coarseGrainedGeneration      bool
-	perFileGeneration            bool
-	perFileGenerationIncludeInit bool
-	libraryNamingConvention      string
-	binaryNamingConvention       string
-	testNamingConvention         string
-	defaultVisibility            []string
-	visibility                   []string
-	testFilePattern              []string
-	labelConvention              string
-	labelNormalization           LabelNormalizationType
+	excludedPatterns                          *singlylinkedlist.List
+	ignoreFiles                               map[string]struct{}
+	ignoreDependencies                        map[string]struct{}
+	validateImportStatements                  bool
+	coarseGrainedGeneration                   bool
+	perFileGeneration                         bool
+	perFileGenerationIncludeInit              bool
+	perPackageGenerationRequireTestEntryPoint bool
+	libraryNamingConvention                   string
+	binaryNamingConvention                    string
+	testNamingConvention                      string
+	defaultVisibility                         []string
+	visibility                                []string
+	testFilePattern                           []string
+	labelConvention                           string
+	labelNormalization                        LabelNormalizationType
 }
 
 type LabelNormalizationType int
@@ -179,24 +184,25 @@ func New(
 	pythonProjectRoot string,
 ) *Config {
 	return &Config{
-		extensionEnabled:             true,
-		repoRoot:                     repoRoot,
-		pythonProjectRoot:            pythonProjectRoot,
-		excludedPatterns:             singlylinkedlist.New(),
-		ignoreFiles:                  make(map[string]struct{}),
-		ignoreDependencies:           make(map[string]struct{}),
-		validateImportStatements:     true,
-		coarseGrainedGeneration:      false,
-		perFileGeneration:            false,
-		perFileGenerationIncludeInit: false,
-		libraryNamingConvention:      packageNameNamingConventionSubstitution,
-		binaryNamingConvention:       fmt.Sprintf("%s_bin", packageNameNamingConventionSubstitution),
-		testNamingConvention:         fmt.Sprintf("%s_test", packageNameNamingConventionSubstitution),
-		defaultVisibility:            []string{fmt.Sprintf(DefaultVisibilityFmtString, "")},
-		visibility:                   []string{},
-		testFilePattern:              strings.Split(DefaultTestFilePatternString, ","),
-		labelConvention:              DefaultLabelConvention,
-		labelNormalization:           DefaultLabelNormalizationType,
+		extensionEnabled:                          true,
+		repoRoot:                                  repoRoot,
+		pythonProjectRoot:                         pythonProjectRoot,
+		excludedPatterns:                          singlylinkedlist.New(),
+		ignoreFiles:                               make(map[string]struct{}),
+		ignoreDependencies:                        make(map[string]struct{}),
+		validateImportStatements:                  true,
+		coarseGrainedGeneration:                   false,
+		perFileGeneration:                         false,
+		perFileGenerationIncludeInit:              false,
+		perPackageGenerationRequireTestEntryPoint: true,
+		libraryNamingConvention:                   packageNameNamingConventionSubstitution,
+		binaryNamingConvention:                    fmt.Sprintf("%s_bin", packageNameNamingConventionSubstitution),
+		testNamingConvention:                      fmt.Sprintf("%s_test", packageNameNamingConventionSubstitution),
+		defaultVisibility:                         []string{fmt.Sprintf(DefaultVisibilityFmtString, "")},
+		visibility:                                []string{},
+		testFilePattern:                           strings.Split(DefaultTestFilePatternString, ","),
+		labelConvention:                           DefaultLabelConvention,
+		labelNormalization:                        DefaultLabelNormalizationType,
 	}
 }
 
@@ -220,14 +226,15 @@ func (c *Config) NewChild() *Config {
 		coarseGrainedGeneration:      c.coarseGrainedGeneration,
 		perFileGeneration:            c.perFileGeneration,
 		perFileGenerationIncludeInit: c.perFileGenerationIncludeInit,
-		libraryNamingConvention:      c.libraryNamingConvention,
-		binaryNamingConvention:       c.binaryNamingConvention,
-		testNamingConvention:         c.testNamingConvention,
-		defaultVisibility:            c.defaultVisibility,
-		visibility:                   c.visibility,
-		testFilePattern:              c.testFilePattern,
-		labelConvention:              c.labelConvention,
-		labelNormalization:           c.labelNormalization,
+		perPackageGenerationRequireTestEntryPoint: c.perPackageGenerationRequireTestEntryPoint,
+		libraryNamingConvention:                   c.libraryNamingConvention,
+		binaryNamingConvention:                    c.binaryNamingConvention,
+		testNamingConvention:                      c.testNamingConvention,
+		defaultVisibility:                         c.defaultVisibility,
+		visibility:                                c.visibility,
+		testFilePattern:                           c.testFilePattern,
+		labelConvention:                           c.labelConvention,
+		labelNormalization:                        c.labelNormalization,
 	}
 }
 
@@ -398,6 +405,14 @@ func (c *Config) PerFileGenerationIncludeInit() bool {
 	return c.perFileGenerationIncludeInit
 }
 
+func (c *Config) SetPerPackageGenerationRequireTestEntryPoint(perPackageGenerationRequireTestEntryPoint bool) {
+	c.perPackageGenerationRequireTestEntryPoint = perPackageGenerationRequireTestEntryPoint
+}
+
+func (c *Config) PerPackageGenerationRequireTestEntryPoint() bool {
+	return c.perPackageGenerationRequireTestEntryPoint
+}
+
 // SetLibraryNamingConvention sets the py_library target naming convention.
 func (c *Config) SetLibraryNamingConvention(libraryNamingConvention string) {
 	c.libraryNamingConvention = libraryNamingConvention
@@ -494,9 +509,9 @@ func (c *Config) FormatThirdPartyDependency(repositoryName string, distributionN
 		normConventionalDistributionName = strings.Trim(normConventionalDistributionName, "_")
 	case Pep503LabelNormalizationType:
 		// See https://packaging.python.org/en/latest/specifications/name-normalization/#name-format
-		normConventionalDistributionName = strings.ToLower(conventionalDistributionName) // ... "should be lowercased"
+		normConventionalDistributionName = strings.ToLower(conventionalDistributionName)                                        // ... "should be lowercased"
 		normConventionalDistributionName = regexp.MustCompile(`[-_.]+`).ReplaceAllString(normConventionalDistributionName, "-") // ... "all runs of the characters ., -, or _ replaced with a single -"
-		normConventionalDistributionName = strings.Trim(normConventionalDistributionName, "-") // ... "must start and end with a letter or number"
+		normConventionalDistributionName = strings.Trim(normConventionalDistributionName, "-")                                  // ... "must start and end with a letter or number"
 	default:
 		fallthrough
 	case NoLabelNormalizationType: