Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions .github/scripts/build-global-mapping.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
#!/usr/bin/env python3

import json
import sys
import os
import ast
import time

def extract_dependencies_and_functions(test_file):
"""Extract dependencies and test functions from the test file"""
dependencies = []
test_functions = []

try:
with open(test_file, "r", encoding="utf-8") as f:
content = f.read()

if len(content) > 1024000: # 1MB restriction
print(f"Skipping large file: {test_file}", file=sys.stderr)
return None

tree = ast.parse(content)

for node in ast.walk(tree):
# test files
if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"):
test_functions.append(node.name)
Comment on lines +26 to +27

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Missing ast.AsyncFunctionDef — async test functions won't be collected.

If any test files define async def test_*(...), those functions won't appear in the mapping's test_functions list.

🔧 Proposed fix
-            if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"):
+            if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith("test_"):
                 test_functions.append(node.name)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"):
test_functions.append(node.name)
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith("test_"):
test_functions.append(node.name)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/build-global-mapping.py around lines 26 - 27, The test
collection logic in build_global_mapping only handles ast.FunctionDef, so async
tests named test_* are skipped. Update the AST scan to also recognize
ast.AsyncFunctionDef in the same collection path that appends to test_functions,
so async test functions are included in the mapping output alongside regular
test functions.


# collect: import deepmd.*
if isinstance(node, ast.Import):
for alias in node.names:
if alias.name.startswith("deepmd"):
module_path = alias.name.replace(".", "/") + ".py"
dep = "source/" + module_path
dependencies.append(dep)

# collect: from deepmd.xxx import yyy
elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"):
base = node.module.replace(".", "/")
if node.names:
for alias in node.names:
if alias.name == "*":
dep = f"source/{base}.py"
else:
dep = f"source/{base}/{alias.name}.py"

if not os.path.exists(dep):
dep = f"source/{base}.py"

dependencies.append(dep)
Comment on lines +30 to +50

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Package import resolution doesn't handle __init__.py files, causing missed test selections.

import deepmd maps to source/deepmd.py (line 33-34), but deepmd is a package — the actual file is source/deepmd/__init__.py. Changes to __init__.py won't match this path, so dependent tests won't be selected.

Similarly, from deepmd.xxx import yyy where yyy is a class/function defined in a package's __init__.py falls back only to source/deepmd/xxx.py (line 48), missing source/deepmd/xxx/__init__.py.

🔧 Proposed fix for package import resolution
             if isinstance(node, ast.Import):
                 for alias in node.names:
                     if alias.name.startswith("deepmd"):
                         module_path = alias.name.replace(".", "/")
-                        dep = "source/" + module_path + ".py"
-                        dependencies.append(dep)
+                        # Package: try __init__.py first, then module .py
+                        dep_pkg = f"source/{module_path}/__init__.py"
+                        dep_mod = f"source/{module_path}.py"
+                        if os.path.exists(dep_pkg):
+                            dependencies.append(dep_pkg)
+                        else:
+                            dependencies.append(dep_mod)
             
             elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"):
                 base = node.module.replace(".", "/")
                 if node.names:
                     for alias in node.names:
                         if alias.name == "*":
                             dep = f"source/{base}.py"
                         else:
                             dep = f"source/{base}/{alias.name}.py"
                         
                         if not os.path.exists(dep):
-                            dep = f"source/{base}.py"
+                            dep_init = f"source/{base}/__init__.py"
+                            if os.path.exists(dep_init):
+                                dep = dep_init
+                            else:
+                                dep = f"source/{base}.py"
                         
                         dependencies.append(dep)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if isinstance(node, ast.Import):
