(superset) 03/03: fix(ci): narrow frontend dependency-install path gate, fix stale comment, add docs test

[ru...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rusackas pushed a commit to branch fix/precommit-ci-skip-unneeded-installs
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 828fc1548d59f9f0ab00ec96549667c9215b7156
Author: Evan Rusackas <ev...@rusackas.com>
AuthorDate: Tue Sep 29 11:02:54 2026 -0700

    fix(ci): narrow frontend dependency-install path gate, fix stale comment, add docs test
    
    Narrows FRONTEND_PATTERNS to the union of extensions the frontend
    pre-commit hooks actually lint (js/jsx/ts/tsx/css/scss/sass/json),
    instead of matching every path under superset-frontend/ — a PR that
    only touches e.g. a README under that tree now skips npm ci too.
    Also drops a misleading "files" mode from emit_needs()'s comment (the
    case statement never handled it) and adds a docs-positive subprocess
    test alongside the existing frontend-positive one.
    
    Co-Authored-By: Evan Rusackas <ev...@preset.io>
    Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
 .github/workflows/pre-commit.yml                   |  3 ++-
 scripts/pre_commit_dependency_needs.py             | 17 +++++++------
 .../scripts/pre_commit_dependency_needs_test.py    | 28 ++++++++++++++++++++++
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml
index 9563b71a10b..b6b89a8710b 100644
--- a/.github/workflows/pre-commit.yml
+++ b/.github/workflows/pre-commit.yml
@@ -59,7 +59,8 @@ jobs:
           set -euo pipefail
 
           emit_needs() {
-            # $1: mode (all/none/files-with-$files-set-in-scope)
+            # $1: mode (all/none). The "files" mode is handled separately,
+            # via scripts/pre_commit_dependency_needs.py, not this function.
             case "$1" in
               all)
                 echo "needs_frontend=true" >> "$GITHUB_OUTPUT"
diff --git a/scripts/pre_commit_dependency_needs.py b/scripts/pre_commit_dependency_needs.py
index 87e39c73742..c59425b9ad0 100644
--- a/scripts/pre_commit_dependency_needs.py
+++ b/scripts/pre_commit_dependency_needs.py
@@ -19,10 +19,10 @@ needed for a given set of changed files.
 
 Every hook that actually needs those installs is already path-gated in
 .pre-commit-config.yaml (oxfmt-frontend, oxlint-frontend, custom-rules-frontend,
-stylelint-frontend, type-checking-frontend on `superset-frontend/`; oxlint-docs
-on `docs/*.{js,jsx,ts,tsx}`), so a changed-file list that matches none of these
-patterns means npm ci / yarn install can be skipped without any hook missing
-its dependencies.
+stylelint-frontend, type-checking-frontend on `superset-frontend/*.{js,jsx,ts,
+tsx,css,scss,sass,json}`; oxlint-docs on `docs/*.{js,jsx,ts,tsx}`), so a
+changed-file list that matches none of these patterns means npm ci / yarn
+install can be skipped without any hook missing its dependencies.
 
 A previous inline-bash version of this (`printf '%s\\n' "$files" | grep -q
 ...`) risked a false "not needed" result: under `set -o pipefail`, grep -q's
@@ -37,9 +37,12 @@ import re
 import sys
 from typing import List
 
-# Mirrors oxfmt-frontend/oxlint-frontend/custom-rules-frontend/
-# stylelint-frontend/type-checking-frontend's own `files:` prefix.
-FRONTEND_PATTERNS: List[str] = [r"^superset-frontend/"]
+# Mirrors the union of oxfmt-frontend/oxlint-frontend/custom-rules-frontend/
+# stylelint-frontend/type-checking-frontend's own `files:` patterns (oxfmt
+# additionally covers css/scss/sass/json; the rest are js/jsx/ts/tsx only).
+FRONTEND_PATTERNS: List[str] = [
+    r"^superset-frontend/.*\.(js|jsx|ts|tsx|css|scss|sass|json)$"
+]
 # Mirrors oxlint-docs's own `files:` pattern exactly. Most docs contributions
 # are prose (.md/.mdx), which this correctly treats as not needing the install.
 DOCS_PATTERNS: List[str] = [r"^docs/.*\.(js|jsx|ts|tsx)$"]
diff --git a/tests/unit_tests/scripts/pre_commit_dependency_needs_test.py b/tests/unit_tests/scripts/pre_commit_dependency_needs_test.py
index 35000424d4b..0029b6a135b 100644
--- a/tests/unit_tests/scripts/pre_commit_dependency_needs_test.py
+++ b/tests/unit_tests/scripts/pre_commit_dependency_needs_test.py
@@ -36,6 +36,16 @@ def test_pure_frontend_change_needs_frontend_only() -> None:
     assert needs.needs_docs(files) is False
 
 
+def test_frontend_prose_change_needs_neither() -> None:
+    """None of the frontend hooks (oxfmt/oxlint/custom-rules/stylelint/
+    type-checking) fire on a superset-frontend/ file outside their own
+    js/jsx/ts/tsx/css/scss/sass/json extensions, so e.g. a README-only PR
+    must not pay for the npm ci."""
+    files = ["superset-frontend/README.md"]
+    assert needs.needs_frontend(files) is False
+    assert needs.needs_docs(files) is False
+
+
 def test_mixed_backend_and_frontend_change_needs_frontend() -> None:
     files = ["superset/foo.py", "superset-frontend/src/bar.ts"]
     assert needs.needs_frontend(files) is True
@@ -101,3 +111,21 @@ def test_main_writes_expected_outputs_to_github_output(tmp_path: Path) -> None:
     output = output_file.read_text()
     assert "needs_frontend=true" in output
     assert "needs_docs=false" in output
+
+
+def test_main_writes_needs_docs_true_for_a_docs_js_change(tmp_path: Path) -> None:
+    """Same subprocess/main() interface as above, but for a docs-only change
+    that should flip needs_docs on and leave needs_frontend off."""
+    output_file = tmp_path / "github_output.txt"
+    result = subprocess.run(  # noqa: S603
+        [sys.executable, str(SCRIPT_PATH)],
+        input="docs/src/components/Foo.tsx\n",
+        capture_output=True,
+        text=True,
+        env={"GITHUB_OUTPUT": str(output_file), "PATH": "/usr/bin:/bin"},
+        check=True,
+    )
+    assert result.returncode == 0
+    output = output_file.read_text()
+    assert "needs_frontend=false" in output
+    assert "needs_docs=true" in output