Skip to content

ci: remove case_validator.py from ALWAYS_RUN_ALL #1330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sbryngelson merged 1 commit into from
Mar 20, 2026
+5 −3
Merged

ci: remove case_validator.py from ALWAYS_RUN_ALL #1330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion toolchain/mfc/test/coverage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
# are conservatively included (not in cache -> always runs).
# - definitions.py: adding a parameter doesn't affect tests that don't use it;
# the PR's .fpp changes trigger the relevant tests via coverage overlap.
# - case_validator.py: validation only affects user-facing error messages for
# invalid configs, not test outputs (tests use valid configs).
Comment on lines +47 to +48
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== CI/workflow coverage for case_validator changes =="
fd -t f .github/workflows | while read -r f; do
  echo "--- $f ---"
  rg -n -C2 'case_validator\.py|only-changes|mfc\.sh test|pytest|paths:' "$f" || true
done

echo
echo "== Pruning-path behavior in toolchain tests =="
rg -n -C3 'should_run_all_tests|filter_tests_by_coverage|changed_fpp|no-changed-fpp' toolchain/mfc/test

Repository: MFlowCode/MFC

Length of output: 501

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== coverage.py (lines 40-60 to see context) ==="
sed -n '40,60p' toolchain/mfc/test/coverage.py

echo
echo "=== ALWAYS_RUN_ALL definition ==="
rg -n -B2 -A8 'ALWAYS_RUN_ALL\s*=' toolchain/mfc/test/coverage.py

echo
echo "=== should_run_all_tests function ==="
ast-grep --pattern 'def should_run_all_tests($_) {
  $$$
}'

Repository: MFlowCode/MFC

Length of output: 1711

🏁 Script executed:

#!/bin/bash
echo "=== Workflow files in .github ==="
fd . '.github/workflows' -t f

echo
echo "=== CI for case_validator or test filtering ==="
fd . '.github/workflows' -t f -x grep -l 'case_validator\|only-changes\|pruning' {} \; || true

Repository: MFlowCode/MFC

Length of output: 897

🏁 Script executed:

#!/bin/bash
echo "=== case_validator.py: Check what it does ==="
head -100 toolchain/mfc/case_validator.py | tail -50

echo
echo "=== Tests that run case_validator ==="
rg -l 'case_validator|CaseConstraintError' toolchain/mfc/test

Repository: MFlowCode/MFC

Length of output: 2963

🏁 Script executed:

#!/bin/bash
echo "=== input.py: Where case_validator is used ==="
rg -n -B3 -A5 'case_validator|validate|CaseConstraint' toolchain/mfc/run/input.py | head -60

echo
echo "=== Do tests import/use case_validator directly? ==="
rg -n 'import.*case_validator|from.*case_validator' toolchain/mfc/test

echo
echo "=== What exceptions can validator raise? ==="
rg -n 'raise.*Exception|class.*Error' toolchain/mfc/case_validator.py | head -20

Repository: MFlowCode/MFC

Length of output: 2292

🏁 Script executed:

#!/bin/bash
echo "=== test.yml: How it uses coverage pruning ==="
grep -n -A10 'coverage' .github/workflows/test.yml | head -40

echo
echo "=== Does test.yml pass changed files to mfc.sh? ==="
grep -n -B5 -A10 'mfc.sh\|changed' .github/workflows/test.yml | head -50

Repository: MFlowCode/MFC

Length of output: 3382

🏁 Script executed:

#!/bin/bash
echo "=== can_skip_test logic in coverage.py ==="
rg -n -B5 -A15 'def.*skip|should_run|can_skip' toolchain/mfc/test/coverage.py

Repository: MFlowCode/MFC

Length of output: 913

case_validator.py should not be excluded from ALWAYS_RUN_ALL

The comment on lines 47-48 claims validator changes only affect user-facing error messages, but input.py (lines 138-152) shows the validator is called during generate() with validate_constraints(), which can raise CaseConstraintError and fail test execution. Validator logic changes—not just error text—can cause tests to fail if they violate new or modified constraints.

Removing case_validator.py from ALWAYS_RUN_ALL means validator-only changes won't trigger the full test suite, creating a blind spot for regressions. The proposed mitigation (keep validator-only changes in ALWAYS_RUN_ALL unless paired with actual source file changes) is appropriate.

Suggested fix 
 def should_run_all_tests(changed_files: set) -> bool:
+    # Validator changes can alter constraint enforcement and cause test failures.
+    # Keep full-suite testing for validator-only changes to catch regressions.
+    if "toolchain/mfc/case_validator.py" in changed_files and not any(
+        f.startswith("src/") and f.endswith((".fpp", ".f90", ".f"))
+        for f in changed_files
+    ):
+        return True
+
     if changed_files & ALWAYS_RUN_ALL:
         return True
     return any(f.startswith(ALWAYS_RUN_ALL_PREFIXES) for f in changed_files)

Also update the comment on line 47-48 to accurately describe validator behavior.

ALWAYS_RUN_ALL = frozenset(
[
"CMakeLists.txt",
Expand All @@ -56,7 +58,6 @@
"toolchain/mfc/test/case.py",
"toolchain/mfc/test/coverage.py",
"toolchain/mfc/run/input.py",
"toolchain/mfc/case_validator.py",
]
)

Expand Down
5 changes: 3 additions & 2 deletions toolchain/mfc/test/test_coverage_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,9 @@ def test_definitions_py_does_not_trigger_all(self):
def test_input_py_triggers_all(self):
assert should_run_all_tests({"toolchain/mfc/run/input.py"}) is True

def test_case_validator_triggers_all(self):
assert should_run_all_tests({"toolchain/mfc/case_validator.py"}) is True
def test_case_validator_does_not_trigger_all(self):
"""case_validator.py removed: validation only affects error messages, not test outputs."""
assert should_run_all_tests({"toolchain/mfc/case_validator.py"}) is False

def test_cmakelists_triggers_all(self):
assert should_run_all_tests({"CMakeLists.txt"}) is True
Expand Down
Loading