for alias in node.names:
if alias.name.startswith("deepmd"):
module_path = alias.name.replace(".", "/") + ".py"
dep = "source/" + module_path
dependencies.append(dep)
# collect: from deepmd.xxx import yyy
elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"):
base = node.module.replace(".", "/")
if node.names:
for alias in node.names:
if alias.name == "*":
dep = f"source/{base}.py"
else:
dep = f"source/{base}/{alias.name}.py"
if not os.path.exists(dep):
dep = f"source/{base}.py"
dependencies.append(dep)
if isinstance(node, ast.Import):
for alias in node.names:
if alias.name.startswith("deepmd"):
module_path = alias.name.replace(".", "/")
# Package: try __init__.py first, then module .py
dep_pkg = f"source/{module_path}/__init__.py"
dep_mod = f"source/{module_path}.py"
if os.path.exists(dep_pkg):
dependencies.append(dep_pkg)
else:
dependencies.append(dep_mod)
# collect: from deepmd.xxx import yyy
elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"):
base = node.module.replace(".", "/")
if node.names:
for alias in node.names:
if alias.name == "*":
dep = f"source/{base}.py"
else:
dep = f"source/{base}/{alias.name}.py"
if not os.path.exists(dep):
dep_init = f"source/{base}/__init__.py"
if os.path.exists(dep_init):
dep = dep_init
else:
dep = f"source/{base}.py"
dependencies.append(dep)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/build-global-mapping.py around lines 30 - 50, The
import-to-dependency mapping in build-global-mapping.py does not account for
package modules resolved through __init__.py, so tests can be missed for
package-level changes. Update the logic in the AST import handling so imports
like import deepmd and from deepmd.xxx import yyy prefer source/.../__init__.py
when that package file exists, and only fall back to source/... .py when
appropriate. Keep the fix localized to the dependency collection branch that
builds dep and checks os.path.exists, and make sure both the ast.Import and
ast.ImportFrom paths resolve package __init__.py correctly.

except SyntaxError:

Check notice

Code scanning / CodeQL

Empty except Note

'except' clause does nothing but pass and there is no explanatory comment.
pass
Comment on lines +51 to +52

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

SyntaxError is silently swallowed — malformed test files vanish from the mapping.

A test file with a syntax error returns empty lists, gets filtered out at line 90, and won't appear in the mapping. If the file is later fixed but the mapping cache isn't rebuilt, tests for that file won't be selected.

At minimum, log the error so it's visible in CI output.

🔧 Proposed fix
-    except SyntaxError:
-        pass
+    except SyntaxError as e:
+        print(f"Syntax error parsing {test_file}: {e}", file=sys.stderr)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except SyntaxError:
pass
except SyntaxError as e:
print(f"Syntax error parsing {test_file}: {e}", file=sys.stderr)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/build-global-mapping.py around lines 51 - 52, The
SyntaxError handling in build-global-mapping.py is swallowing malformed test
files, which causes them to disappear from the mapping without any visibility.
Update the exception path in the file-scanning logic to log the SyntaxError with
enough context (at least the affected file name) instead of silently passing, so
CI output shows when a file is skipped; keep the rest of the mapping flow
unchanged around the existing parsing and filtering logic.

except Exception as e:
print(f"Error parsing {test_file}: {e}", file=sys.stderr)

return {
"dependencies": sorted(set(dependencies)),
"test_functions": sorted(set(test_functions))
}

def main():
# Check if the test file list exists
if not os.path.exists("test_files.txt"):
print("Error: test_files.txt not found!", file=sys.stderr)
sys.exit(1)

# read test file list
with open("test_files.txt", "r") as f:
test_files = [line.strip() for line in f if line.strip()]

mapping = {}
processed = 0
total = len(test_files)
start_time = time.time()

for test_file in test_files:
processed += 1
if processed % 100 == 0:
elapsed = time.time() - start_time
print(f"Processed {processed}/{total} files ({elapsed:.1f}s)", file=sys.stderr)

if not os.path.exists(test_file):
continue

result = extract_dependencies_and_functions(test_file)
if result:
mapping[test_file] = result

# Clear empty entries
mapping = {k: v for k, v in mapping.items()
if v["dependencies"] or v["test_functions"]}

# write global_test_mapping
with open(".global_test_mapping.json", "w") as f:
json.dump(mapping, f, indent=2, sort_keys=True)

