mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-08-11 00:30:12 -04:00
+ ci: shard backend tests 4-way + drop -v for ~3.5x wall-clock speedup Root cause of the 4 CI failures on PR #1514 (all in test_print_start_assigns_printer_id_to_vp_archive.py + test_timelapse_baseline_restart_recovery.py): test_all_modules_importable in test_code_quality.py was deleting backend.app.main from sys.modules and re-importing it via importlib.import_module. That created NEW module-level dicts (_timelapse_baselines, _expected_prints, _active_prints, …) and re-ran root_logger.addHandler — hence the duplicate log lines at the same microsecond in captured stderr. Any sibling test that bound those names via "from backend.app.main import _timelapse_baselines" before the reimport now held a reference to the OLD dict; production code (reached via "from backend.app.main import on_print_start") resolved the symbol through the NEW module instance. Production mutated the new dict, the test read the old one, the assertion saw None / un-mutated mock_archive. Locally with -n 30, xdist load-balanced test_code_quality.py to a different worker process so the collision never happened (which is why the suite was green for me). CI's -n auto = -n 2 on ubuntu-latest made the collision deterministic. Fix: drop the "del sys.modules[name]" step. importlib.import_module already returns the cached module if cached, or runs the import machinery if not — either way, any import-time error surfaces. The "fresh import" framing was theatre; in practice every module in the list is already imported by other tests/fixtures before this test runs, so we were never actually getting a fresh import anyway — just destruction. CI workflow tightening (separate concern, same PR since both touch the test infrastructure): - Dropped -v from the pytest invocation. 5300+ "PASSED foo::bar" lines per worker were eating ~30-60s of stdout I/O on 2-vCPU runners. --tb=short is sufficient for failure context. - Sharded backend-tests into a 4-way matrix via pytest-split (new dev dep). Each shard runs ~1326 tests in ~95s on a 2-vCPU runner; all 4 run in parallel so wall-clock drops from 362s -> ~100s. - fail-fast: false on the matrix so a single failing shard doesn't hide failures in the other three — PRs see the complete failure picture in one push.
289 lines
11 KiB
Python
289 lines
11 KiB
Python
"""
|
|
Code quality tests for BamBuddy backend.
|
|
|
|
These tests check for common anti-patterns and code quality issues
|
|
that could cause runtime errors but aren't caught by normal tests.
|
|
"""
|
|
|
|
import ast
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
# Get the backend source directory
|
|
BACKEND_DIR = Path(__file__).parent.parent.parent / "app"
|
|
|
|
|
|
# Safe imports that are commonly re-imported in functions without issues
|
|
# These are typically imported at the START of a function, not midway through
|
|
SAFE_REIMPORT_NAMES = {
|
|
"logging",
|
|
"re",
|
|
"os",
|
|
"sys",
|
|
"json",
|
|
"Path",
|
|
"datetime",
|
|
"timedelta",
|
|
"asyncio",
|
|
"time",
|
|
"typing",
|
|
"Optional",
|
|
"List",
|
|
"Dict",
|
|
"Any",
|
|
"Union",
|
|
}
|
|
|
|
|
|
class DangerousImportVisitor(ast.NodeVisitor):
|
|
"""AST visitor that detects dangerous import patterns.
|
|
|
|
Specifically looks for cases where:
|
|
1. A name is imported at module level
|
|
2. The same name is imported locally in a function
|
|
3. The name is USED before the local import in that function
|
|
|
|
This pattern causes 'cannot access local variable' errors.
|
|
"""
|
|
|
|
def __init__(self):
|
|
self.module_imports: set[str] = set()
|
|
self.dangerous_imports: list[tuple[str, int, str, int]] = [] # (name, import_line, function, first_use_line)
|
|
self.current_function: str | None = None
|
|
self.function_start_line: int = 0
|
|
self.in_function = False
|
|
|
|
def visit_Import(self, node: ast.Import):
|
|
for alias in node.names:
|
|
name = alias.asname or alias.name
|
|
if not self.in_function:
|
|
self.module_imports.add(name)
|
|
self.generic_visit(node)
|
|
|
|
def visit_ImportFrom(self, node: ast.ImportFrom):
|
|
for alias in node.names:
|
|
name = alias.asname or alias.name
|
|
if not self.in_function:
|
|
self.module_imports.add(name)
|
|
self.generic_visit(node)
|
|
|
|
def _check_function(self, node):
|
|
"""Check a function for dangerous import patterns."""
|
|
if not self.in_function:
|
|
return
|
|
|
|
# Skip safe reimports
|
|
# Collect all local imports in this function
|
|
local_imports: dict[str, int] = {} # name -> line number
|
|
name_uses: dict[str, int] = {} # name -> first use line number
|
|
|
|
for child in ast.walk(node):
|
|
# Find local imports
|
|
if isinstance(child, (ast.Import, ast.ImportFrom)):
|
|
for alias in child.names:
|
|
name = alias.asname or alias.name
|
|
if name in self.module_imports and name not in SAFE_REIMPORT_NAMES:
|
|
local_imports[name] = child.lineno
|
|
|
|
# Find name uses
|
|
if isinstance(child, ast.Name):
|
|
if child.id not in name_uses:
|
|
name_uses[child.id] = child.lineno
|
|
|
|
# Check for dangerous pattern: use before import
|
|
for name, import_line in local_imports.items():
|
|
if name in name_uses:
|
|
first_use = name_uses[name]
|
|
if first_use < import_line:
|
|
self.dangerous_imports.append((name, import_line, self.current_function, first_use))
|
|
|
|
def visit_FunctionDef(self, node: ast.FunctionDef):
|
|
old_function = self.current_function
|
|
old_in_function = self.in_function
|
|
old_start_line = self.function_start_line
|
|
|
|
self.current_function = node.name
|
|
self.in_function = True
|
|
self.function_start_line = node.lineno
|
|
|
|
self._check_function(node)
|
|
self.generic_visit(node)
|
|
|
|
self.current_function = old_function
|
|
self.in_function = old_in_function
|
|
self.function_start_line = old_start_line
|
|
|
|
def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef):
|
|
old_function = self.current_function
|
|
old_in_function = self.in_function
|
|
old_start_line = self.function_start_line
|
|
|
|
self.current_function = node.name
|
|
self.in_function = True
|
|
self.function_start_line = node.lineno
|
|
|
|
self._check_function(node)
|
|
self.generic_visit(node)
|
|
|
|
self.current_function = old_function
|
|
self.in_function = old_in_function
|
|
self.function_start_line = old_start_line
|
|
|
|
|
|
def find_import_shadowing(file_path: Path) -> list[tuple[str, int, str]]:
|
|
"""Find cases where local imports shadow module-level imports AND are used before import.
|
|
|
|
Returns list of (name, line_number, function_name) tuples.
|
|
"""
|
|
try:
|
|
with open(file_path, encoding="utf-8") as f:
|
|
source = f.read()
|
|
tree = ast.parse(source)
|
|
visitor = DangerousImportVisitor()
|
|
visitor.visit(tree)
|
|
# Convert (name, import_line, function, first_use_line) to (name, import_line, function)
|
|
return [(name, import_line, func) for name, import_line, func, _ in visitor.dangerous_imports]
|
|
except SyntaxError:
|
|
return [] # Skip files with syntax errors
|
|
|
|
|
|
def get_python_files(directory: Path) -> list[Path]:
|
|
"""Get all Python files in a directory recursively."""
|
|
return list(directory.rglob("*.py"))
|
|
|
|
|
|
class TestImportShadowing:
|
|
"""Tests for import shadowing anti-pattern."""
|
|
|
|
def test_no_import_shadowing_in_main(self):
|
|
"""Check main.py has no import shadowing issues.
|
|
|
|
This test would have caught the ArchiveService scoping bug.
|
|
"""
|
|
main_file = BACKEND_DIR / "main.py"
|
|
if not main_file.exists():
|
|
pytest.skip("main.py not found")
|
|
|
|
shadows = find_import_shadowing(main_file)
|
|
|
|
if shadows:
|
|
error_msg = "Import shadowing detected in main.py:\n"
|
|
for name, line, func in shadows:
|
|
error_msg += f" - '{name}' at line {line} in function '{func}' shadows module-level import\n"
|
|
error_msg += "\nThis can cause 'cannot access local variable' errors."
|
|
pytest.fail(error_msg)
|
|
|
|
def test_no_import_shadowing_in_services(self):
|
|
"""Check service files have no import shadowing issues."""
|
|
services_dir = BACKEND_DIR / "services"
|
|
if not services_dir.exists():
|
|
pytest.skip("services directory not found")
|
|
|
|
all_shadows = []
|
|
for py_file in get_python_files(services_dir):
|
|
shadows = find_import_shadowing(py_file)
|
|
for name, line, func in shadows:
|
|
all_shadows.append((py_file.name, name, line, func))
|
|
|
|
if all_shadows:
|
|
error_msg = "Import shadowing detected in services:\n"
|
|
for filename, name, line, func in all_shadows:
|
|
error_msg += f" - {filename}: '{name}' at line {line} in function '{func}'\n"
|
|
pytest.fail(error_msg)
|
|
|
|
def test_no_import_shadowing_in_routes(self):
|
|
"""Check route files have no import shadowing issues."""
|
|
routes_dir = BACKEND_DIR / "api" / "routes"
|
|
if not routes_dir.exists():
|
|
pytest.skip("routes directory not found")
|
|
|
|
all_shadows = []
|
|
for py_file in get_python_files(routes_dir):
|
|
shadows = find_import_shadowing(py_file)
|
|
for name, line, func in shadows:
|
|
all_shadows.append((py_file.name, name, line, func))
|
|
|
|
if all_shadows:
|
|
error_msg = "Import shadowing detected in routes:\n"
|
|
for filename, name, line, func in all_shadows:
|
|
error_msg += f" - {filename}: '{name}' at line {line} in function '{func}'\n"
|
|
pytest.fail(error_msg)
|
|
|
|
|
|
class TestModuleImports:
|
|
"""Tests for module import health."""
|
|
|
|
def test_all_modules_importable(self):
|
|
"""Verify all Python modules can be imported without errors.
|
|
|
|
This catches syntax errors and missing dependencies.
|
|
|
|
IMPORTANT: We must NOT ``del sys.modules[name]`` to force a fresh
|
|
import here. ``backend.app.main`` is a stateful module — re-importing
|
|
it builds NEW module-level dicts (_timelapse_baselines,
|
|
_expected_prints, _active_prints, …) and re-runs ``root_logger.
|
|
addHandler(console_handler)``. Any test that already bound those
|
|
names via ``from backend.app.main import _timelapse_baselines`` now
|
|
holds a stale reference, while production code resolves the symbol
|
|
through the new module instance — they're two different dicts. CI
|
|
under -n 2 puts test_code_quality.py on the same worker as
|
|
test_print_start_assigns_printer_id_to_vp_archive.py and
|
|
test_timelapse_baseline_restart_recovery.py, and those tests see
|
|
their mock_archive un-mutated / their baseline dict empty even
|
|
though production logged the mutations went through. Local -n 30
|
|
spreads the tests across workers and the collision never happens.
|
|
|
|
``importlib.import_module`` already covers the "is this importable"
|
|
check — it returns the cached module if cached, or runs the import
|
|
machinery if not. Either way, an import-time error surfaces here.
|
|
"""
|
|
import importlib
|
|
|
|
# Modules to test importing
|
|
modules = [
|
|
"backend.app.main",
|
|
"backend.app.services.bambu_mqtt",
|
|
"backend.app.services.printer_manager",
|
|
"backend.app.services.archive",
|
|
"backend.app.services.notification_service",
|
|
"backend.app.services.smart_plug_manager",
|
|
]
|
|
|
|
errors = []
|
|
for module_name in modules:
|
|
try:
|
|
importlib.import_module(module_name)
|
|
except Exception as e:
|
|
errors.append(f"{module_name}: {type(e).__name__}: {e}")
|
|
|
|
if errors:
|
|
pytest.fail("Failed to import modules:\n" + "\n".join(errors))
|
|
|
|
|
|
class TestLogErrorPatterns:
|
|
"""Tests that use log capture to detect runtime errors."""
|
|
|
|
def test_mqtt_message_processing_no_errors(self, capture_logs):
|
|
"""Test that MQTT message processing doesn't log errors."""
|
|
from backend.app.services.bambu_mqtt import BambuMQTTClient
|
|
|
|
client = BambuMQTTClient(
|
|
ip_address="192.168.1.100",
|
|
serial_number="TEST123",
|
|
access_code="12345678",
|
|
)
|
|
client.on_print_start = lambda data: None
|
|
client.on_print_complete = lambda data: None
|
|
|
|
# Process a realistic print lifecycle
|
|
messages = [
|
|
{"print": {"gcode_state": "RUNNING", "gcode_file": "/test.gcode", "subtask_name": "Test"}},
|
|
{"print": {"gcode_state": "RUNNING", "gcode_file": "/test.gcode", "mc_percent": 50}},
|
|
{"print": {"gcode_state": "FINISH", "gcode_file": "/test.gcode", "subtask_name": "Test"}},
|
|
]
|
|
|
|
for msg in messages:
|
|
client._process_message(msg)
|
|
|
|
assert not capture_logs.has_errors(), f"Errors during MQTT processing:\n{capture_logs.format_errors()}"
|