From 92a86624b028a16f1ba6ca1f873c80e44c620e5f Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 19 Dec 2025 15:46:18 +0000 Subject: [PATCH] fix(telemetry): Fix enterprise linting errors - Fix mypy type error: collector_results should store int not str - Remove unused variables in test files (F841 errors) - All 99 tests still passing after fixes Co-authored-by: openhands --- enterprise/server/telemetry/service.py | 33 ++++++++------ .../tests/integration/test_telemetry_flow.py | 44 +++++++++++------- .../tests/unit/telemetry/test_service.py | 45 +++++++------------ 3 files changed, 65 insertions(+), 57 deletions(-) diff --git a/enterprise/server/telemetry/service.py b/enterprise/server/telemetry/service.py index 76a9785143..963bc58c88 100644 --- a/enterprise/server/telemetry/service.py +++ b/enterprise/server/telemetry/service.py @@ -15,6 +15,7 @@ from telemetry.registry import CollectorRegistry # Optional import for Replicated SDK (to be implemented in M4) try: from replicated import InstanceStatus, ReplicatedClient + REPLICATED_AVAILABLE = True except ImportError: REPLICATED_AVAILABLE = False @@ -233,7 +234,9 @@ class TelemetryService: # If identity was just established, it will be created during upload # Continue with short interval for one more cycle to ensure upload succeeds if not was_established_before and self._is_identity_established(): - logger.info('Identity just established - first upload completed') + logger.info( + 'Identity just established - first upload completed' + ) logger.info('Metrics upload completed') @@ -268,9 +271,11 @@ class TelemetryService: """ try: with session_maker() as session: - identity = session.query(TelemetryIdentity).filter( - TelemetryIdentity.id == 1 - ).first() + identity = ( + session.query(TelemetryIdentity) + .filter(TelemetryIdentity.id == 1) + .first() + ) # Identity is established if we have both customer_id and instance_id return ( @@ -316,9 +321,11 @@ class TelemetryService: if not last_uploaded: # Check if we have any pending metrics to upload - pending_count = session.query(TelemetryMetrics).filter( - TelemetryMetrics.uploaded_at.is_(None) - ).count() + pending_count = ( + session.query(TelemetryMetrics) + .filter(TelemetryMetrics.uploaded_at.is_(None)) + .count() + ) return pending_count > 0 hours_since = ( @@ -355,7 +362,7 @@ class TelemetryService: f'Collector {collector.collector_name} failed: {e}', exc_info=True, ) - collector_results[collector.collector_name] = f'error: {str(e)}' + collector_results[collector.collector_name] = 0 # Store metrics in database with session_maker() as session: @@ -483,13 +490,11 @@ class TelemetryService: return None - def _get_or_create_identity( - self, session, admin_email: str - ) -> TelemetryIdentity: + def _get_or_create_identity(self, session, admin_email: str) -> TelemetryIdentity: """Get or create telemetry identity with customer and instance IDs.""" - identity = session.query(TelemetryIdentity).filter( - TelemetryIdentity.id == 1 - ).first() + identity = ( + session.query(TelemetryIdentity).filter(TelemetryIdentity.id == 1).first() + ) if not identity: identity = TelemetryIdentity(id=1) diff --git a/enterprise/tests/integration/test_telemetry_flow.py b/enterprise/tests/integration/test_telemetry_flow.py index 9bfc1337c6..1afd1eded0 100644 --- a/enterprise/tests/integration/test_telemetry_flow.py +++ b/enterprise/tests/integration/test_telemetry_flow.py @@ -5,10 +5,7 @@ from datetime import datetime, timedelta, timezone from unittest.mock import AsyncMock, MagicMock, patch import pytest - from server.telemetry.service import TelemetryService -from storage.telemetry_identity import TelemetryIdentity -from storage.telemetry_metrics import TelemetryMetrics @pytest.fixture @@ -40,10 +37,10 @@ class TestTelemetryServiceLifecycle: """Test that service starts and stops without errors.""" with patch.object( fresh_telemetry_service, '_collection_loop', new_callable=AsyncMock - ) as mock_collection: + ): with patch.object( fresh_telemetry_service, '_upload_loop', new_callable=AsyncMock - ) as mock_upload: + ): with patch.object( fresh_telemetry_service, '_initial_collection_check', @@ -78,7 +75,7 @@ class TestTelemetryServiceLifecycle: fresh_telemetry_service, '_initial_collection_check', new_callable=AsyncMock, - ) as mock_initial: + ): await fresh_telemetry_service.start() # Wait for async task to be created @@ -114,7 +111,8 @@ class TestTelemetryServiceLifecycle: # Verify tasks are the same (not recreated) assert ( - fresh_telemetry_service._collection_task == first_collection_task + fresh_telemetry_service._collection_task + == first_collection_task ) # Clean up @@ -286,16 +284,22 @@ class TestCollectionLoop: pass with patch.object( - fresh_telemetry_service, '_collect_metrics', side_effect=mock_collect_with_error + fresh_telemetry_service, + '_collect_metrics', + side_effect=mock_collect_with_error, ): with patch.object( fresh_telemetry_service, '_should_collect', return_value=True ): with patch.object( - fresh_telemetry_service, '_is_identity_established', return_value=True + fresh_telemetry_service, + '_is_identity_established', + return_value=True, ): # Start collection loop - task = asyncio.create_task(fresh_telemetry_service._collection_loop()) + task = asyncio.create_task( + fresh_telemetry_service._collection_loop() + ) # Wait for multiple iterations await asyncio.sleep(0.3) @@ -354,13 +358,17 @@ class TestUploadLoop: pass with patch.object( - fresh_telemetry_service, '_upload_pending_metrics', side_effect=mock_upload_with_error + fresh_telemetry_service, + '_upload_pending_metrics', + side_effect=mock_upload_with_error, ): with patch.object( fresh_telemetry_service, '_should_upload', return_value=True ): with patch.object( - fresh_telemetry_service, '_is_identity_established', return_value=True + fresh_telemetry_service, + '_is_identity_established', + return_value=True, ): # Start upload loop task = asyncio.create_task(fresh_telemetry_service._upload_loop()) @@ -392,7 +400,9 @@ class TestMetricsCollection: mock_session.__exit__ = MagicMock(return_value=None) mock_session_maker.return_value = mock_session - with patch('server.telemetry.service.CollectorRegistry') as mock_registry_class: + with patch( + 'server.telemetry.service.CollectorRegistry' + ) as mock_registry_class: mock_registry = MagicMock() mock_registry_class.return_value = mock_registry @@ -464,14 +474,18 @@ class TestReplicatedIntegration: with patch('server.telemetry.service.REPLICATED_AVAILABLE', True): with patch('server.telemetry.service.InstanceStatus') as mock_status: mock_status.RUNNING = 'RUNNING' - with patch('server.telemetry.service.ReplicatedClient') as mock_client_class: + with patch( + 'server.telemetry.service.ReplicatedClient' + ) as mock_client_class: mock_client = MagicMock() mock_customer = MagicMock() mock_customer.customer_id = 'cust-123' mock_instance = MagicMock() mock_instance.instance_id = 'inst-456' - mock_customer.get_or_create_instance.return_value = mock_instance + mock_customer.get_or_create_instance.return_value = ( + mock_instance + ) mock_client.customer.get_or_create.return_value = mock_customer mock_client_class.return_value = mock_client diff --git a/enterprise/tests/unit/telemetry/test_service.py b/enterprise/tests/unit/telemetry/test_service.py index 677dfbed6d..ef2a7439eb 100644 --- a/enterprise/tests/unit/telemetry/test_service.py +++ b/enterprise/tests/unit/telemetry/test_service.py @@ -1,11 +1,9 @@ """Unit tests for the TelemetryService.""" -import asyncio from datetime import datetime, timedelta, timezone from unittest.mock import AsyncMock, MagicMock, patch import pytest - from server.telemetry.service import TelemetryService @@ -39,7 +37,6 @@ class TestTelemetryServiceInitialization: def test_initialization_once(self, telemetry_service): """Test that __init__ only runs once.""" - initial_collection_interval = telemetry_service.collection_interval_days telemetry_service.collection_interval_days = 999 # Create another "instance" (should be same singleton) @@ -132,9 +129,7 @@ class TestIdentityEstablishment: assert not telemetry_service._is_identity_established() @patch('server.telemetry.service.session_maker') - def test_identity_established_complete( - self, mock_session_maker, telemetry_service - ): + def test_identity_established_complete(self, mock_session_maker, telemetry_service): """Test identity established when both customer_id and instance_id exist.""" mock_session = MagicMock() mock_session.__enter__ = MagicMock(return_value=mock_session) @@ -239,9 +234,7 @@ class TestUploadLogic: assert telemetry_service._should_upload() @patch('server.telemetry.service.session_maker') - def test_should_not_upload_no_pending( - self, mock_session_maker, telemetry_service - ): + def test_should_not_upload_no_pending(self, mock_session_maker, telemetry_service): """Test should not upload when no pending metrics exist.""" mock_session = MagicMock() mock_session.__enter__ = MagicMock(return_value=mock_session) @@ -355,9 +348,7 @@ class TestGetAdminEmail: """Test when no admin email is available.""" mock_getenv.return_value = None - mock_session.query.return_value.filter.return_value.filter.return_value.order_by.return_value.first.return_value = ( - None - ) + mock_session.query.return_value.filter.return_value.filter.return_value.order_by.return_value.first.return_value = None email = telemetry_service._get_admin_email(mock_session) @@ -388,7 +379,7 @@ class TestGetOrCreateIdentity: mock_customer ) - identity = telemetry_service._get_or_create_identity( + telemetry_service._get_or_create_identity( mock_session, 'test@example.com' ) @@ -405,9 +396,7 @@ class TestGetOrCreateIdentity: mock_identity ) - identity = telemetry_service._get_or_create_identity( - mock_session, 'test@example.com' - ) + telemetry_service._get_or_create_identity(mock_session, 'test@example.com') # Should not create new instance since both IDs exist assert mock_session.add.call_count == 0 @@ -425,9 +414,7 @@ class TestLicenseWarningStatus: mock_session.__exit__ = MagicMock(return_value=None) mock_session_maker.return_value = mock_session - mock_session.query.return_value.filter.return_value.order_by.return_value.first.return_value = ( - None - ) + mock_session.query.return_value.filter.return_value.order_by.return_value.first.return_value = None status = telemetry_service.get_license_warning_status() @@ -446,9 +433,7 @@ class TestLicenseWarningStatus: mock_metric = MagicMock() mock_metric.uploaded_at = datetime.now(timezone.utc) - timedelta(days=2) - mock_session.query.return_value.filter.return_value.order_by.return_value.first.return_value = ( - mock_metric - ) + mock_session.query.return_value.filter.return_value.order_by.return_value.first.return_value = mock_metric status = telemetry_service.get_license_warning_status() @@ -466,9 +451,7 @@ class TestLicenseWarningStatus: mock_metric = MagicMock() mock_metric.uploaded_at = datetime.now(timezone.utc) - timedelta(days=5) - mock_session.query.return_value.filter.return_value.order_by.return_value.first.return_value = ( - mock_metric - ) + mock_session.query.return_value.filter.return_value.order_by.return_value.first.return_value = mock_metric status = telemetry_service.get_license_warning_status() @@ -489,7 +472,9 @@ class TestLifecycleManagement: telemetry_service, '_upload_loop', new_callable=AsyncMock ): with patch.object( - telemetry_service, '_initial_collection_check', new_callable=AsyncMock + telemetry_service, + '_initial_collection_check', + new_callable=AsyncMock, ): await telemetry_service.start() @@ -509,7 +494,9 @@ class TestLifecycleManagement: telemetry_service, '_upload_loop', new_callable=AsyncMock ): with patch.object( - telemetry_service, '_initial_collection_check', new_callable=AsyncMock + telemetry_service, + '_initial_collection_check', + new_callable=AsyncMock, ): await telemetry_service.start() first_collection_task = telemetry_service._collection_task @@ -535,7 +522,9 @@ class TestLifecycleManagement: telemetry_service, '_upload_loop', new_callable=AsyncMock ): with patch.object( - telemetry_service, '_initial_collection_check', new_callable=AsyncMock + telemetry_service, + '_initial_collection_check', + new_callable=AsyncMock, ): await telemetry_service.start() await telemetry_service.stop()