elapsed = time.time() - start_time
print(f" Built global mapping with {len(mapping)} test files in {elapsed:.1f}s", file=sys.stderr)

if __name__ == "__main__":
main()
190 changes: 190 additions & 0 deletions .github/scripts/detect_affected_tests.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
#!/usr/bin/env python3
"""
Detect affected tests based on changed files and global mapping.
"""
import json
import os
import sys
import subprocess

def debug(msg):
"""Print debug message to stderr so it doesn't mix with output parsing."""
print(f"DEBUG: {msg}", file=sys.stderr)

def get_changed_files_from_git():
"""Fallback: get changed files directly from git diff."""
debug("Attempting to get changed files from git diff...")

# Get base SHA from environment (set by workflow)
base_sha = os.environ.get("COMPARE_SHA")
if not base_sha:
debug("COMPARE_SHA not found in environment")
# Try alternative environment variables
base_sha = os.environ.get("GITHUB_BASE_SHA")
if not base_sha:
# Last resort: use HEAD~1
base_sha = "HEAD~1"
debug(f"Using fallback base SHA: {base_sha}")

# Get current SHA
current_sha = os.environ.get("GITHUB_SHA", "HEAD")
debug(f"Git diff range: {base_sha}..{current_sha}")

try:
# Run git diff to get changed files
result = subprocess.run(
["git", "diff", "--name-only", base_sha, current_sha],
capture_output=True,
text=True,
check=True
)

changed_files = [line.strip() for line in result.stdout.splitlines() if line.strip()]
debug(f"Git diff returned {len(changed_files)} changed files")
for cf in changed_files:
debug(f" - {cf}")
return changed_files

except subprocess.CalledProcessError as e:
debug(f"Git diff failed: {e.stderr}")
return []
except Exception as e:
debug(f"Error running git diff: {e}")
return []

def main():
# ========== 1. Read changed files (Priority order) ==========
changed_files = []

# Priority 1: Read from file (most reliable, passed from workflow)
changed_files_path = os.environ.get("CHANGED_FILES_PATH")
debug(f"CHANGED_FILES_PATH={changed_files_path}")

if changed_files_path and os.path.exists(changed_files_path):
debug(f"Reading changed files from file: {changed_files_path}")
try:
with open(changed_files_path, "r") as f:
content = f.read()
debug(f"Raw file content: {repr(content)}")
changed_files = [line.strip() for line in content.splitlines() if line.strip()]
except Exception as e:
debug(f"Failed to read file: {e}")

# Priority 2: Read from single-line environment variable
if not changed_files:
raw = os.environ.get("CHANGED_FILES_SINGLELINE", "")
debug(f"CHANGED_FILES_SINGLELINE={repr(raw)}")
if raw:
changed_files = raw.split()

# Priority 3: Fallback to old CHANGED_FILES variable
if not changed_files:
raw = os.environ.get("CHANGED_FILES", "")
debug(f"CHANGED_FILES={repr(raw)}")
if raw:
# Handle both newline and space separated formats
if "\n" in raw:
changed_files = [line.strip() for line in raw.splitlines() if line.strip()]
else:
changed_files = raw.split()

# Priority 4: Fallback to git diff (NEW - most reliable when env vars fail)
if not changed_files:
debug("No changed files found via environment variables, trying git diff...")
changed_files = get_changed_files_from_git()

debug(f"Parsed changed files ({len(changed_files)}):")
for cf in changed_files:
debug(f" - {cf}")

# If no changed files, trigger full test (safe default)
if not changed_files:
debug("No changed files found after all attempts, falling back to full test")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)

# ========== 2. Load global mapping ==========
mapping_path = ".global_test_mapping.json"
debug(f"Loading mapping from: {mapping_path}")

if not os.path.exists(mapping_path):
debug(f"Mapping file not found, falling back to full test")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)

try:
with open(mapping_path, "r") as f:
mapping = json.load(f)
except json.JSONDecodeError as e:
debug(f"Invalid JSON in mapping file: {e}")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)

