Skip to content

Conversation

dandye
Copy link
Collaborator

@dandye dandye commented Aug 27, 2025

Summary

  • Adds GitHub Actions workflow to catch Python import errors before they reach production
  • Implements pragmatic file-by-file import testing to prevent errors like from typing import str
  • Tests all Python files across server directories to ensure they can be imported without errors

Context

A production incident occurred when from typing import str (an invalid import) made it through to deployment. Standard tools like python -m compileall only catch syntax errors, not import errors. This PR implements a practical solution to catch these errors in CI.

Implementation

  • .github/workflows/pr-tests.yml: GitHub Actions workflow that runs on all PRs
  • tests/check_imports.py: Script that attempts to import every Python file to catch runtime import errors

Caveats & Trade-offs

This is an expedient solution with known limitations:

Known Issues

  • Brute force approach: Tests every .py file in isolation, including setup.py and test files
  • Potential false positives: Some files may not be designed to run standalone
  • Performance: Importing every file individually is slower than ideal
  • No exclusion mechanism: Cannot skip specific files or patterns
  • Missing dependencies: Files might fail if they import optional dependencies

Why This Approach

  • Catches the exact error that broke production (from typing import str)
  • Simple and pragmatic: No complex configuration or type annotation requirements
  • Works immediately: Doesn't require fixing legacy code style issues
  • Better than alternatives:
    • compileall doesn't catch import errors
    • mypy would require type hints throughout legacy code
    • Linters (ruff, flake8) don't catch invalid typing imports

Future Improvements

Consider these enhancements in follow-up PRs:

  • Add exclusion patterns for test files and setup.py
  • Implement parallel testing for performance
  • Add configuration file for customizing behavior
  • Consider migrating to mypy once legacy code is cleaned up

Test Plan

  • Tested locally - correctly identifies import errors with exit code 1
  • Verified it catches the specific from typing import str error
  • CI workflow will validate on this PR

This is a pragmatic, immediate fix to prevent critical import errors from reaching production. While not perfect, it solves the immediate problem and can be refined over time.

- Add GitHub Actions workflow for PR testing
- Create tests/check_imports.py to validate all Python files can be imported
- Tests all files in server/gti, server/scc, server/secops, server/secops-soar
- Catches import errors like "from typing import str" that break production
- Pragmatic approach that tests actual runtime imports
- Will fail CI with exit code 1 on any import errors
@dandye dandye requested a review from a team August 27, 2025 03:06
dandye added 3 commits August 26, 2025 23:14
setup.py files execute setuptools commands when imported, causing false
positive failures in CI. These files don't need import testing as they're
not imported during normal runtime.
- Fix models.py: remove invalid 'str' import from typing
- Use double quotes consistently throughout check_imports.py
- Exclude problematic file types from import testing:
  - setup.py files (execute setuptools commands when imported)
  - test directories and conftest.py (test fixtures)
  - example.py files (standalone examples)
  - __init__.py files (some have special import logic)
  - tools/ subdirectories (use relative imports)

These exclusions prevent false positives while still catching real
import errors in the main codebase.
- Special handling for tools/ directories: import as modules not files
- This allows relative imports (from .. import utils) to work correctly
- Tests 323 Python files including all tools directories
- Still catches import errors like 'from typing import str'
- More comprehensive coverage of the codebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant