rusackas commented on code in PR #44738: URL: https://github.com/apache/superset/pull/44738#discussion_r4136707390
########## scripts/pre_commit_dependency_needs.py: ########## @@ -0,0 +1,80 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. +"""Decides whether pre-commit.yml's frontend/docs dependency installs are +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. + +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 +early exit on a match can SIGPIPE a producer still mid-write for a large +enough list, and that failure surfaces as the `if` condition's own status. +Plain Python reading stdin to EOF has no equivalent early-exit-close +behavior, so it isn't exposed to that failure mode at all. +""" + +import os +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/"] Review Comment: Good catch, narrowed `FRONTEND_PATTERNS` to the extensions the frontend hooks actually lint (ad5acced). A superset-frontend/ README-only PR skips npm ci now too. ########## .github/workflows/pre-commit.yml: ########## @@ -95,9 +58,24 @@ jobs: run: | set -euo pipefail + emit_needs() { + # $1: mode (all/none/files-with-$files-set-in-scope) Review Comment: Good catch, dropped `files` from that comment in ad5acced. That mode is handled by `pre_commit_dependency_needs.py`, not `emit_needs()`. ########## tests/unit_tests/scripts/pre_commit_dependency_needs_test.py: ########## @@ -0,0 +1,103 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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 subprocess +import sys +from pathlib import Path + +from scripts import pre_commit_dependency_needs as needs + +REPO_ROOT = Path(__file__).resolve().parents[3] +SCRIPT_PATH = REPO_ROOT / "scripts" / "pre_commit_dependency_needs.py" + + +def test_pure_backend_change_needs_neither() -> None: + files = ["superset/commands/database/update.py", "tests/unit_tests/foo.py"] + assert needs.needs_frontend(files) is False + assert needs.needs_docs(files) is False + + +def test_pure_frontend_change_needs_frontend_only() -> None: + files = ["superset-frontend/src/preamble.ts"] + assert needs.needs_frontend(files) is True + 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 + assert needs.needs_docs(files) is False + + +def test_docs_prose_change_needs_neither() -> None: + """oxlint-docs only lints JS/TS under docs/, so a prose-only docs PR + (the common case) must not pay for the yarn install.""" + files = ["docs/docs/some-page.mdx", "docs/docs/another.md"] + assert needs.needs_frontend(files) is False + assert needs.needs_docs(files) is False + + +def test_docs_js_change_needs_docs_only() -> None: + files = ["docs/src/components/Foo.tsx"] + assert needs.needs_frontend(files) is False + assert needs.needs_docs(files) is True + + +def test_empty_file_list_needs_neither() -> None: + assert needs.needs_frontend([]) is False + assert needs.needs_docs([]) is False + + +def test_similarly_named_path_outside_the_real_directory_does_not_match() -> None: + """A path that merely contains "superset-frontend" or "docs" as a + substring, without it being the actual path prefix, must not match -- + _matches_any anchors with re.match, not a bare substring search.""" + files = ["scripts/not-superset-frontend-related.py", "some/docs-ish/file.py"] + assert needs.needs_frontend(files) is False + assert needs.needs_docs(files) is False + + +def test_large_file_list_with_an_early_frontend_match() -> None: + """Regression test for the bug a previous `printf | grep -q` bash + implementation had: under `pipefail`, grep -q's early exit on a match + could SIGPIPE a producer still mid-write for a large list, corrupting + the result. A large list (order of magnitude past a typical 64KB pipe + buffer if this were piped) with the real match on the first line must + still resolve correctly -- plain Python has no equivalent failure mode, + which this pins.""" + files = ["superset-frontend/early-match.ts"] + [ + f"path/to/file_{i}.py" for i in range(5000) + ] + assert needs.needs_frontend(files) is True + + +def test_main_writes_expected_outputs_to_github_output(tmp_path: Path) -> None: Review Comment: Added a docs-positive case through the same `main()`/`$GITHUB_OUTPUT` subprocess interface in ad5acced, plus one for a superset-frontend/ file outside the lint extensions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