# Filter out metadata field
mapping = {k: v for k, v in mapping.items() if k != "metadata"}
debug(f"Loaded {len(mapping)} test mappings (excluded metadata)")

# ========== 3. Match changed files to tests ==========
affected_tests = set()
new_source_files_without_tests = []

for cf in changed_files:
debug(f"Processing changed file: {cf}")

# Case A: Changed file is a test file itself
if cf.startswith("source/tests/"):
debug(f"Changed file is a test file, adding to affected tests: {cf}")
affected_tests.add(cf)
continue

# Case B: Changed file is a workflow/config file - trigger full test
if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] Non-Python / C++ / build changes are not covered. Besides the workflow/scripts handled here, changes under source/{lib,op,api_cc,api_c,lmp}/**, pyproject.toml, CMakeLists.txt, source/install/**, and test data/fixtures (*.pth, *.pbtxt, *.npy) match no case and fall through to skip_all=true — yet many Python tests validate behavior through the compiled ops. These need an analogous force-FULL branch. (Covers CodeRabbit's pyproject.toml/CMakeLists.txt point too.)

debug(f"Changed workflow/script file {cf}, triggering full test")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)

# Case C: Changed file is a source file, look for dependent tests
if cf.startswith("source/deepmd/"):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] Wrong path prefix — real source changes never match, so normal PRs skip all tests.

The package lives at repo root deepmd/ (there is no source/deepmd/; the workflow itself runs pytest --cov=deepmd). But git diff --name-only emits source changes as deepmd/..., while this check tests source/deepmd/. A PR that only edits deepmd/** therefore matches none of Case A/B/C, both affected_tests and new_source_files_without_tests stay empty, and we hit the elseskip_all=true → the entire Python suite is skipped and pass goes green. The same wrong prefix is in build-global-mapping.py (dep = "source/" + module_path) and in the workflow's new_source_files / syntax-check greps (^source/deepmd/). Note the asymmetry: tests are correctly source/tests/, only the package prefix is wrong. Fix: use deepmd/ everywhere for the package.

found_match = False
for test_file, meta in mapping.items():
dependencies = meta.get("dependencies", [])
if cf in dependencies:
debug(f"Found test {test_file} depends on {cf}, adding to affected tests")
affected_tests.add(test_file)
found_match = True

if not found_match:
debug(f"Changed source file {cf} has no test coverage, marking for full test")
new_source_files_without_tests.append(cf)

Comment on lines +137 to +167

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Unrecognized changed files are silently ignored, leading to skip_all=true and tests being skipped.

Changed files outside source/tests/, .github/workflows/, .github/scripts/, and source/deepmd/ (e.g., pyproject.toml, source/setup.py, CMakeLists.txt, requirements.txt) don't match any case. If all changed files fall into this gap, both affected_tests and new_source_files_without_tests remain empty, hitting the else branch at line 182 which sets skip_all=trueall tests are skipped.

This is a safe-default inversion: unrecognized changes should trigger full tests, not skip them.

🔧 Proposed fix — add a catch-all default case
     for cf in changed_files:
         debug(f"Processing changed file: {cf}")
         
         # Case A: Changed file is a test file itself
         if cf.startswith("source/tests/"):
             debug(f"Changed file is a test file, adding to affected tests: {cf}")
             affected_tests.add(cf)
             continue
         
         # Case B: Changed file is a workflow/config file - trigger full test
         if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"):
             debug(f"Changed workflow/script file {cf}, triggering full test")
             print("selected_paths=source/tests")
             print("skip_all=false")
             print("need_full_test=true")
             sys.exit(0)
         
         # Case C: Changed file is a source file, look for dependent tests
         if cf.startswith("source/deepmd/"):
             found_match = False
             for test_file, meta in mapping.items():
                 dependencies = meta.get("dependencies", [])
                 if cf in dependencies:
                     debug(f"Found test {test_file} depends on {cf}, adding to affected tests")
                     affected_tests.add(test_file)
                     found_match = True
             
             if not found_match:
                 debug(f"Changed source file {cf} has no test coverage, marking for full test")
                 new_source_files_without_tests.append(cf)
+            continue
+        
+        # Case D: Unrecognized file — safe default is full test
+        debug(f"Unrecognized changed file {cf}, triggering full test")
+        print("selected_paths=source/tests")
+        print("skip_all=false")
+        print("need_full_test=true")
+        sys.exit(0)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for cf in changed_files:
debug(f"Processing changed file: {cf}")
# Case A: Changed file is a test file itself
if cf.startswith("source/tests/"):
debug(f"Changed file is a test file, adding to affected tests: {cf}")
affected_tests.add(cf)
continue
# Case B: Changed file is a workflow/config file - trigger full test
if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"):
debug(f"Changed workflow/script file {cf}, triggering full test")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)
# Case C: Changed file is a source file, look for dependent tests
if cf.startswith("source/deepmd/"):
found_match = False
for test_file, meta in mapping.items():
dependencies = meta.get("dependencies", [])
if cf in dependencies:
debug(f"Found test {test_file} depends on {cf}, adding to affected tests")
affected_tests.add(test_file)
found_match = True
if not found_match:
debug(f"Changed source file {cf} has no test coverage, marking for full test")
new_source_files_without_tests.append(cf)
for cf in changed_files:
debug(f"Processing changed file: {cf}")
# Case A: Changed file is a test file itself
if cf.startswith("source/tests/"):
debug(f"Changed file is a test file, adding to affected tests: {cf}")
affected_tests.add(cf)
continue
# Case B: Changed file is a workflow/config file - trigger full test
if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"):
debug(f"Changed workflow/script file {cf}, triggering full test")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)
# Case C: Changed file is a source file, look for dependent tests
if cf.startswith("source/deepmd/"):
found_match = False
for test_file, meta in mapping.items():
dependencies = meta.get("dependencies", [])
if cf in dependencies:
debug(f"Found test {test_file} depends on {cf}, adding to affected tests")
affected_tests.add(test_file)
found_match = True
if not found_match:
debug(f"Changed source file {cf} has no test coverage, marking for full test")
new_source_files_without_tests.append(cf)
continue
# Case D: Unrecognized file — safe default is full test
debug(f"Unrecognized changed file {cf}, triggering full test")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/detect_affected_tests.py around lines 137 - 167, The change
detection logic in detect_affected_tests.py has a gap where files that do not
match the existing source/test/workflow paths are ignored, which can incorrectly
lead to skip_all=true. Update the main changed_files handling in the
affected-tests selection flow to add a catch-all default case after the existing
source/deepmd/ and config/workflow checks, so unrecognized paths like
build/config/dependency files are treated as requiring full test execution. Use
the existing affected_tests, new_source_files_without_tests, and the final
skip_all/full-test decision logic to ensure the fallback routes to
need_full_test=true instead of silently skipping.

# ========== 4. Output results ==========
if new_source_files_without_tests:
# New source files without test coverage -> full test
debug(f"New source files without test coverage: {new_source_files_without_tests}")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
elif affected_tests:
# Found affected tests -> run only those
selected = " ".join(sorted(affected_tests))
debug(f"Affected tests found: {selected}")
print(f"selected_paths={selected}")
print("skip_all=false")
print("need_full_test=false")
else:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] Safe-default is inverted (fail-open). When no rule matches, the selector currently skips tests. A merge gate must fail-closed: anything not provably irrelevant → FULL. Please remove the free skip_all=true path; only allow skipping for an explicit docs-only allowlist (e.g. all changed files match *.md). Every other bucket — unmatched paths, non-Python, data/fixtures, build/CI config, new files, deletes/renames, missing or unparsable mapping — must set need_full_test=true.

# No affected tests and no new source files -> skip tests
debug("No affected tests found and no new source files")
print("selected_paths=")
print("skip_all=true")
print("need_full_test=false")

if __name__ == "__main__":
main()
Loading
Loading