test: fix unit test quality issues found during audit

Fix copy-paste bug in request queue tests (was testing KVS instead of RQ),
replace fragile index-based log assertions with content-based filtering,
strengthen weak delegation tests to verify arguments, and improve test
isolation with monkeypatch.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: vdusek

tests/unit/actor/test_actor_helpers.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import asyncio
 import warnings
 from datetime import timedelta
 from typing import TYPE_CHECKING
@@ -130,19 +131,21 @@ async def test_metamorph_fails_locally(caplog: pytest.LogCaptureFixture) -> None
     async with Actor:
         await Actor.metamorph('random-id')
 
-    assert len(caplog.records) == 1
-    assert caplog.records[0].levelname == 'ERROR'
-    assert 'Actor.metamorph() is only supported when running on the Apify platform.' in caplog.records[0].message
+    matching = [r for r in caplog.records if 'Actor.metamorph()' in r.message]
+    assert len(matching) == 1
+    assert matching[0].levelname == 'ERROR'
+    assert 'only supported when running on the Apify platform' in matching[0].message
 
 
 async def test_reboot_fails_locally(caplog: pytest.LogCaptureFixture) -> None:
     caplog.set_level('WARNING')
     async with Actor:
         await Actor.reboot()
 
-    assert len(caplog.records) == 1
-    assert caplog.records[0].levelname == 'ERROR'
-    assert 'Actor.reboot() is only supported when running on the Apify platform.' in caplog.records[0].message
+    matching = [r for r in caplog.records if 'Actor.reboot()' in r.message]
+    assert len(matching) == 1
+    assert matching[0].levelname == 'ERROR'
+    assert 'only supported when running on the Apify platform' in matching[0].message
 
 
 async def test_add_webhook_fails_locally(caplog: pytest.LogCaptureFixture) -> None:
@@ -152,9 +155,10 @@ async def test_add_webhook_fails_locally(caplog: pytest.LogCaptureFixture) -> No
             Webhook(event_types=[WebhookEventType.ACTOR_BUILD_ABORTED], request_url='https://example.com')
         )
 
-    assert len(caplog.records) == 1
-    assert caplog.records[0].levelname == 'ERROR'
-    assert 'Actor.add_webhook() is only supported when running on the Apify platform.' in caplog.records[0].message
+    matching = [r for r in caplog.records if 'Actor.add_webhook()' in r.message]
+    assert len(matching) == 1
+    assert matching[0].levelname == 'ERROR'
+    assert 'only supported when running on the Apify platform' in matching[0].message
 
 
 async def test_set_status_message_locally(caplog: pytest.LogCaptureFixture) -> None:
@@ -189,8 +193,9 @@ async def test_push_data_with_empty_data() -> None:
         assert result is None
 
 
-async def test_off_removes_event_listener() -> None:
+async def test_off_removes_event_listener(monkeypatch: pytest.MonkeyPatch) -> None:
     """Test that Actor.off() removes an event listener."""
+    monkeypatch.setenv(ApifyEnvVars.PERSIST_STATE_INTERVAL_MILLIS, '50')
     called = False
 
     async def listener(_data: object) -> None:
@@ -200,6 +205,10 @@ async def listener(_data: object) -> None:
     async with Actor:
         Actor.on(Event.PERSIST_STATE, listener)
         Actor.off(Event.PERSIST_STATE, listener)
