From fa14706a39e3faf7d0450bfc8d5f1b564c4eaeeb Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 09:21:07 -0700 Subject: [PATCH 01/14] feat(py_test): add opt-in safeguard against silently-passing tests py_test runs the main module directly and does not automatically invoke a test runner. A common pitfall is to define test classes/functions but forget to run them, causing the test to silently pass. Add a validation action, gated behind the new //python/config_settings:validate_test_main flag (auto/enabled/disabled, default off), that statically analyzes the main module with ast and fails the build if its top-level body is inert (only definitions, imports, assignments, docstrings) with nothing that runs tests. Fixes #3824 --- CHANGELOG.md | 6 + .../python/config_settings/index.md | 34 ++++ python/config_settings/BUILD.bazel | 9 ++ python/private/BUILD.bazel | 21 +++ python/private/attributes.bzl | 8 + python/private/common_labels.bzl | 1 + python/private/flags.bzl | 21 +++ python/private/py_executable.bzl | 81 +++++++++- python/private/py_test_main_validator.py | 147 ++++++++++++++++++ tests/base_rules/py_test/py_test_tests.bzl | 58 ++++++- tests/validate_test_main/BUILD.bazel | 24 +++ .../validate_test_main_test.py | 105 +++++++++++++ 12 files changed, 512 insertions(+), 3 deletions(-) create mode 100644 python/private/py_test_main_validator.py create mode 100644 tests/validate_test_main/BUILD.bazel create mode 100644 tests/validate_test_main/validate_test_main_test.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7efdd66312..8940475143 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,12 @@ END_UNRELEASED_TEMPLATE * (pypi) `package_metadata` support, fixes [#2054](https://github.com/bazel-contrib/rules_python/issues/2054). * (coverage) Add support for python 3.14 and bump `coverage.py` to 7.10.7. +* (py_test) Added an opt-in safeguard against `py_test` targets that silently + pass without running any tests. Set + {obj}`--@rules_python//python/config_settings:validate_test_main=enabled` to + fail the build when a test's main module only contains inert top-level + statements (definitions, imports, assignments) and never invokes a test + runner ([#3824](https://github.com/bazel-contrib/rules_python/issues/3824)). {#v2-0-3} ## [2.0.3] - 2026-06-15 diff --git a/docs/api/rules_python/python/config_settings/index.md b/docs/api/rules_python/python/config_settings/index.md index d7bc296296..cf2139997f 100644 --- a/docs/api/rules_python/python/config_settings/index.md +++ b/docs/api/rules_python/python/config_settings/index.md @@ -242,6 +242,40 @@ The `auto` value The `omit_if_generated_source` value was removed :::: +::::{bzl:flag} validate_test_main +Determines if `py_test` runs a build-time validation that its main module +actually runs tests. + +A common `py_test` pitfall is to define test classes or functions but forget +to add code that runs them (for example, assuming `py_test` automatically +invokes `unittest` or `pytest`). When that happens, the test does nothing and +silently passes. + +When enabled, a validation action statically analyzes the main module and +fails the build if its top-level body is "inert" -- i.e. it only contains +definitions, imports, assignments, and docstrings, with nothing that actually +runs tests (such as an `if __name__ == "__main__":` guard that invokes a test +runner). + +This is only applicable to `py_test` targets that have a `main` source file; +targets using `main_module` are not checked. + +Values: + +* `auto`: (default) Automatically decide the effective value; the current + behavior is `disabled`. +* `enabled`: Run the validation action. +* `disabled`: Don't run the validation action. + +:::{note} +Enabling this requires the exec tools toolchain (with an exec interpreter) to +be registered, which is the case for the default hermetic toolchains. +::: + +:::{versionadded} VERSION_NEXT_FEATURE +::: +:::: + ::::{bzl:flag} py_linux_libc Set what libc is used for the target platform. This will affect which whl binaries will be pulled and what toolchain will be auto-detected. Currently `rules_python` only supplies toolchains compatible with `glibc`. diff --git a/python/config_settings/BUILD.bazel b/python/config_settings/BUILD.bazel index 5b1317872f..04e1acb0d8 100644 --- a/python/config_settings/BUILD.bazel +++ b/python/config_settings/BUILD.bazel @@ -10,6 +10,7 @@ load( "LibcFlag", "PrecompileFlag", "PrecompileSourceRetentionFlag", + "ValidateTestMainFlag", "VenvsSitePackages", "VenvsUseDeclareSymlinkFlag", rp_string_flag = "string_flag", @@ -72,6 +73,14 @@ string_flag( visibility = ["//visibility:public"], ) +string_flag( + name = "validate_test_main", + build_setting_default = ValidateTestMainFlag.AUTO, + values = ValidateTestMainFlag.flag_values(), + # NOTE: Only public because it's an implicit dependency of py_test. + visibility = ["//visibility:public"], +) + string_flag( name = "precompile_source_retention", build_setting_default = PrecompileSourceRetentionFlag.AUTO, diff --git a/python/private/BUILD.bazel b/python/private/BUILD.bazel index e4b38c2d1d..3e0b93d0ca 100644 --- a/python/private/BUILD.bazel +++ b/python/private/BUILD.bazel @@ -18,6 +18,7 @@ load("//python:py_binary.bzl", "py_binary") load("//python:py_library.bzl", "py_library") load(":bazel_config_mode.bzl", "bazel_config_mode") load(":py_exec_tools_toolchain.bzl", "current_interpreter_executable") +load(":py_interpreter_program.bzl", "py_interpreter_program") load(":sentinel.bzl", "sentinel") load(":stamp.bzl", "stamp_build_setting") load(":uncachable_version_file.bzl", "define_uncachable_version_file") @@ -442,6 +443,7 @@ bzl_library( ":py_executable_info_bzl", ":py_info_bzl", ":py_internal_bzl", + ":py_interpreter_program_bzl", ":py_runtime_info_bzl", ":rules_cc_srcs_bzl", ":toolchain_types_bzl", @@ -913,3 +915,22 @@ py_binary( srcs = ["tools/sync_runtimes_manifest_workspace.py"], visibility = ["//:__subpackages__"], ) + +# Tool used by py_test's validation action to statically check that the main +# module actually runs tests. See the validate_test_main config setting. +py_interpreter_program( + name = "py_test_main_validator", + main = "py_test_main_validator.py", + # Not actually public. Only public because it's an implicit dependency of + # the py_test rule. + visibility = ["//visibility:public"], +) + +py_library( + name = "py_test_main_validator_lib", + srcs = ["py_test_main_validator.py"], + imports = ["../.."], + visibility = [ + "//tests/validate_test_main:__pkg__", + ], +) diff --git a/python/private/attributes.bzl b/python/private/attributes.bzl index ad741d078d..61b41d527c 100644 --- a/python/private/attributes.bzl +++ b/python/private/attributes.bzl @@ -535,6 +535,14 @@ environment when the test is executed by bazel test. "@platforms//os:watchos", ], ), + "_validate_test_main": lambda: attrb.Label( + default = "//python/private:py_test_main_validator", + cfg = "exec", + ), + "_validate_test_main_flag": lambda: attrb.Label( + default = labels.VALIDATE_TEST_MAIN, + providers = [BuildSettingInfo], + ), }) # Attributes specific to Python test-equivalent executable rules. Such rules may diff --git a/python/private/common_labels.bzl b/python/private/common_labels.bzl index a83ba2b462..32dc6ddcf4 100644 --- a/python/private/common_labels.bzl +++ b/python/private/common_labels.bzl @@ -29,6 +29,7 @@ labels = struct( PY_FREETHREADED = str(Label("//python/config_settings:py_freethreaded")), PY_LINUX_LIBC = str(Label("//python/config_settings:py_linux_libc")), REPL_DEP = str(Label("//python/bin:repl_dep")), + VALIDATE_TEST_MAIN = str(Label("//python/config_settings:validate_test_main")), VENVS_SITE_PACKAGES = str(Label("//python/config_settings:venvs_site_packages")), VENVS_USE_DECLARE_SYMLINK = str(Label("//python/config_settings:venvs_use_declare_symlink")), VISIBLE_FOR_TESTING = str(Label("//python/private:visible_for_testing")), diff --git a/python/private/flags.bzl b/python/private/flags.bzl index d9e3aa41c3..32769e4084 100644 --- a/python/private/flags.bzl +++ b/python/private/flags.bzl @@ -87,6 +87,27 @@ AddSrcsToRunfilesFlag = FlagEnum( is_enabled = _AddSrcsToRunfilesFlag_is_enabled, ) +def _ValidateTestMainFlag_is_enabled(ctx): + value = ctx.attr._validate_test_main_flag[BuildSettingInfo].value + if value == ValidateTestMainFlag.AUTO: + # Default off; intended to be flipped to enabled in a future major + # version (e.g. rules_python 3.0). + value = ValidateTestMainFlag.DISABLED + return value == ValidateTestMainFlag.ENABLED + +# Determines if py_test runs a validation action that statically checks the +# main module actually runs tests (instead of silently passing). +# buildifier: disable=name-conventions +ValidateTestMainFlag = FlagEnum( + # Automatically decide the effective value; currently resolves to disabled. + AUTO = "auto", + # Run the validation action. + ENABLED = "enabled", + # Don't run the validation action. + DISABLED = "disabled", + is_enabled = _ValidateTestMainFlag_is_enabled, +) + def _string_flag_impl(ctx): if ctx.attr.override: value = ctx.attr.override diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index 198ff9d548..4847d67868 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -57,12 +57,13 @@ load( "runfiles_root_path", ) load(":common_labels.bzl", "labels") -load(":flags.bzl", "BootstrapImplFlag", "VenvsUseDeclareSymlinkFlag", "read_possibly_native_flag") +load(":flags.bzl", "BootstrapImplFlag", "ValidateTestMainFlag", "VenvsUseDeclareSymlinkFlag", "read_possibly_native_flag") load(":precompile.bzl", "maybe_precompile") load(":py_cc_link_params_info.bzl", "PyCcLinkParamsInfo") load(":py_executable_info.bzl", "PyExecutableInfo") load(":py_info.bzl", "PyInfo", "VenvSymlinkKind") load(":py_internal.bzl", "py_internal") +load(":py_interpreter_program.bzl", "PyInterpreterProgramInfo") load(":py_runtime_info.bzl", "DEFAULT_STUB_SHEBANG") load(":reexports.bzl", "BuiltinPyInfo", "BuiltinPyRuntimeInfo") load(":rule_builders.bzl", "ruleb") @@ -1167,6 +1168,11 @@ def py_executable_base_impl(ctx, *, semantics, is_test, inherited_environment = main_py = determine_main(ctx) else: main_py = None + + # Keep a reference to the main source file (before it may be replaced with a + # precompiled pyc below) so the test-main validation can statically analyze + # the original source. + main_py_source = main_py direct_sources = filter_to_py_srcs(ctx.files.srcs) precompile_result = maybe_precompile(ctx, direct_sources) @@ -1285,10 +1291,81 @@ def py_executable_base_impl(ctx, *, semantics, is_test, inherited_environment = implicit_pyc_source_files = implicit_pyc_source_files, imports = imports, ) - _add_provider_output_group_info(providers, py_info, exec_result.output_groups) + output_groups = dict(exec_result.output_groups) + if is_test: + _maybe_add_test_main_validation(ctx, main_py_source, output_groups) + _add_provider_output_group_info(providers, py_info, output_groups) return providers +def _maybe_add_test_main_validation(ctx, main_py, output_groups): + """Adds a validation action that checks the test main actually runs tests. + + This is a safeguard against the common pitfall of defining test classes or + functions but forgetting to invoke a test runner, which causes the test to + silently pass without running anything. See the + `//python/config_settings:validate_test_main` flag. + + Args: + ctx: Rule ctx. + main_py: File or None; the main entry point source file. None when the + target uses `main_module` (which can't be statically analyzed here). + output_groups: dict[str, depset[File]]; mutated in place to add the + `_validation` output group when a validation action is created. + """ + if not ValidateTestMainFlag.is_enabled(ctx): + return + + # `main_module` targets execute a module by name; there's no single source + # file to statically analyze, so the check doesn't apply. + if main_py == None: + return + + exec_tools_toolchain = ctx.toolchains[EXEC_TOOLS_TOOLCHAIN_TYPE] + if exec_tools_toolchain == None or exec_tools_toolchain.exec_tools.exec_interpreter == None: + fail( + "Validating py_test main modules requires the exec tools toolchain " + + "with an exec interpreter, but none was found. Either register one " + + "or set --@rules_python//python/config_settings:validate_test_main=disabled.", + ) + + exec_tools = exec_tools_toolchain.exec_tools + validator = ctx.attr._validate_test_main + program_info = validator[PyInterpreterProgramInfo] + interpreter = exec_tools.exec_interpreter[DefaultInfo].files_to_run + validator_files_to_run = validator[DefaultInfo].files_to_run + + validation_output = ctx.actions.declare_file(ctx.label.name + "_validate_test_main.txt") + + args = ctx.actions.args() + args.add_all(program_info.interpreter_args) + args.add(validator_files_to_run.executable) + args.add("--src", main_py) + args.add("--src_name", main_py.short_path) + args.add("--label", str(ctx.label)) + args.add("--output", validation_output) + + execution_requirements = {} + if testing.ExecutionInfo in validator: + execution_requirements = validator[testing.ExecutionInfo].requirements + + ctx.actions.run( + executable = interpreter, + arguments = [args], + inputs = [main_py], + outputs = [validation_output], + tools = [validator_files_to_run], + mnemonic = "PyValidateTestMain", + progress_message = "Validating py_test main %{label}", + env = program_info.env | { + "PYTHONNOUSERSITE": "1", + "PYTHONSAFEPATH": "1", + }, + execution_requirements = execution_requirements, + toolchain = EXEC_TOOLS_TOOLCHAIN_TYPE, + ) + output_groups["_validation"] = depset([validation_output]) + def _get_build_info(ctx, cc_toolchain): build_info_files = py_internal.cc_toolchain_build_info_files(cc_toolchain) if cc_helper.is_stamping_enabled(ctx): diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py new file mode 100644 index 0000000000..4dd3451d7a --- /dev/null +++ b/python/private/py_test_main_validator.py @@ -0,0 +1,147 @@ +# Copyright 2024 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. + +"""Static check that a py_test main module actually runs something. + +A common ``py_test`` pitfall is to define test classes or functions but forget +to add any code that actually executes them (for example, assuming that +``py_test`` automatically invokes ``unittest`` or ``pytest``). When that +happens, running the test does nothing and the target silently passes. + +This validator parses the main module with :mod:`ast` and fails if the +module body is "inert", i.e. every top-level statement is one that does not +run anything (definitions, imports, assignments, docstrings, ``pass``). A +single active statement -- a bare call, a loop, or the conventional +``if __name__ == "__main__":`` guard -- is enough to consider the module able +to run tests. +""" + +import argparse +import ast +import sys + + +# Statement node types that do not, on their own, run any test code. A module +# whose top-level body consists solely of these is considered inert. +_INERT_NODE_TYPES = ( + ast.FunctionDef, + ast.AsyncFunctionDef, + ast.ClassDef, + ast.Import, + ast.ImportFrom, + ast.Assign, + ast.AnnAssign, + ast.AugAssign, + ast.Pass, +) + + +def _is_inert_statement(node: ast.stmt) -> bool: + """Returns True if the top-level statement does not run any test code.""" + if isinstance(node, _INERT_NODE_TYPES): + return True + + # Bare constant expressions: docstrings, `...` (Ellipsis), and similar + # no-op expression statements. + if isinstance(node, ast.Expr) and isinstance(node.value, ast.Constant): + return True + + return False + + +def module_runs_something(tree: ast.Module) -> bool: + """Returns True if the module body has at least one active statement.""" + return any(not _is_inert_statement(node) for node in tree.body) + + +def _format_error(label: str, src_name: str) -> str: + target = label or src_name + return ( + "py_test target {target} will not run any tests.\n" + "\n" + "The main module '{src_name}' only contains inert top-level statements " + "(class/function definitions, imports, assignments). Running it does " + "nothing, so the test silently passes without executing any test " + "code.\n" + "\n" + "py_test runs the main module directly; it does not automatically " + "invoke a test runner such as unittest or pytest. Add code that runs " + "your tests, for example:\n" + "\n" + " if __name__ == \"__main__\":\n" + " unittest.main()\n" + "\n" + "or use a main module that invokes a runner (e.g. pytest.main()).\n" + "\n" + "This check can be disabled by setting " + "--@rules_python//python/config_settings:validate_test_main=disabled." + ).format(target=target, src_name=src_name) + + +def main(args) -> int: + parser = argparse.ArgumentParser() + parser.add_argument( + "--src", + required=True, + help="Path to the main .py source file to analyze.", + ) + parser.add_argument( + "--src_name", + default="", + help="Human-friendly name of the source file, used in error messages.", + ) + parser.add_argument( + "--label", + default="", + help="The py_test target label, used in error messages.", + ) + parser.add_argument( + "--output", + required=True, + help="Path to the validation marker file to write on success.", + ) + options = parser.parse_args(args) + + src_name = options.src_name or options.src + + with open(options.src, "rb") as f: + source = f.read() + + try: + tree = ast.parse(source, filename=src_name) + except SyntaxError as e: + # A syntax error is surfaced by other actions (compilation/execution). + # The validator can't analyze the file, so don't fail here; treat it as + # passing to avoid duplicate or confusing errors. + sys.stderr.write( + "WARNING: py_test main validator could not parse {}: {}\n".format( + src_name, e + ) + ) + with open(options.output, "w") as out: + out.write("") + return 0 + + if not module_runs_something(tree): + sys.stderr.write(_format_error(options.label, src_name) + "\n") + return 1 + + # Validation actions must produce their declared outputs on success. + with open(options.output, "w") as out: + out.write("") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/tests/base_rules/py_test/py_test_tests.bzl b/tests/base_rules/py_test/py_test_tests.bzl index fd284beffd..e16204e9f2 100644 --- a/tests/base_rules/py_test/py_test_tests.bzl +++ b/tests/base_rules/py_test/py_test_tests.bzl @@ -16,12 +16,13 @@ load("@rules_testing//lib:analysis_test.bzl", "analysis_test") load("@rules_testing//lib:util.bzl", rt_util = "util") load("//python:py_test.bzl", "py_test") +load("//python/private:common_labels.bzl", "labels") # buildifier: disable=bzl-visibility load( "//tests/base_rules:py_executable_base_tests.bzl", "create_executable_tests", ) load("//tests/base_rules:util.bzl", pt_util = "util") -load("//tests/support:support.bzl", "CC_TOOLCHAIN", "CROSSTOOL_TOP") +load("//tests/support:support.bzl", "CC_TOOLCHAIN", "CROSSTOOL_TOP", "PY_TOOLCHAINS") load("//tests/support/platforms:platforms.bzl", "platform_targets") # The Windows CI currently runs as root, which breaks when @@ -96,6 +97,61 @@ def _test_non_mac_doesnt_require_darwin_for_execution_impl(env, target): _tests.append(_test_non_mac_doesnt_require_darwin_for_execution) +_VALIDATE_TEST_MAIN_CONFIG_SETTINGS = { + "//command_line_option:extra_toolchains": [PY_TOOLCHAINS, CC_TOOLCHAIN], + labels.EXEC_TOOLS_TOOLCHAIN: "enabled", +} + +def _test_validate_test_main_enabled(name, config): + rt_util.helper_target( + config.rule, + name = name + "_subject", + srcs = [name + "_subject.py"], + ) + analysis_test( + name = name, + impl = _test_validate_test_main_enabled_impl, + target = name + "_subject", + config_settings = _VALIDATE_TEST_MAIN_CONFIG_SETTINGS | { + labels.VALIDATE_TEST_MAIN: "enabled", + }, + attr_values = _SKIP_WINDOWS, + ) + +def _test_validate_test_main_enabled_impl(env, target): + mnemonics = [a.mnemonic for a in target.actions] + env.expect.that_collection(mnemonics).contains("PyValidateTestMain") + env.expect.that_bool( + hasattr(target[OutputGroupInfo], "_validation"), + ).equals(True) + +_tests.append(_test_validate_test_main_enabled) + +def _test_validate_test_main_disabled(name, config): + rt_util.helper_target( + config.rule, + name = name + "_subject", + srcs = [name + "_subject.py"], + ) + analysis_test( + name = name, + impl = _test_validate_test_main_disabled_impl, + target = name + "_subject", + config_settings = _VALIDATE_TEST_MAIN_CONFIG_SETTINGS | { + labels.VALIDATE_TEST_MAIN: "disabled", + }, + attr_values = _SKIP_WINDOWS, + ) + +def _test_validate_test_main_disabled_impl(env, target): + mnemonics = [a.mnemonic for a in target.actions] + env.expect.that_collection(mnemonics).not_contains("PyValidateTestMain") + env.expect.that_bool( + hasattr(target[OutputGroupInfo], "_validation"), + ).equals(False) + +_tests.append(_test_validate_test_main_disabled) + def py_test_test_suite(name): config = struct(rule = py_test) native.test_suite( diff --git a/tests/validate_test_main/BUILD.bazel b/tests/validate_test_main/BUILD.bazel new file mode 100644 index 0000000000..b37538777e --- /dev/null +++ b/tests/validate_test_main/BUILD.bazel @@ -0,0 +1,24 @@ +# Copyright 2024 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. + +load("//python:py_test.bzl", "py_test") + +py_test( + name = "validate_test_main_test", + srcs = ["validate_test_main_test.py"], + main = "validate_test_main_test.py", + deps = [ + "//python/private:py_test_main_validator_lib", + ], +) diff --git a/tests/validate_test_main/validate_test_main_test.py b/tests/validate_test_main/validate_test_main_test.py new file mode 100644 index 0000000000..0f630d8f3f --- /dev/null +++ b/tests/validate_test_main/validate_test_main_test.py @@ -0,0 +1,105 @@ +#!/usr/bin/env python3 +# Copyright 2024 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 ast +import textwrap +import unittest + +from python.private.py_test_main_validator import module_runs_something + + +def _runs_something(source: str) -> bool: + tree = ast.parse(textwrap.dedent(source)) + return module_runs_something(tree) + + +class ModuleRunsSomethingTest(unittest.TestCase): + def test_only_definitions_is_inert(self): + self.assertFalse( + _runs_something( + """ + import unittest + + class MyTest(unittest.TestCase): + def test_foo(self): + self.assertTrue(True) + """ + ) + ) + + def test_definitions_with_assignments_is_inert(self): + self.assertFalse( + _runs_something( + """ + import unittest + + CONSTANT = 5 + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + + def helper(): pass + """ + ) + ) + + def test_empty_module_is_inert(self): + self.assertFalse(_runs_something("")) + + def test_docstring_only_is_inert(self): + self.assertFalse(_runs_something('"""A module docstring."""')) + + def test_if_name_main_guard_runs_something(self): + self.assertTrue( + _runs_something( + """ + import unittest + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + + if __name__ == "__main__": + unittest.main() + """ + ) + ) + + def test_bare_call_runs_something(self): + self.assertTrue( + _runs_something( + """ + import pytest + pytest.main() + """ + ) + ) + + def test_top_level_loop_runs_something(self): + self.assertTrue( + _runs_something( + """ + def f(): pass + + for _ in range(1): + f() + """ + ) + ) + + +if __name__ == "__main__": + unittest.main() From a2a4fcddba4af82b817b769a89925f6a3d8fdcb3 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 09:27:27 -0700 Subject: [PATCH 02/14] test(py_test): add bazel-in-bazel e2e test for validate_test_main Add an integration test that builds real py_test targets with the validate_test_main flag and asserts: - an inert test main fails the build with a descriptive error when enabled - a main that invokes a runner builds when enabled - an inert test builds when disabled and by default --- .bazelignore | 1 + .bazelrc.deleted_packages | 5 +- tests/integration/BUILD.bazel | 5 ++ .../validate_test_main/BUILD.bazel | 28 +++++++++++ .../validate_test_main/MODULE.bazel | 24 +++++++++ .../integration/validate_test_main/WORKSPACE | 13 +++++ .../validate_test_main/WORKSPACE.bzlmod | 0 .../validate_test_main/good_test.py | 24 +++++++++ .../validate_test_main/inert_test.py | 21 ++++++++ tests/integration/validate_test_main_test.py | 50 +++++++++++++++++++ 10 files changed, 169 insertions(+), 2 deletions(-) create mode 100644 tests/integration/validate_test_main/BUILD.bazel create mode 100644 tests/integration/validate_test_main/MODULE.bazel create mode 100644 tests/integration/validate_test_main/WORKSPACE create mode 100644 tests/integration/validate_test_main/WORKSPACE.bzlmod create mode 100644 tests/integration/validate_test_main/good_test.py create mode 100644 tests/integration/validate_test_main/inert_test.py create mode 100644 tests/integration/validate_test_main_test.py diff --git a/.bazelignore b/.bazelignore index 2cf1523aef..ad914fe895 100644 --- a/.bazelignore +++ b/.bazelignore @@ -36,3 +36,4 @@ tests/integration/local_toolchains/bazel-local_toolchains tests/integration/py_cc_toolchain_registered/bazel-py_cc_toolchain_registered tests/integration/toolchain_target_settings/bazel-module_under_test tests/integration/uv_lock/bazel-uv_lock +tests/integration/validate_test_main/bazel-module_under_test diff --git a/.bazelrc.deleted_packages b/.bazelrc.deleted_packages index 407fd1cb48..5a3d35449c 100644 --- a/.bazelrc.deleted_packages +++ b/.bazelrc.deleted_packages @@ -27,20 +27,21 @@ common --deleted_packages=gazelle/manifest/hasher common --deleted_packages=gazelle/manifest/test common --deleted_packages=gazelle/modules_mapping common --deleted_packages=gazelle/python -common --deleted_packages=gazelle/pythonconfig common --deleted_packages=gazelle/python/private +common --deleted_packages=gazelle/pythonconfig common --deleted_packages=tests/integration/bzlmod_lockfile common --deleted_packages=tests/integration/compile_pip_requirements common --deleted_packages=tests/integration/compile_pip_requirements_test_from_external_repo common --deleted_packages=tests/integration/custom_commands common --deleted_packages=tests/integration/local_toolchains common --deleted_packages=tests/integration/pip_parse -common --deleted_packages=tests/integration/pip_parse/empty common --deleted_packages=tests/integration/pip_parse_isolated +common --deleted_packages=tests/integration/pip_parse/empty common --deleted_packages=tests/integration/py_cc_toolchain_registered common --deleted_packages=tests/integration/runtime_manifests common --deleted_packages=tests/integration/toolchain_target_settings common --deleted_packages=tests/integration/uv_lock +common --deleted_packages=tests/integration/validate_test_main common --deleted_packages=tests/modules/another_module common --deleted_packages=tests/modules/other common --deleted_packages=tests/modules/other/nspkg_delta diff --git a/tests/integration/BUILD.bazel b/tests/integration/BUILD.bazel index 904fb4c247..ea44859f9a 100644 --- a/tests/integration/BUILD.bazel +++ b/tests/integration/BUILD.bazel @@ -118,6 +118,11 @@ rules_python_integration_test( py_main = "toolchain_target_settings_test.py", ) +rules_python_integration_test( + name = "validate_test_main_test", + py_main = "validate_test_main_test.py", +) + rules_python_integration_test( name = "uv_lock_test", py_deps = [ diff --git a/tests/integration/validate_test_main/BUILD.bazel b/tests/integration/validate_test_main/BUILD.bazel new file mode 100644 index 0000000000..e6e7545e33 --- /dev/null +++ b/tests/integration/validate_test_main/BUILD.bazel @@ -0,0 +1,28 @@ +# Copyright 2024 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. + +load("@rules_python//python:py_test.bzl", "py_test") + +# A test whose main module only defines a test case but never runs it. With +# --validate_test_main=enabled this should fail to build. +py_test( + name = "inert_test", + srcs = ["inert_test.py"], +) + +# A test whose main module invokes a runner. This should always build. +py_test( + name = "good_test", + srcs = ["good_test.py"], +) diff --git a/tests/integration/validate_test_main/MODULE.bazel b/tests/integration/validate_test_main/MODULE.bazel new file mode 100644 index 0000000000..499ba0dc90 --- /dev/null +++ b/tests/integration/validate_test_main/MODULE.bazel @@ -0,0 +1,24 @@ +# Copyright 2024 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. + +module(name = "module_under_test") + +bazel_dep(name = "rules_python", version = "0.0.0") +local_path_override( + module_name = "rules_python", + path = "../../..", +) + +python = use_extension("@rules_python//python/extensions:python.bzl", "python") +python.toolchain(python_version = "3.11") diff --git a/tests/integration/validate_test_main/WORKSPACE b/tests/integration/validate_test_main/WORKSPACE new file mode 100644 index 0000000000..de908549c0 --- /dev/null +++ b/tests/integration/validate_test_main/WORKSPACE @@ -0,0 +1,13 @@ +local_repository( + name = "rules_python", + path = "../../..", +) + +load("@rules_python//python:repositories.bzl", "py_repositories", "python_register_toolchains") + +py_repositories() + +python_register_toolchains( + name = "python_3_11", + python_version = "3.11", +) diff --git a/tests/integration/validate_test_main/WORKSPACE.bzlmod b/tests/integration/validate_test_main/WORKSPACE.bzlmod new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/integration/validate_test_main/good_test.py b/tests/integration/validate_test_main/good_test.py new file mode 100644 index 0000000000..9f1ca21e3b --- /dev/null +++ b/tests/integration/validate_test_main/good_test.py @@ -0,0 +1,24 @@ +# Copyright 2024 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 GoodTest(unittest.TestCase): + def test_something(self): + self.assertTrue(True) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/integration/validate_test_main/inert_test.py b/tests/integration/validate_test_main/inert_test.py new file mode 100644 index 0000000000..84fe8166a0 --- /dev/null +++ b/tests/integration/validate_test_main/inert_test.py @@ -0,0 +1,21 @@ +# Copyright 2024 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 InertTest(unittest.TestCase): + def test_nothing_runs(self): + # This test case is never executed because nothing invokes a runner. + self.assertTrue(True) diff --git a/tests/integration/validate_test_main_test.py b/tests/integration/validate_test_main_test.py new file mode 100644 index 0000000000..2b82fb56c7 --- /dev/null +++ b/tests/integration/validate_test_main_test.py @@ -0,0 +1,50 @@ +# Copyright 2024 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 + +from tests.integration import runner + +_FLAG = "--@rules_python//python/config_settings:validate_test_main" + + +class ValidateTestMainTest(runner.TestCase): + def test_inert_test_fails_when_enabled(self): + """An inert test main should fail the build when validation is on.""" + result = self.run_bazel( + "build", + f"{_FLAG}=enabled", + "//:inert_test", + check=False, + ) + self.assertNotEqual(result.exit_code, 0, "Expected build to fail") + self.assert_result_matches(result, r"will not run any tests") + + def test_good_test_builds_when_enabled(self): + """A main that invokes a runner should build when validation is on.""" + self.run_bazel("build", f"{_FLAG}=enabled", "//:good_test") + + def test_inert_test_builds_when_disabled(self): + """Validation is off by default, so even an inert test builds.""" + self.run_bazel("build", f"{_FLAG}=disabled", "//:inert_test") + + def test_inert_test_builds_by_default(self): + """The default (auto) resolves to disabled, so an inert test builds.""" + self.run_bazel("build", "//:inert_test") + + +if __name__ == "__main__": + # Enabling this makes the runner log subprocesses as the test goes along. + # logging.basicConfig(level = "INFO") + unittest.main() From b3fee23c2d2a911d481635e035a79593b5347d52 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 09:36:16 -0700 Subject: [PATCH 03/14] fix(py_test): treat top-level global and type-alias as inert Address review feedback: top-level `global x` and PEP 695 `type X = ...` aliases don't run any test code, so they should count as inert when deciding whether a test main runs something. `ast.TypeAlias` only exists on Python 3.12+, so it's added dynamically. --- python/private/py_test_main_validator.py | 15 +++++++--- .../validate_test_main_test.py | 29 +++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index 4dd3451d7a..4585d4b04f 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -31,10 +31,9 @@ import ast import sys - # Statement node types that do not, on their own, run any test code. A module # whose top-level body consists solely of these is considered inert. -_INERT_NODE_TYPES = ( +_INERT_NODE_TYPES = [ ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, @@ -43,8 +42,16 @@ ast.Assign, ast.AnnAssign, ast.AugAssign, + ast.Global, ast.Pass, -) +] + +# `ast.TypeAlias` (PEP 695, e.g. `type Alias = int`) only exists on Python +# 3.12+. Add it dynamically so the validator still imports on older versions. +if hasattr(ast, "TypeAlias"): + _INERT_NODE_TYPES.append(ast.TypeAlias) + +_INERT_NODE_TYPES = tuple(_INERT_NODE_TYPES) def _is_inert_statement(node: ast.stmt) -> bool: @@ -79,7 +86,7 @@ def _format_error(label: str, src_name: str) -> str: "invoke a test runner such as unittest or pytest. Add code that runs " "your tests, for example:\n" "\n" - " if __name__ == \"__main__\":\n" + ' if __name__ == "__main__":\n' " unittest.main()\n" "\n" "or use a main module that invokes a runner (e.g. pytest.main()).\n" diff --git a/tests/validate_test_main/validate_test_main_test.py b/tests/validate_test_main/validate_test_main_test.py index 0f630d8f3f..646abb58b5 100644 --- a/tests/validate_test_main/validate_test_main_test.py +++ b/tests/validate_test_main/validate_test_main_test.py @@ -62,6 +62,35 @@ def test_empty_module_is_inert(self): def test_docstring_only_is_inert(self): self.assertFalse(_runs_something('"""A module docstring."""')) + def test_global_statement_is_inert(self): + self.assertFalse( + _runs_something( + """ + global x + + class MyTest: + def test_foo(self): + pass + """ + ) + ) + + @unittest.skipUnless( + hasattr(ast, "TypeAlias"), "PEP 695 type aliases require Python 3.12+" + ) + def test_type_alias_is_inert(self): + self.assertFalse( + _runs_something( + """ + type Alias = int + + class MyTest: + def test_foo(self): + pass + """ + ) + ) + def test_if_name_main_guard_runs_something(self): self.assertTrue( _runs_something( From 19ba852ad52c3f20323e96b6b310b680a85337cc Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 09:43:20 -0700 Subject: [PATCH 04/14] test: update bzlmod lockfile for common_labels.bzl change The uv module extension transitively loads common_labels.bzl, so adding the VALIDATE_TEST_MAIN label changed the extension's bzlTransitiveDigest. Regenerate the integration test's MODULE.bazel.lock to match. --- tests/integration/bzlmod_lockfile/MODULE.bazel.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/bzlmod_lockfile/MODULE.bazel.lock b/tests/integration/bzlmod_lockfile/MODULE.bazel.lock index 2a0bc7d76b..24488c470e 100644 --- a/tests/integration/bzlmod_lockfile/MODULE.bazel.lock +++ b/tests/integration/bzlmod_lockfile/MODULE.bazel.lock @@ -250,7 +250,7 @@ }, "@@rules_python+//python/uv:uv.bzl%uv": { "general": { - "bzlTransitiveDigest": "46RcxJnhOapMeaxdcMm3RmVdNp1nPCewOOXoZyIbQ20=", + "bzlTransitiveDigest": "9Xpv8iahdckhsr75bzQjRQRXxth6cYUerRq3CBWkRy8=", "usagesDigest": "6yXGw7XDyXjOfqBL0SBu1YBEMMYPQzCE3jTzUCkxPgg=", "recordedInputs": [ "REPO_MAPPING:rules_python+,bazel_tools bazel_tools", From ec04df19c830ea5f590ae21da32beb8ea3fef45e Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 10:54:42 -0700 Subject: [PATCH 05/14] feat(py_test): allow test mains that only import modules Only flag a main module as inert when it actually defines test classes or functions but never runs them. A module that defines nothing and only imports other modules (e.g. an aggregator that relies on import side effects) is now always allowed, avoiding false positives. Update unit tests, docs, and add an import-only e2e case. --- .../python/config_settings/index.md | 12 ++-- python/private/py_test_main_validator.py | 29 +++++++- .../validate_test_main/BUILD.bazel | 7 ++ .../validate_test_main/import_only_test.py | 17 +++++ tests/integration/validate_test_main_test.py | 4 ++ .../validate_test_main_test.py | 71 ++++++++++++------- 6 files changed, 111 insertions(+), 29 deletions(-) create mode 100644 tests/integration/validate_test_main/import_only_test.py diff --git a/docs/api/rules_python/python/config_settings/index.md b/docs/api/rules_python/python/config_settings/index.md index cf2139997f..341fad61df 100644 --- a/docs/api/rules_python/python/config_settings/index.md +++ b/docs/api/rules_python/python/config_settings/index.md @@ -252,10 +252,14 @@ invokes `unittest` or `pytest`). When that happens, the test does nothing and silently passes. When enabled, a validation action statically analyzes the main module and -fails the build if its top-level body is "inert" -- i.e. it only contains -definitions, imports, assignments, and docstrings, with nothing that actually -runs tests (such as an `if __name__ == "__main__":` guard that invokes a test -runner). +fails the build if it defines test classes or functions but its top-level body +is "inert" -- i.e. it only contains definitions, imports, assignments, and +docstrings, with nothing that actually runs tests (such as an +`if __name__ == "__main__":` guard that invokes a test runner). + +A module that defines no classes or functions at all (for example, one that +only imports other modules) is always allowed, since it isn't the +"defined some tests but forgot to run them" case this check targets. This is only applicable to `py_test` targets that have a `main` source file; targets using `main_module` are not checked. diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index 4585d4b04f..f3d2975801 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -25,6 +25,11 @@ single active statement -- a bare call, a loop, or the conventional ``if __name__ == "__main__":`` guard -- is enough to consider the module able to run tests. + +As an exception, a module that defines no classes or functions at all (for +example, one that only imports other modules) is always allowed: it isn't the +"defined some tests but forgot to run them" case this check targets, and it +may legitimately rely on import side effects. """ import argparse @@ -72,6 +77,28 @@ def module_runs_something(tree: ast.Module) -> bool: return any(not _is_inert_statement(node) for node in tree.body) +def _defines_test_code(tree: ast.Module) -> bool: + """Returns True if the module defines any top-level class or function.""" + return any( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + for node in tree.body + ) + + +def module_runs_tests(tree: ast.Module) -> bool: + """Returns True if the module appears able to run tests (or isn't checked). + + The check targets the specific pitfall of defining test classes/functions + but never running them. A module that defines no classes or functions at + all (for example, one that only imports other modules) is not subject to + the check and is always allowed, since it isn't the "defined but never run" + case and may legitimately rely on import side effects. + """ + if module_runs_something(tree): + return True + return not _defines_test_code(tree) + + def _format_error(label: str, src_name: str) -> str: target = label or src_name return ( @@ -140,7 +167,7 @@ def main(args) -> int: out.write("") return 0 - if not module_runs_something(tree): + if not module_runs_tests(tree): sys.stderr.write(_format_error(options.label, src_name) + "\n") return 1 diff --git a/tests/integration/validate_test_main/BUILD.bazel b/tests/integration/validate_test_main/BUILD.bazel index e6e7545e33..a539b8f066 100644 --- a/tests/integration/validate_test_main/BUILD.bazel +++ b/tests/integration/validate_test_main/BUILD.bazel @@ -26,3 +26,10 @@ py_test( name = "good_test", srcs = ["good_test.py"], ) + +# A test whose main module defines nothing and only imports. This is allowed +# even with validation enabled, since it isn't the "defined but never run" case. +py_test( + name = "import_only_test", + srcs = ["import_only_test.py"], +) diff --git a/tests/integration/validate_test_main/import_only_test.py b/tests/integration/validate_test_main/import_only_test.py new file mode 100644 index 0000000000..765a489c57 --- /dev/null +++ b/tests/integration/validate_test_main/import_only_test.py @@ -0,0 +1,17 @@ +# Copyright 2024 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. + +# This module defines no classes or functions; it only imports. The validation +# action allows it even when enabled. +import os diff --git a/tests/integration/validate_test_main_test.py b/tests/integration/validate_test_main_test.py index 2b82fb56c7..36d9fa0514 100644 --- a/tests/integration/validate_test_main_test.py +++ b/tests/integration/validate_test_main_test.py @@ -35,6 +35,10 @@ def test_good_test_builds_when_enabled(self): """A main that invokes a runner should build when validation is on.""" self.run_bazel("build", f"{_FLAG}=enabled", "//:good_test") + def test_import_only_test_builds_when_enabled(self): + """A main that only imports (defines nothing) is allowed when on.""" + self.run_bazel("build", f"{_FLAG}=enabled", "//:import_only_test") + def test_inert_test_builds_when_disabled(self): """Validation is off by default, so even an inert test builds.""" self.run_bazel("build", f"{_FLAG}=disabled", "//:inert_test") diff --git a/tests/validate_test_main/validate_test_main_test.py b/tests/validate_test_main/validate_test_main_test.py index 646abb58b5..a4c37f444e 100644 --- a/tests/validate_test_main/validate_test_main_test.py +++ b/tests/validate_test_main/validate_test_main_test.py @@ -17,18 +17,18 @@ import textwrap import unittest -from python.private.py_test_main_validator import module_runs_something +from python.private.py_test_main_validator import module_runs_tests -def _runs_something(source: str) -> bool: +def _runs_tests(source: str) -> bool: tree = ast.parse(textwrap.dedent(source)) - return module_runs_something(tree) + return module_runs_tests(tree) -class ModuleRunsSomethingTest(unittest.TestCase): - def test_only_definitions_is_inert(self): +class ModuleRunsTestsTest(unittest.TestCase): + def test_only_definitions_is_rejected(self): self.assertFalse( - _runs_something( + _runs_tests( """ import unittest @@ -39,9 +39,9 @@ def test_foo(self): ) ) - def test_definitions_with_assignments_is_inert(self): + def test_definitions_with_assignments_is_rejected(self): self.assertFalse( - _runs_something( + _runs_tests( """ import unittest @@ -56,15 +56,9 @@ def helper(): pass ) ) - def test_empty_module_is_inert(self): - self.assertFalse(_runs_something("")) - - def test_docstring_only_is_inert(self): - self.assertFalse(_runs_something('"""A module docstring."""')) - - def test_global_statement_is_inert(self): + def test_global_statement_with_definition_is_rejected(self): self.assertFalse( - _runs_something( + _runs_tests( """ global x @@ -78,9 +72,9 @@ def test_foo(self): @unittest.skipUnless( hasattr(ast, "TypeAlias"), "PEP 695 type aliases require Python 3.12+" ) - def test_type_alias_is_inert(self): + def test_type_alias_with_definition_is_rejected(self): self.assertFalse( - _runs_something( + _runs_tests( """ type Alias = int @@ -91,9 +85,38 @@ def test_foo(self): ) ) - def test_if_name_main_guard_runs_something(self): + def test_import_only_module_is_allowed(self): + # A module that defines nothing and only imports other modules is not + # the "defined but never run" case, so it is allowed. + self.assertTrue( + _runs_tests( + """ + import my_tests + from my_pkg.tests import suite + """ + ) + ) + + def test_imports_and_assignments_without_definitions_is_allowed(self): + self.assertTrue( + _runs_tests( + """ + import my_tests + + CONSTANT = 5 + """ + ) + ) + + def test_empty_module_is_allowed(self): + self.assertTrue(_runs_tests("")) + + def test_docstring_only_is_allowed(self): + self.assertTrue(_runs_tests('"""A module docstring."""')) + + def test_if_name_main_guard_runs_tests(self): self.assertTrue( - _runs_something( + _runs_tests( """ import unittest @@ -107,9 +130,9 @@ def test_foo(self): ) ) - def test_bare_call_runs_something(self): + def test_bare_call_runs_tests(self): self.assertTrue( - _runs_something( + _runs_tests( """ import pytest pytest.main() @@ -117,9 +140,9 @@ def test_bare_call_runs_something(self): ) ) - def test_top_level_loop_runs_something(self): + def test_top_level_loop_runs_tests(self): self.assertTrue( - _runs_something( + _runs_tests( """ def f(): pass From 9136fc69a3648eea07b9da8f263466438214a088 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 10:59:04 -0700 Subject: [PATCH 06/14] test: silence ruff F401 on import-only test fixture The import-only fixture intentionally has an unused import; add a noqa so ruff (pinned v0.15.14 in CI) passes. The comment doesn't affect the AST, so the module stays import-only for the validator. --- tests/integration/validate_test_main/import_only_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/validate_test_main/import_only_test.py b/tests/integration/validate_test_main/import_only_test.py index 765a489c57..96a3da3527 100644 --- a/tests/integration/validate_test_main/import_only_test.py +++ b/tests/integration/validate_test_main/import_only_test.py @@ -14,4 +14,4 @@ # This module defines no classes or functions; it only imports. The validation # action allows it even when enabled. -import os +import os # noqa: F401 From 06ee8ae1279b7c0c92d13394c7393331d869e6a3 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 11:42:35 -0700 Subject: [PATCH 07/14] fix(py_test): treat inert if/try guards as inert recursively Address review feedback: top-level `if TYPE_CHECKING:` and `try: import foo except ImportError:` blocks whose branches are entirely inert previously counted as "active" and let an otherwise-inert test main bypass the check. Inspect `if`/`try`/`try*` branches recursively so these common import guards are correctly treated as inert. The `if` condition is ignored (a guard doesn't run tests; a runner call in a branch body is still detected as active). Add unit tests for TYPE_CHECKING, try/except ImportError, and a runner invoked inside an `if` body. --- python/private/py_test_main_validator.py | 30 +++++++++- .../validate_test_main_test.py | 56 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index f3d2975801..9545349230 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -24,7 +24,9 @@ run anything (definitions, imports, assignments, docstrings, ``pass``). A single active statement -- a bare call, a loop, or the conventional ``if __name__ == "__main__":`` guard -- is enough to consider the module able -to run tests. +to run tests. Top-level ``if`` and ``try`` blocks are inspected recursively, so +common guards like ``if TYPE_CHECKING:`` or ``try: import foo except +ImportError:`` (whose branches are themselves inert) don't bypass the check. As an exception, a module that defines no classes or functions at all (for example, one that only imports other modules) is always allowed: it isn't the @@ -58,6 +60,13 @@ _INERT_NODE_TYPES = tuple(_INERT_NODE_TYPES) +# `ast.TryStar` (PEP 654, `try/except*`) only exists on Python 3.11+. +_TRY_NODE_TYPES = (ast.Try, ast.TryStar) if hasattr(ast, "TryStar") else (ast.Try,) + + +def _all_inert(nodes) -> bool: + return all(_is_inert_statement(node) for node in nodes) + def _is_inert_statement(node: ast.stmt) -> bool: """Returns True if the top-level statement does not run any test code.""" @@ -69,6 +78,25 @@ def _is_inert_statement(node: ast.stmt) -> bool: if isinstance(node, ast.Expr) and isinstance(node.value, ast.Constant): return True + # An `if` whose branches are entirely inert, e.g. the very common + # `if TYPE_CHECKING:` guard around typing-only imports. The condition itself + # is intentionally ignored: a guard expression doesn't run tests, and a + # runner call in a branch body is still detected as active by the recursion. + if isinstance(node, ast.If): + return _all_inert(node.body) and _all_inert(node.orelse) + + # A `try` (or `try/except*`) whose body, handlers, else, and finally are all + # inert, e.g. the common `try: import foo except ImportError: ...` optional + # import pattern. + if isinstance(node, _TRY_NODE_TYPES): + if not ( + _all_inert(node.body) + and _all_inert(node.orelse) + and _all_inert(node.finalbody) + ): + return False + return all(_all_inert(handler.body) for handler in node.handlers) + return False diff --git a/tests/validate_test_main/validate_test_main_test.py b/tests/validate_test_main/validate_test_main_test.py index a4c37f444e..635afa2113 100644 --- a/tests/validate_test_main/validate_test_main_test.py +++ b/tests/validate_test_main/validate_test_main_test.py @@ -85,6 +85,62 @@ def test_foo(self): ) ) + def test_type_checking_block_with_definition_is_rejected(self): + # `if TYPE_CHECKING:` guarding typing-only imports is inert; with a test + # definition and no runner, the module is still rejected. + self.assertFalse( + _runs_tests( + """ + from typing import TYPE_CHECKING + import unittest + + if TYPE_CHECKING: + from typing import Any + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + """ + ) + ) + + def test_try_except_import_error_with_definition_is_rejected(self): + # `try: import foo except ImportError:` optional imports are inert. + self.assertFalse( + _runs_tests( + """ + try: + import foo + except ImportError: + foo = None + + import unittest + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + """ + ) + ) + + def test_if_block_invoking_runner_runs_tests(self): + # A runner call inside an `if` body must still count as active. + self.assertTrue( + _runs_tests( + """ + import sys + import unittest + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + + if "--run" in sys.argv: + unittest.main() + """ + ) + ) + def test_import_only_module_is_allowed(self): # A module that defines nothing and only imports other modules is not # the "defined but never run" case, so it is allowed. From 66811ad6b68cfff57fb6023889b55d18d2cf1167 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 13:51:30 -0700 Subject: [PATCH 08/14] fix(py_test): treat assignments/expressions running calls as active Address review feedback: assignments were unconditionally inert, so a main like `exit_code = unittest.main()` (which actually runs the tests) was a false positive that would fail a valid test build. Mark an assignment or expression statement inert only when its value contains no call/await/yield; class/function bodies are still skipped (those node types remain unconditionally inert), so the method-body calls don't defeat the check. Add unit tests for active assignment and active expression mains. --- python/private/py_test_main_validator.py | 39 ++++++++++++++----- .../validate_test_main_test.py | 32 +++++++++++++++ 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index 9545349230..bd3c4b8c15 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -38,17 +38,15 @@ import ast import sys -# Statement node types that do not, on their own, run any test code. A module -# whose top-level body consists solely of these is considered inert. +# Statement node types that never run any code on their own, regardless of +# their contents. A module whose top-level body consists solely of these (and +# inert assignments/expressions/guards, see below) is considered inert. _INERT_NODE_TYPES = [ ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Import, ast.ImportFrom, - ast.Assign, - ast.AnnAssign, - ast.AugAssign, ast.Global, ast.Pass, ] @@ -63,6 +61,24 @@ # `ast.TryStar` (PEP 654, `try/except*`) only exists on Python 3.11+. _TRY_NODE_TYPES = (ast.Try, ast.TryStar) if hasattr(ast, "TryStar") else (ast.Try,) +# Expression node types whose evaluation runs code (and thus may run tests). +# `await`/`yield` can't appear at the module top level, but are included for +# completeness when walking nested expressions. +_ACTIVE_EXPR_NODE_TYPES = (ast.Call, ast.Await, ast.Yield, ast.YieldFrom) + + +def _expression_runs_code(value) -> bool: + """Returns True if evaluating the expression would invoke/await/yield. + + Note this walks the expression as written; it does not try to model what + actually executes (e.g. a call inside a `lambda` body won't run until the + lambda is called). Erring toward "runs code" keeps the safeguard from + flagging valid tests. + """ + if value is None: + return False + return any(isinstance(child, _ACTIVE_EXPR_NODE_TYPES) for child in ast.walk(value)) + def _all_inert(nodes) -> bool: return all(_is_inert_statement(node) for node in nodes) @@ -73,10 +89,15 @@ def _is_inert_statement(node: ast.stmt) -> bool: if isinstance(node, _INERT_NODE_TYPES): return True - # Bare constant expressions: docstrings, `...` (Ellipsis), and similar - # no-op expression statements. - if isinstance(node, ast.Expr) and isinstance(node.value, ast.Constant): - return True + # Assignments are inert unless their value runs code, e.g. + # `exit_code = unittest.main()` actually runs the tests. + if isinstance(node, (ast.Assign, ast.AnnAssign, ast.AugAssign)): + return not _expression_runs_code(node.value) + + # Bare expression statements: docstrings and other no-op expressions are + # inert; a call/await/yield (e.g. `unittest.main()`) runs code. + if isinstance(node, ast.Expr): + return not _expression_runs_code(node.value) # An `if` whose branches are entirely inert, e.g. the very common # `if TYPE_CHECKING:` guard around typing-only imports. The condition itself diff --git a/tests/validate_test_main/validate_test_main_test.py b/tests/validate_test_main/validate_test_main_test.py index 635afa2113..bb3776f3b4 100644 --- a/tests/validate_test_main/validate_test_main_test.py +++ b/tests/validate_test_main/validate_test_main_test.py @@ -123,6 +123,38 @@ def test_foo(self): ) ) + def test_definitions_with_active_assignment_runs_tests(self): + # An assignment whose value runs a call actually runs the tests. + self.assertTrue( + _runs_tests( + """ + import unittest + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + + exit_code = unittest.main() + """ + ) + ) + + def test_definitions_with_active_expression_runs_tests(self): + self.assertTrue( + _runs_tests( + """ + import sys + import unittest + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + + sys.exit(unittest.main()) + """ + ) + ) + def test_if_block_invoking_runner_runs_tests(self): # A runner call inside an `if` body must still count as active. self.assertTrue( From 77562bd4c182cd5d8dfdeac508b9d5391a6a1e49 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 14:28:28 -0700 Subject: [PATCH 09/14] Update python/private/py_executable.bzl Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- python/private/py_executable.bzl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index 4847d67868..25d1c83ee2 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -1364,7 +1364,10 @@ def _maybe_add_test_main_validation(ctx, main_py, output_groups): execution_requirements = execution_requirements, toolchain = EXEC_TOOLS_TOOLCHAIN_TYPE, ) - output_groups["_validation"] = depset([validation_output]) + if "_validation" in output_groups: + output_groups["_validation"] = depset([validation_output], transitive = [output_groups["_validation"]]) + else: + output_groups["_validation"] = depset([validation_output]) def _get_build_info(ctx, cc_toolchain): build_info_files = py_internal.cc_toolchain_build_info_files(cc_toolchain) From f9a417f03745996582e753812826a4b6742d8062 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 14:34:48 -0700 Subject: [PATCH 10/14] Apply suggestions from code review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- python/private/py_test_main_validator.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index bd3c4b8c15..4080a88e44 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -96,6 +96,10 @@ def _is_inert_statement(node: ast.stmt) -> bool: # Bare expression statements: docstrings and other no-op expressions are # inert; a call/await/yield (e.g. `unittest.main()`) runs code. + # Assert statements are inert unless their condition or message runs code. + if isinstance(node, ast.Assert): + return not (_expression_runs_code(node.test) or _expression_runs_code(node.msg)) + if isinstance(node, ast.Expr): return not _expression_runs_code(node.value) From 51c50177b970730f246befd7b35137c7ffd4a9e2 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 14:37:30 -0700 Subject: [PATCH 11/14] test(py_test): cover assert statements in validator Address review feedback: add unit tests for the ast.Assert handling -- an inert `assert True` with a definition is rejected, while an assert whose condition runs a call (e.g. `assert unittest.main()`) is treated as active. --- .../validate_test_main_test.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/validate_test_main/validate_test_main_test.py b/tests/validate_test_main/validate_test_main_test.py index bb3776f3b4..c9426d9150 100644 --- a/tests/validate_test_main/validate_test_main_test.py +++ b/tests/validate_test_main/validate_test_main_test.py @@ -69,6 +69,36 @@ def test_foo(self): ) ) + def test_assert_statement_with_definition_is_rejected(self): + # A bare `assert` with an inert condition runs no tests. + self.assertFalse( + _runs_tests( + """ + assert True + + class MyTest: + def test_foo(self): + pass + """ + ) + ) + + def test_assert_statement_with_active_expression_runs_tests(self): + # An assert whose condition runs a call actually runs the tests. + self.assertTrue( + _runs_tests( + """ + import unittest + + class MyTest(unittest.TestCase): + def test_foo(self): + pass + + assert unittest.main() + """ + ) + ) + @unittest.skipUnless( hasattr(ast, "TypeAlias"), "PEP 695 type aliases require Python 3.12+" ) From c15077a0de4d62fb6a189aab0e763f06497d7eb6 Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 14:42:50 -0700 Subject: [PATCH 12/14] Update python/private/py_test_main_validator.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- python/private/py_test_main_validator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index 4080a88e44..4bcc4889f1 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -108,7 +108,7 @@ def _is_inert_statement(node: ast.stmt) -> bool: # is intentionally ignored: a guard expression doesn't run tests, and a # runner call in a branch body is still detected as active by the recursion. if isinstance(node, ast.If): - return _all_inert(node.body) and _all_inert(node.orelse) + return not _expression_runs_code(node.test) and _all_inert(node.body) and _all_inert(node.orelse) # A `try` (or `try/except*`) whose body, handlers, else, and finally are all # inert, e.g. the common `try: import foo except ImportError: ...` optional From fc625c7c053a0a334c6fb35dbd320c4883d14dcb Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 14:45:39 -0700 Subject: [PATCH 13/14] style: ruff format py_test_main_validator.py --- python/private/py_test_main_validator.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index 4bcc4889f1..a4f2e6272a 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -108,7 +108,11 @@ def _is_inert_statement(node: ast.stmt) -> bool: # is intentionally ignored: a guard expression doesn't run tests, and a # runner call in a branch body is still detected as active by the recursion. if isinstance(node, ast.If): - return not _expression_runs_code(node.test) and _all_inert(node.body) and _all_inert(node.orelse) + return ( + not _expression_runs_code(node.test) + and _all_inert(node.body) + and _all_inert(node.orelse) + ) # A `try` (or `try/except*`) whose body, handlers, else, and finally are all # inert, e.g. the common `try: import foo except ImportError: ...` optional From e96f33f3640ff4ff9bbec4bf62b511808def91be Mon Sep 17 00:00:00 2001 From: Ted Kaplan Date: Tue, 16 Jun 2026 14:47:41 -0700 Subject: [PATCH 14/14] docs: fix stale comment on if-condition handling The if handling now also checks the condition for calls; update the comment which still claimed the condition was ignored. --- python/private/py_test_main_validator.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/private/py_test_main_validator.py b/python/private/py_test_main_validator.py index a4f2e6272a..c070cccf5b 100644 --- a/python/private/py_test_main_validator.py +++ b/python/private/py_test_main_validator.py @@ -103,10 +103,10 @@ def _is_inert_statement(node: ast.stmt) -> bool: if isinstance(node, ast.Expr): return not _expression_runs_code(node.value) - # An `if` whose branches are entirely inert, e.g. the very common - # `if TYPE_CHECKING:` guard around typing-only imports. The condition itself - # is intentionally ignored: a guard expression doesn't run tests, and a - # runner call in a branch body is still detected as active by the recursion. + # An `if` whose condition and branches are entirely inert, e.g. the very + # common `if TYPE_CHECKING:` guard around typing-only imports. A call in the + # condition (e.g. `if feature_enabled():`) or a runner call in a branch body + # makes it active. if isinstance(node, ast.If): return ( not _expression_runs_code(node.test)