feat(gazelle): Add include_pytest_conftest annotation (#3080)

Fixes #3076.

Add a new gazelle annotation `include_pytest_conftest`. When unset
or true, the gazelle behavior is unchanged. When false, gazelle will
*not* inject the `:conftest` dependency to py_test targets.

One of the refactorings that is done to support this is to pass around
an `annotations` struct in `target.targetBuilder`. This will also open
up support for other annotations in the future.

---------

Co-authored-by: Ignas Anikevicius <[email protected]>

Author: dougthor42

CHANGELOG.md

@@ -106,6 +106,11 @@ END_UNRELEASED_TEMPLATE
     * 3.12.11
     * 3.13.5
     * 3.14.0b3
+* (gazelle): New annotation `gazelle:include_pytest_conftest`. When not set (the
+  default) or `true`, gazelle will inject any `conftest.py` file found in the same
+  directory as a {obj}`py_test` target to that {obj}`py_test` target's `deps`.
+  This behavior is unchanged from previous versions. When `false`, the `:conftest`
+  dep is not added to the {obj}`py_test` target.
 * (gazelle) New directive `gazelle:python_generate_proto`; when `true`,
   Gazelle generates `py_proto_library` rules for `proto_library`. `false` by default.
 

gazelle/README.md

@@ -550,6 +550,8 @@ The annotations are:
 | Tells Gazelle to ignore import statements. `imports` is a comma-separated list of imports to ignore. | |
 | [`# gazelle:include_dep targets`](#annotation-include_dep)    | N/A               |
 | Tells Gazelle to include a set of dependencies, even if they are not imported in a Python module. `targets` is a comma-separated list of target names to include as dependencies. | |
+| [`# gazelle:include_pytest_conftest bool`](#annotation-include_pytest_conftest)    | N/A               |
+| Whether or not to include a sibling `:conftest` target in the deps of a `py_test` target. Default behaviour is to include `:conftest`. | |
 
 
 #### Annotation: `ignore`
@@ -622,6 +624,89 @@ deps = [
 ]
 ```
 
+#### Annotation: `include_pytest_conftest`
+
+Added in [#3080][gh3080].
+
+[gh3080]: https://github.com/bazel-contrib/rules_python/pull/3080
+
+This annotation accepts any string that can be parsed by go's
+[`strconv.ParseBool`][ParseBool]. If an unparsable string is passed, the
+annotation is ignored.
+
+[ParseBool]: https://pkg.go.dev/strconv#ParseBool
+
+Starting with [`rules_python` 0.14.0][rules-python-0.14.0] (specifically [PR #879][gh879]),
+Gazelle will include a `:conftest` dependency to an `py_test` target that is in
+the same directory as `conftest.py`.
+
+[rules-python-0.14.0]: https://github.com/bazel-contrib/rules_python/releases/tag/0.14.0
+[gh879]: https://github.com/bazel-contrib/rules_python/pull/879
+
+This annotation allows users to adjust that behavior. To disable the behavior, set
+the annotation value to "false":
+
+```
+# some_file_test.py
+# gazelle:include_pytest_conftest false
+```
+
+Example:
+
+Given a directory tree like:
+
+```
+.
+├── BUILD.bazel
+├── conftest.py
+└── some_file_test.py
+```
+
+The default Gazelle behavior would create:
+
+```starlark
+py_library(
+    name = "conftest",
+    testonly = True,
+    srcs = ["conftest.py"],
+    visibility = ["//:__subpackages__"],
+)
+
+py_test(
+    name = "some_file_test",
+    srcs = ["some_file_test.py"],
+    deps = [":conftest"],
+)
+```
+
+When `# gazelle:include_pytest_conftest false` is found in `some_file_test.py`
+
+```python
+# some_file_test.py
+# gazelle:include_pytest_conftest false
+```
+
+Gazelle will generate:
+
+```starlark
+py_library(
+    name = "conftest",
+    testonly = True,
+    srcs = ["conftest.py"],
+    visibility = ["//:__subpackages__"],
+)
+
+py_test(
+    name = "some_file_test",
+    srcs = ["some_file_test.py"],
+)
+```
+
+See [Issue #3076][gh3076] for more information.
+
+[gh3076]: https://github.com/bazel-contrib/rules_python/issues/3076
+
+
 #### Directive: `experimental_allow_relative_imports`
 Enables experimental support for resolving relative imports in
 `python_generation_mode package`.

gazelle/python/generate.go

@@ -264,7 +264,9 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 					addSrc(filename).
 					addModuleDependencies(mainModules[filename]).
 					addResolvedDependencies(annotations.includeDeps).
-					generateImportsAttribute().build()
+					generateImportsAttribute().
+					setAnnotations(*annotations).
+					build()
 				result.Gen = append(result.Gen, pyBinary)
 				result.Imports = append(result.Imports, pyBinary.PrivateAttr(config.GazelleImportsKey))
 			}
@@ -305,6 +307,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			addModuleDependencies(allDeps).
 			addResolvedDependencies(annotations.includeDeps).
 			generateImportsAttribute().
+			setAnnotations(*annotations).
 			build()
 
 		if pyLibrary.IsEmpty(py.Kinds()[pyLibrary.Kind()]) {
@@ -357,6 +360,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			addSrc(pyBinaryEntrypointFilename).
 			addModuleDependencies(deps).
 			addResolvedDependencies(annotations.includeDeps).
+			setAnnotations(*annotations).
 			generateImportsAttribute()
 
 		pyBinary := pyBinaryTarget.build()
@@ -387,6 +391,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			addSrc(conftestFilename).
 			addModuleDependencies(deps).
 			addResolvedDependencies(annotations.includeDeps).
+			setAnnotations(*annotations).
 			addVisibility(visibility).
 			setTestonly().
 			generateImportsAttribute()
@@ -418,6 +423,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			addSrcs(srcs).
 			addModuleDependencies(deps).
 			addResolvedDependencies(annotations.includeDeps).
+			setAnnotations(*annotations).
 			generateImportsAttribute()
 	}
 	if (!cfg.PerPackageGenerationRequireTestEntryPoint() || hasPyTestEntryPointFile || hasPyTestEntryPointTarget || cfg.CoarseGrainedGeneration()) && !cfg.PerFileGeneration() {
@@ -470,7 +476,14 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 
 	for _, pyTestTarget := range pyTestTargets {
 		if conftest != nil {
-			pyTestTarget.addModuleDependency(Module{Name: strings.TrimSuffix(conftestFilename, ".py")})
+			conftestModule := Module{Name: strings.TrimSuffix(conftestFilename, ".py")}
+			if pyTestTarget.annotations.includePytestConftest == nil {
+				// unset; default behavior
+				pyTestTarget.addModuleDependency(conftestModule)
+			} else if *pyTestTarget.annotations.includePytestConftest {
+				// set; add if true, do not add if false
+				pyTestTarget.addModuleDependency(conftestModule)
+			}
 		}
 		pyTest := pyTestTarget.build()
 

gazelle/python/parser.go

@@ -18,6 +18,8 @@ import (
 	"context"
 	_ "embed"
 	"fmt"
+	"log"
+	"strconv"
 	"strings"
 
 	"github.com/emirpasic/gods/sets/treeset"
@@ -123,6 +125,7 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[strin
 			allAnnotations.ignore[k] = v
 		}
 		allAnnotations.includeDeps = append(allAnnotations.includeDeps, annotations.includeDeps...)
+		allAnnotations.includePytestConftest = annotations.includePytestConftest
 	}
 
 	allAnnotations.includeDeps = removeDupesFromStringTreeSetSlice(allAnnotations.includeDeps)
@@ -183,8 +186,12 @@ const (
 	// The Gazelle annotation prefix.
 	annotationPrefix string = "gazelle:"
 	// The ignore annotation kind. E.g. '# gazelle:ignore <module_name>'.
-	annotationKindIgnore     annotationKind = "ignore"
-	annotationKindIncludeDep annotationKind = "include_dep"
+	annotationKindIgnore annotationKind = "ignore"
+	// Force a particular target to be added to `deps`. Multiple invocations are
+	// accumulated and the value can be comma separated.
+	// Eg: '# gazelle:include_dep //foo/bar:baz,@repo//:target
+	annotationKindIncludeDep            annotationKind = "include_dep"
+	annotationKindIncludePytestConftest annotationKind = "include_pytest_conftest"
 )
 
 // Comment represents a Python comment.
@@ -222,13 +229,18 @@ type annotations struct {
 	ignore map[string]struct{}
 	// Labels that Gazelle should include as deps of the generated target.
 	includeDeps []string
+	// Whether the conftest.py file, found in the same directory as the current
+	// python test file, should be added to the py_test target's `deps` attribute.
+	// A *bool is used so that we can handle the "not set" state.
+	includePytestConftest *bool
 }
 
 // annotationsFromComments returns all the annotations parsed out of the
 // comments of a Python module.
 func annotationsFromComments(comments []Comment) (*annotations, error) {
 	ignore := make(map[string]struct{})
 	includeDeps := []string{}
+	var includePytestConftest *bool
 	for _, comment := range comments {
 		annotation, err := comment.asAnnotation()
 		if err != nil {
@@ -255,11 +267,21 @@ func annotationsFromComments(comments []Comment) (*annotations, error) {
 					includeDeps = append(includeDeps, t)
 				}
 			}
+			if annotation.kind == annotationKindIncludePytestConftest {
+				val := annotation.value
+				parsedVal, err := strconv.ParseBool(val)
+				if err != nil {
+					log.Printf("WARNING: unable to cast %q to bool in %q. Ignoring annotation", val, comment)
+					continue
+				}
+				includePytestConftest = &parsedVal
+			}
 		}
 	}
 	return &annotations{
-		ignore:      ignore,
-		includeDeps: includeDeps,
+		ignore:                ignore,
+		includeDeps:           includeDeps,
+		includePytestConftest: includePytestConftest,
 	}, nil
 }
 

gazelle/python/target.go

@@ -37,6 +37,7 @@ type targetBuilder struct {
 	main              *string
 	imports           []string
 	testonly          bool
+	annotations       *annotations
 }
 
 // newTargetBuilder constructs a new targetBuilder.
@@ -51,6 +52,7 @@ func newTargetBuilder(kind, name, pythonProjectRoot, bzlPackage string, siblingS
 		deps:              treeset.NewWith(moduleComparator),
 		resolvedDeps:      treeset.NewWith(godsutils.StringComparator),
 		visibility:        treeset.NewWith(godsutils.StringComparator),
+		annotations:       new(annotations),
 	}
 }
 
@@ -130,6 +132,13 @@ func (t *targetBuilder) setTestonly() *targetBuilder {
 	return t
 }
 
+// setAnnotations sets the annotations attribute on the target.
+func (t *targetBuilder) setAnnotations(val annotations) *targetBuilder {
+	t.annotations = &val
+	return t
+}
+
+
 // generateImportsAttribute generates the imports attribute.
 // These are a list of import directories to be added to the PYTHONPATH. In our
 // case, the value we add is on Bazel sub-packages to be able to perform imports

gazelle/python/testdata/annotation_include_pytest_conftest/README.md

@@ -0,0 +1,25 @@
+# Annotation: Include Pytest Conftest
+
+Validate that the `# gazelle:include_pytest_conftest` annotation follows
+this logic:
+
++ When a `conftest.py` file does not exist:
+  + all values have no affect
++ When a `conftest.py` file does exist:
+  + Truthy values add `:conftest` to `deps`.
+  + Falsey values do not add `:conftest` to `deps`.
+  + Unset (no annotation) performs the default action.
+
+Additionally, we test that:
+
++ invalid values (eg `foo`) print a warning and then act as if
+  the annotation was not present.
++ last annotation (highest line number) wins.
++ the annotation has no effect on non-test files/targets.
++ the `include_dep` can still inject `:conftest` even when `include_pytest_conftest`
+  is false.
++ `import conftest` will still add the dep even when `include_pytest_conftest` is
+  false.
+
+An annotation without a value is not tested, as that's part of the core
+annotation framework and not specific to this annotation.

gazelle/python/testdata/annotation_include_pytest_conftest/WORKSPACE

@@ -0,0 +1,5 @@
+---
+expect:
+  stderr: |
+    gazelle: WARNING: unable to cast "foo" to bool in "# gazelle:include_pytest_conftest foo". Ignoring annotation
+  exit_code: 0

gazelle/python/testdata/annotation_include_pytest_conftest/test.yaml

@@ -0,0 +1,68 @@
+load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test")
+
+py_binary(
+    name = "binary",
+    srcs = ["binary.py"],
+    visibility = ["//:__subpackages__"],
+)
+
+py_library(
+    name = "with_conftest",
+    srcs = [
+        "binary.py",
+        "library.py",
+    ],
+    visibility = ["//:__subpackages__"],
+)
+
+py_library(
+    name = "conftest",
+    testonly = True,
+    srcs = ["conftest.py"],
+    visibility = ["//:__subpackages__"],
+)
+
+py_test(
+    name = "bad_value_test",
+    srcs = ["bad_value_test.py"],
+    deps = [":conftest"],
+)
+
+py_test(
+    name = "conftest_imported_test",
+    srcs = ["conftest_imported_test.py"],
+    deps = [":conftest"],
+)
+
+py_test(
+    name = "conftest_included_test",
+    srcs = ["conftest_included_test.py"],
+    deps = [":conftest"],
+)
+
+py_test(
+    name = "false_test",
+    srcs = ["false_test.py"],
+)
+
+py_test(
+    name = "falsey_test",
+    srcs = ["falsey_test.py"],
+)
+
+py_test(
+    name = "last_value_wins_test",
+    srcs = ["last_value_wins_test.py"],
+)
+
+py_test(
+    name = "true_test",
+    srcs = ["true_test.py"],
+    deps = [":conftest"],
+)
+
+py_test(
+    name = "unset_test",
+    srcs = ["unset_test.py"],
+    deps = [":conftest"],
+)