diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 964fb5abc..2bd0cc133 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -24,3 +24,9 @@ jobs: python-version: "3.12" cache: 'pip' - uses: pre-commit/action@v3.0.1 + # Don't run the pytest mark check against everything in the CI + # for now since a lot of files are still missing them. This check + # is used to help contributors remember to add the marks when they + # commit. + env: + SKIP: check-test-ci-coverage diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6db093e0e..be649785d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -54,6 +54,13 @@ repos: verbose: true stages: [commit-msg] + - id: check-test-ci-coverage + name: Ensure test files have pytestmarks + entry: python tools/pre_commit/check_test_marks.py + language: python + types: [python] + files: ^tests/ + # Keep `suggestion` last - id: suggestion name: Suggestion diff --git a/tools/pre_commit/check_test_marks.py b/tools/pre_commit/check_test_marks.py new file mode 100644 index 000000000..9a727ab72 --- /dev/null +++ b/tools/pre_commit/check_test_marks.py @@ -0,0 +1,111 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright contributors to the vLLM project +"""Run a pre-commit hook that fails if test files are modified or added +that (probably) never run in the CI. For now, this means that every tests file +needs to have a CI level marker (e.g., core_model, advanced_model, full_model, +etc) and hardware mark / helper so that we ensure mutated tests will actually +be selected as long as there are pytest commands pointing at the right paths. +""" + +import os +import re +import sys + +# CI level markers +LEVEL_MARKERS = ("core_model", "advanced_model", "full_model", "slow") + +# Hardware markers. These are platforms and resources that CI filters on. +# NOTE: If new hardware marks are added to pyproject.toml etc, we need +# to add them here as well. +HARDWARE_MARKERS = ( + "cpu", + "gpu", + "cuda", + "rocm", + "xpu", + "npu", + "musa", + "H100", + "L4", + "MI325", + "B60", + "S5000", + "A2", + "A3", +) + +# Helpers from tests/helpers/mark.py that auto-apply hardware marks. +HARDWARE_HELPERS = ("hardware_test", "hardware_marks") + +# Match mark.X since we could also do `from pytest import mark`. +# \b prevents matching prefixes (e.g., mark.slow vs mark.slow_test). +LEVEL_RE = re.compile(r"mark\.(?:" + "|".join(LEVEL_MARKERS) + r")\b") +HARDWARE_RE = re.compile(r"mark\.(?:" + "|".join(HARDWARE_MARKERS) + r")\b") +HELPER_RE = re.compile(r"(?:" + "|".join(HARDWARE_HELPERS) + r")\s*\(") + +MISSING_LEVEL_MARKER = "Level" +MISSING_HARDWARE_MARKER = "Hardware" + +# Check if a file is located under tests/ and matches test_.py +# or _test.py, since pytest technically collects on both. +# Note that we use the former everywhere in this repo by convention. +TEST_FILE_RE = re.compile(r"^tests/(?:.*/)?(?:test_[^/]*\.py$|[^/]*_test\.py$)") + + +def is_test_file(path: str) -> bool: + """Determine whether or not a path is pointing at a test file or not.""" + return bool(TEST_FILE_RE.match(path)) + + +def read_test_file(path: str) -> str | None: + """Read a test file's contents, or return None if it doesn't exist.""" + if not os.path.isfile(path): + return None + with open(path, encoding="utf-8") as f: + return f.read() + + +def has_level_marker(contents: str) -> bool: + """Check if file contents contain at least one CI level marker.""" + return bool(LEVEL_RE.search(contents)) + + +def has_hardware_marker(contents: str) -> bool: + """Check if file contents contain any hardware marker or helpers.""" + return bool(HARDWARE_RE.search(contents) or HELPER_RE.search(contents)) + + +def get_files_missing_markers( + staged_files: list[str], +) -> dict[str, list[str]]: + """Return a dict mapping file path to list of missing marker types.""" + results: dict[str, list[str]] = {} + for path in staged_files: + if is_test_file(path) and (contents := read_test_file(path)) is not None: + missing = [] + if not has_level_marker(contents): + missing.append(MISSING_LEVEL_MARKER) + if not has_hardware_marker(contents): + missing.append(MISSING_HARDWARE_MARKER) + if missing: + results[path] = missing + return results + + +if __name__ == "__main__": + missing = get_files_missing_markers(sys.argv[1:]) + + if missing: + file_lines = "\n".join(f" - {path} [{' and '.join(problems)}]" for path, problems in missing.items()) + print( + "\033[91merror:\033[0m test files are missing pytest marks " + "required for Buildkite CI collection.\n\n" + f"Level marks, e.g.: {', '.join(LEVEL_MARKERS[:4])}\n" + f"Hardware marks, e.g.: {', '.join(HARDWARE_MARKERS[:4])}, ...\n" + f" or helpers: {', '.join(HARDWARE_HELPERS)}\n\n" + "The following files are missing marks:\n" + f"{file_lines}\n\n" + "To skip: SKIP=check-test-ci-coverage git commit ..." + ) + sys.exit(1)