+        # Wait long enough for at least one PERSIST_STATE event to fire
+        await asyncio.sleep(0.2)
+        # Verify the listener was NOT called because it was removed
+        assert called is False
 
 
 async def test_start_actor_with_webhooks(
@@ -214,7 +223,11 @@ async def test_start_actor_with_webhooks(
             webhooks=[Webhook(event_types=[WebhookEventType.ACTOR_RUN_SUCCEEDED], request_url='https://example.com')],
         )
 
-    assert len(apify_client_async_patcher.calls['actor']['start']) == 1
+    calls = apify_client_async_patcher.calls['actor']['start']
+    assert len(calls) == 1
+    _, kwargs = calls[0][0], calls[0][1]
+    assert 'webhooks' in kwargs
+    assert kwargs['webhooks'] is not None
 
 
 async def test_start_actor_with_timedelta_timeout(
@@ -226,7 +239,10 @@ async def test_start_actor_with_timedelta_timeout(
     async with Actor:
         await Actor.start('some-actor-id', timeout=timedelta(seconds=120))
 
-    assert len(apify_client_async_patcher.calls['actor']['start']) == 1
+    calls = apify_client_async_patcher.calls['actor']['start']
+    assert len(calls) == 1
+    _, kwargs = calls[0][0], calls[0][1]
+    assert kwargs.get('timeout_secs') == 120
 
 
 async def test_start_actor_with_invalid_timeout(
@@ -252,7 +268,11 @@ async def test_call_actor_with_webhooks(
             webhooks=[Webhook(event_types=[WebhookEventType.ACTOR_RUN_SUCCEEDED], request_url='https://example.com')],
         )
 
-    assert len(apify_client_async_patcher.calls['actor']['call']) == 1
+    calls = apify_client_async_patcher.calls['actor']['call']
+    assert len(calls) == 1
+    _, kwargs = calls[0][0], calls[0][1]
+    assert 'webhooks' in kwargs
+    assert kwargs['webhooks'] is not None
 
 
 async def test_call_actor_with_timedelta_timeout(
@@ -264,7 +284,10 @@ async def test_call_actor_with_timedelta_timeout(
     async with Actor:
         await Actor.call('some-actor-id', timeout=timedelta(seconds=120))
 
-    assert len(apify_client_async_patcher.calls['actor']['call']) == 1
+    calls = apify_client_async_patcher.calls['actor']['call']
+    assert len(calls) == 1
+    _, kwargs = calls[0][0], calls[0][1]
+    assert kwargs.get('timeout_secs') == 120
 
 
 async def test_call_actor_with_remaining_time_deprecation(
@@ -305,7 +328,11 @@ async def test_call_task_with_webhooks(
             webhooks=[Webhook(event_types=[WebhookEventType.ACTOR_RUN_SUCCEEDED], request_url='https://example.com')],
         )
 
-    assert len(apify_client_async_patcher.calls['task']['call']) == 1
+    calls = apify_client_async_patcher.calls['task']['call']
+    assert len(calls) == 1
+    _, kwargs = calls[0][0], calls[0][1]
+    assert 'webhooks' in kwargs
+    assert kwargs['webhooks'] is not None
 
 
 async def test_call_task_with_timedelta_timeout(
@@ -317,7 +344,10 @@ async def test_call_task_with_timedelta_timeout(
     async with Actor:
         await Actor.call_task('some-task-id', timeout=timedelta(seconds=120))
 
-    assert len(apify_client_async_patcher.calls['task']['call']) == 1
+    calls = apify_client_async_patcher.calls['task']['call']
+    assert len(calls) == 1
+    _, kwargs = calls[0][0], calls[0][1]
+    assert kwargs.get('timeout_secs') == 120
 
 
 async def test_call_task_with_invalid_timeout(

tests/unit/actor/test_actor_lifecycle.py

@@ -306,10 +306,15 @@ async def test_actor_fail_prevents_further_execution(caplog: pytest.LogCaptureFi
             await actor.fail(exit_code=2, exception=Exception('abc'), status_message='cde')
             raise RuntimeError('This should not trigger')
     except SystemExit:
-        assert caplog.records[-3].levelno == logging.ERROR
-        assert caplog.records[-3].msg == 'Actor failed with an exception'
-        assert caplog.records[-3].exc_text == 'Exception: abc'
-        assert caplog.records[-2].levelno == logging.INFO
-        assert caplog.records[-2].msg == 'Exiting Actor'
-        assert caplog.records[-1].levelno == logging.INFO
-        assert caplog.records[-1].msg == '[Terminal status message]: cde'
+        fail_records = [r for r in caplog.records if r.msg == 'Actor failed with an exception']
+        assert len(fail_records) == 1
+        assert fail_records[0].levelno == logging.ERROR
+        assert fail_records[0].exc_text == 'Exception: abc'
+
+        exit_records = [r for r in caplog.records if r.msg == 'Exiting Actor']
+        assert len(exit_records) == 1
+        assert exit_records[0].levelno == logging.INFO
+
+        status_records = [r for r in caplog.records if r.msg == '[Terminal status message]: cde']
+        assert len(status_records) == 1
+        assert status_records[0].levelno == logging.INFO

tests/unit/actor/test_actor_log.py

@@ -38,15 +38,7 @@ async def test_actor_logs_messages_correctly(caplog: pytest.LogCaptureFixture) -
 
     records = caplog.records
 
-    # Expected number of log records
-    assert len(records) == 13
-
     expected_logs = [
-        (logging.INFO, 'Initializing Actor'),
-        (logging.DEBUG, 'Configuration initialized'),
-        (logging.DEBUG, 'Storage client initialized'),
-        (logging.DEBUG, 'Event manager initialized'),
-        (logging.DEBUG, 'Charging manager initialized'),
         (logging.DEBUG, 'Debug message'),
         (logging.INFO, 'Info message'),
         (logging.WARNING, 'Warning message'),
@@ -58,9 +50,10 @@ async def test_actor_logs_messages_correctly(caplog: pytest.LogCaptureFixture) -
     ]
 
     for level, message, *exception in expected_logs:
-        record = records.pop(0)
+        matching = [r for r in records if r.message == message]
+        assert len(matching) == 1, f'Expected exactly one log record with message {message!r}, found {len(matching)}'
+        record = matching[0]
         assert record.levelno == level
-        assert record.message == message
         if exception:
             assert record.exc_info is not None
             assert record.exc_info[0] is type(exception[0])

tests/unit/actor/test_actor_request_queue.py

@@ -17,12 +17,12 @@ async def test_open_returns_same_references() -> None:
         assert rq1 is rq2
 
         rq_name = 'non-default'
-        rq_by_name_1 = await Actor.open_key_value_store(name=rq_name)
-        rq_by_name_2 = await Actor.open_key_value_store(name=rq_name)
+        rq_by_name_1 = await Actor.open_request_queue(name=rq_name)
+        rq_by_name_2 = await Actor.open_request_queue(name=rq_name)
         assert rq_by_name_1 is rq_by_name_2
 
         rq_1_metadata = await rq_by_name_1.get_metadata()
-        rq_by_id_1 = await Actor.open_key_value_store(id=rq_1_metadata.id)
-        rq_by_id_2 = await Actor.open_key_value_store(id=rq_1_metadata.id)
+        rq_by_id_1 = await Actor.open_request_queue(id=rq_1_metadata.id)
+        rq_by_id_2 = await Actor.open_request_queue(id=rq_1_metadata.id)
         assert rq_by_id_1 is rq_by_name_1
         assert rq_by_id_2 is rq_by_id_1

tests/unit/actor/test_configuration.py

@@ -76,7 +76,7 @@ async def test_setting_config_after_actor_raises_exception() -> None:
 
 
 async def test_actor_using_input_configuration() -> None:
-    """Test that setting configuration in service locator after actor was created raises an exception."""
+    """Test that configuration passed to Actor is stored in the service locator."""
     apify_config = ApifyConfiguration()
     async with Actor(configuration=apify_config):
         pass

tests/unit/storage_clients/test_alias_resolver.py

@@ -65,10 +65,14 @@ async def test_concurrent_alias_creation_uses_lock() -> None:
     assert not lock.locked()
 
 
-async def test_alias_map_cached_locally() -> None:
-    """Test that alias map is returned from memory when already loaded."""
+async def test_get_alias_map_returns_in_memory_map() -> None:
+    """Test that _get_alias_map returns the in-memory map when not at home."""
     AliasResolver._alias_map = {'existing_key': 'existing_id'}
     config = Configuration(is_at_home=False, token='test-token')
 
     result = await AliasResolver._get_alias_map(config)
     assert result == {'existing_key': 'existing_id'}
+    # Also verify that an empty map is returned without fetching from KVS when not at home
+    AliasResolver._alias_map = {}
+    result = await AliasResolver._get_alias_map(config)
+    assert result == {}

tests/unit/storage_clients/test_smart_apify_storage_client.py

@@ -79,9 +79,12 @@ def test_cache_key_at_home(monkeypatch: pytest.MonkeyPatch) -> None:
     service_locator._configuration = None
 
     config = Configuration(is_at_home=True, token='test-token')
-    client = SmartApifyStorageClient()
+    cloud_client = MagicMock(spec=ApifyStorageClient)
+    cloud_client.get_storage_client_cache_key.return_value = 'cloud-key'
+    client = SmartApifyStorageClient(cloud_storage_client=cloud_client)
     key = client.get_storage_client_cache_key(config)
-    assert key is not None
+    assert key == 'cloud-key'
+    cloud_client.get_storage_client_cache_key.assert_called_once_with(config)
 
 
 def test_cache_key_local(monkeypatch: pytest.MonkeyPatch) -> None:
@@ -90,6 +93,9 @@ def test_cache_key_local(monkeypatch: pytest.MonkeyPatch) -> None:
     service_locator._configuration = None
 
     config = Configuration()
-    client = SmartApifyStorageClient()
+    local_client = MagicMock(spec=FileSystemStorageClient)
+    local_client.get_storage_client_cache_key.return_value = 'local-key'
+    client = SmartApifyStorageClient(local_storage_client=local_client)
     key = client.get_storage_client_cache_key(config)
-    assert key is not None
+    assert key == 'local-key'
+    local_client.get_storage_client_cache_key.assert_called_once_with(config)

tests/unit/test_crypto.py

@@ -112,7 +112,7 @@ def test_crypto_random_object_id_length_and_charset() -> None:
     assert len(crypto_random_object_id(5)) == 5
     long_random_object_id = crypto_random_object_id(1000)
     for char in long_random_object_id:
-        assert char in 'abcdefghijklmnopqrstuvwxyzABCEDFGHIJKLMNOPQRSTUVWXYZ0123456789'
+        assert char in '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
 
 
 @pytest.mark.parametrize(('test_input', 'expected'), [(0, '0'), (10, 'a'), (999999999, '15FTGf')])

tests/unit/test_log.py

@@ -1,36 +1,25 @@
 from __future__ import annotations
 
 import logging
-import os
+from typing import TYPE_CHECKING
 
 from apify.log import _configure_logging
 
+if TYPE_CHECKING:
+    import pytest
 
-def test_configure_logging_debug_level() -> None:
+
+def test_configure_logging_debug_level(monkeypatch: pytest.MonkeyPatch) -> None:
     """Test that apify_client logger is set to DEBUG when global level is DEBUG."""
-    original = os.environ.get('CRAWLEE_LOG_LEVEL')
-    try:
-        os.environ['CRAWLEE_LOG_LEVEL'] = 'DEBUG'
-        _configure_logging()
-        apify_client_logger = logging.getLogger('apify_client')
-        assert apify_client_logger.level == logging.DEBUG
-    finally:
-        if original is None:
-            os.environ.pop('CRAWLEE_LOG_LEVEL', None)
-        else:
-            os.environ['CRAWLEE_LOG_LEVEL'] = original
+    monkeypatch.setenv('CRAWLEE_LOG_LEVEL', 'DEBUG')
+    _configure_logging()
+    apify_client_logger = logging.getLogger('apify_client')
+    assert apify_client_logger.level == logging.DEBUG
 
 
-def test_configure_logging_info_level() -> None:
+def test_configure_logging_info_level(monkeypatch: pytest.MonkeyPatch) -> None:
     """Test that apify_client logger is set to INFO when global level > DEBUG."""
-    original = os.environ.get('CRAWLEE_LOG_LEVEL')
-    try:
-        os.environ['CRAWLEE_LOG_LEVEL'] = 'INFO'
-        _configure_logging()
-        apify_client_logger = logging.getLogger('apify_client')
-        assert apify_client_logger.level == logging.INFO
-    finally:
-        if original is None:
-            os.environ.pop('CRAWLEE_LOG_LEVEL', None)
-        else:
-            os.environ['CRAWLEE_LOG_LEVEL'] = original
+    monkeypatch.setenv('CRAWLEE_LOG_LEVEL', 'INFO')
+    _configure_logging()
+    apify_client_logger = logging.getLogger('apify_client')
+    assert apify_client_logger.level == logging.INFO

tests/unit/test_proxy_configuration.py

@@ -255,9 +255,6 @@ async def test_url_reference_not_shared_between_instances() -> None:
     proxy_urls.append('http://proxy-example-3.com:8000')
     proxy_configuration_2 = ProxyConfiguration(proxy_urls=proxy_urls)
 
-    assert proxy_configuration_1 is not None
-    assert proxy_configuration_2 is not None
-
     assert proxy_configuration_1._proxy_urls is not proxy_configuration_2._proxy_urls
 
     session_id = 'ABCD'