The default sources for conftest.py and *_test.pyi now belong to new target generator python_test_utils, not python_tests

[Eric-Arellano]

Part 1 of #13238.

Why we need to fix this

Currently, python_tests includes conftest.py and *_test.pyi in its sources field, and it generates python_test targets for those. We special case our Pytest code to ignore running tests on those generated targets:

pants/src/python/pants/backend/python/subsystems/pytest.py

Lines 50 to 57 in e865418

	@classmethod
	def opt_out(cls, tgt: Target) -> bool:
	if tgt.get(SkipPythonTestsField).value:
	return True
	# TODO: Replace this by having `python_tests` generate `python_source` targets for these
	# files.
	file_name = PurePath(tgt.address.filename)
	return file_name.name == "conftest.py" or file_name.suffix == ".pyi"

This will not work with lockfiles #13238: conftest.py and *_test.pyi both must be modeled by a python_source target, not python_test. A python_test target has the field resolve: str, whereas python_source has compatible_resolves: list[str]. The resolve says which exact resolve to use, and then we validate the transitive closure is compatible with that one. Because a conftest.py can be a dependency of many python_test targets, it's important that it participates in this compatible_resolves mechanism.

Solution

Add a new python_test_utils target generator, which generates python_source targets. It behaves identically to the python_sources target generator, except that it has the default sources of conftest.py and *_test.pyi.

Why a new target that behaves identically to python_sources? This modeling reduces the risk of folks unintentionally including test support files like conftest.py in production, such as in a pex_binary and python_distribution. For python_distribution, it's very common to add a dependency on a python_sources target generator, rather than a generated python_source target, i.e. :lib instead of foo.py:lib. We want to avoid the gotcha of :lib including conftest.py because that's how the default sources work, and we don't want users to have to be aware of this gotcha and to do set arithmetic in the sources fields of two python_sources to make sure there aren't duplicate owners of the same conftest.py.

A new target also dramatically minimizes the breaking changes of this PR. Now, the risk is that some files will be unowned, whereas before there was the risk that they'd be owned by a different target generator than before, so might inherit different metadata like dependencies and might be included in unexpected places like a python_distribution depending on :lib.

--

This PR does not deprecate the special casing in pytest.py that allows for a python_test target to be used with conftest.py and *_test.pyi. That special casing will be deprecated in a followup.

Alternatives considered

python_tests generates python_source targets

This is technically feasible, but really clunky and confusing to document. For example, the conftest.py python_source target would presumably default to the global compatible_resolves option (configured in [python-setup]). To change it, we'd either need to add a field to python_tests like conftest_compatible_resolves, or the user would have to know to use the overrides mechanism from #13270. Not intuitive!

Deprecating having a default sources field for python_tests and python_sources

Users would have to either explicitly use the new defaults or stick to the old defaults. Once we're confident people aren't using a default, we can safely change it without breaking anyone.

However, this would require a ludicrous amount of churn for a problem that most users don't even have. Even if we automated this change with ./pants update-build-files, it is still extremely disruptive.

Also, this isn't very feasible because of how we validate that each glob in the sources field matches. When it's the field's default, we only require any glob to match. When it's explicitly declared in a BUILD file, we require all globs to match. So, a user could not safely set ["*_test.py", "test_*.py", "tests.py"] in a BUILD file unless they had all those files, which is unlikely.

A global option to change the default

I don't think this is technically feasible. The Target API is intentionally shielded from the Rules API. There's no way for an option to change the default of a field. It would require a ludicrous amount of special casing in engine/internals/graph.py etc to have our Rules code that consumes these fields to change behavior of things like hydrating sources based on the option.

Impact

Our dependency inference rule for conftest.py requires that there be an owning target:

pants/src/python/pants/backend/python/dependency_inference/rules.py

Lines 231 to 235 in a9af04d

	# NB: Because conftest.py files effectively always have content, we require an owning target.
	owners = await MultiGet(
	Get(Owners, OwnersRequest((f,), OwnersNotFoundBehavior.error))
	for f in extra_conftest_files.snapshot.files
	)

So,, unless you have explicitly put conftest.py into a python_sources or python_tests target's sources field, then you'll get this error when upgrading to Pants 2.8:

pants.base.exceptions.ResolveError: No owning targets could be found for the file `src/python/pants/conftest.py`.

Please check that there is a BUILD file in the parent directory src/python/pants with a target whose `sources` field includes the file. See https://www.pantsbuild.org/v2.8/docs/targets for more information on target definitions.

You may want to run `./pants tailor` to autogenerate your BUILD files. See https://www.pantsbuild.org/v2.8/docs/create-initial-build-files.

It violates our deprecation policy to error here, but we don't think there's a way to gracefully deprecate. Hopefully folks will run ./pants tailor after encountering this and get python_test_utils targets added.

When a new python_test_utils target is added, the user may need to update its metadata like its dependencies to work properly. That might not be very intuitive, so we should document it in upgrade notes. Fortunately, the risk is mostly that users' tests will fail: there is far less risk this will break production code.

[ci skip-rust]
[ci skip-build-wheels]

[stuhood]

It seems like we have already put quite a few deprecations in 2.8.x, and this one doesn't seem time sensitive. We'll revisit lockfiles after our current work, but it seems like it would be fine for this deprecation to be in the release that introduces them... especially since being necessary for that feature would help to justify it.

I'm not 100% certain with the conclusion on #12714, so holding off until we're making that push also de-risks things a bit from my perspective.

[Eric-Arellano]

this one doesn't seem time sensitive

I think it's important to fix before we land the target generator vs atom split in 2.8. Once that stabilizes, this PR will be even more disruptive because we're breaking ./pants peek and ./pants filter --target-type.

Note that I used lockfiles in this PR's concrete motivation, whereas in the issue #13238 I use a more conceptual motivation that it is not sound to claim conftest.py is a python_test target. What the heck does the timeout field mean for conftest.py? The modeling is off. We could choose to say "Yeah, but so what?" Hence bringing up lockfiles: that there is a concrete motivation to care about the modeling being wrong.

but it seems like it would be fine for this deprecation to be in the release that introduces them

This isn't a deprecation. This is a breaking change.