Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
The default
sources for conftest.py and *_test.pyi now belong t…
…o new target generator `python_test_utils`, not `python_tests` (#13299) 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: https://github.com/pantsbuild/pants/blob/e8654186c11c87e372bd62ab4f82f39ad2f5b208/src/python/pants/backend/python/subsystems/pytest.py#L50-L57 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: https://github.com/pantsbuild/pants/blob/a9af04dc94de2f718aae5b8ce488faaf8c39c07e/src/python/pants/backend/python/dependency_inference/rules.py#L231-L235 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]
- Loading branch information
1 parent
1c24e52
commit 239b695