Skip to content

chore: add type checking, CI workflow, fix type errors#15

Merged
jy-tan merged 3 commits intomainfrom
fix-type-check
Jan 8, 2026
Merged

chore: add type checking, CI workflow, fix type errors#15
jy-tan merged 3 commits intomainfrom
fix-type-check

Conversation

@jy-tan
Copy link
Contributor

@jy-tan jy-tan commented Jan 8, 2026

Summary

Introduces static type checking with ty and adds a GitHub Actions CI pipeline. Fixes all type errors in the SDK core and test suite.

Changes

CI/Tooling

  • Add GitHub Actions workflow with lint, typecheck, unit tests, and build jobs
  • Configure ty type checker in pyproject.toml:
    • Exclude e2e-tests/ directories
    • Ignore unresolved imports for optional dependencies (django, psycopg, redis, etc.)

Type Fixes (drift/)

  • Fix protobuf serialization: proper Struct/Value conversion, NullValue.NULL_VALUE, StatusCode.UNSPECIFIED
  • Add None-safety checks in communicator, socket instrumentation, and trace blocking manager
  • Use setattr() for monkey-patching to satisfy type checker
  • Fix import paths and method signatures in WSGI/OTel code

Test Cleanup

  • Remove broken unit tests that mocked internal implementation details incorrectly
  • Add type narrowing assertions after assertIsNotNone() calls
  • Fix test utility imports and signatures

Test Changes Explained

Why tests were removed instead of fixed:

The removed tests (in test_requests_instrumentation.py, test_error_resilience.py) had fundamental issues that couldn't be fixed without significant refactoring:

  1. Incorrect mocking patterns — Tests used @patch("drift.instrumentation.requests.instrumentation.JsonSchemaHelper") but JsonSchemaHelper isn't imported at that module level. It's accessed via from ...core.json_schema_helper import JsonSchemaHelper. This is a common Python mocking pitfall where you must patch where a name is looked up, not where it's defined.

  2. Testing internal implementation details — Tests like TestBatchProcessorErrorResilience directly instantiated BatchSpanProcessor with specific constructor signatures that have since changed. These tests were tightly coupled to implementation rather than behavior.

  3. Functionality is still tested — The behaviors these tests intended to verify (replay trace IDs, transform engine, mock responses, context propagation) are exercised by the E2E test suite which tests the full integration.

Tests that were kept/fixed:

  • Helper method tests (TestRequestsInstrumentationHelpers) — test public APIs with real inputs/outputs
  • TestMockResponseDecoding — tests actual response decoding without mocking internals
  • Adapter error resilience tests — test the public ExportResult API
  • All tests that verify behavior rather than implementation

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

2 issues found across 51 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name=".github/workflows/ci.yml">

<violation number="1" location=".github/workflows/ci.yml:99">
P2: The verification step installs to system Python (`--system`) and tests with bare `python`, but the system Python on ubuntu-latest is 3.10 while the project requires Python 3.12+. Use the uv-managed environment instead to ensure consistency.</violation>
</file>

<file name="drift/instrumentation/psycopg2/instrumentation.py">

<violation number="1" location="drift/instrumentation/psycopg2/instrumentation.py:678">
P2: The comment above this code block states "Schemas must be None to avoid betterproto map serialization issues", but the code now uses `JsonSchema()` instead of `None`. Either update the comment if the empty `JsonSchema()` object doesn't cause serialization issues, or reconsider this change if the warning is still valid.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@jy-tan jy-tan merged commit 8d2901b into main Jan 8, 2026
5 checks passed
@jy-tan jy-tan deleted the fix-type-check branch January 8, 2026 05:28
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