From 4e7cd68c66901c28fa1257c54d5b416147403c18 Mon Sep 17 00:00:00 2001 From: Arjun Dixit1 Date: Thu, 2 Oct 2025 23:07:46 +0530 Subject: [PATCH 1/4] feat: Add unit tests for RateLimiter --- forklet/infrastructure/test_rate_limiter.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 forklet/infrastructure/test_rate_limiter.py diff --git a/forklet/infrastructure/test_rate_limiter.py b/forklet/infrastructure/test_rate_limiter.py new file mode 100644 index 0000000..e69de29 From 3c084676b6942a68fc3fdd98f082ccae4283ea97 Mon Sep 17 00:00:00 2001 From: Arjun Dixit1 Date: Fri, 3 Oct 2025 00:26:43 +0530 Subject: [PATCH 2/4] fix: Populate test file with code --- forklet/infrastructure/test_rate_limiter.py | 195 ++++++++++++++++++++ 1 file changed, 195 insertions(+) diff --git a/forklet/infrastructure/test_rate_limiter.py b/forklet/infrastructure/test_rate_limiter.py index e69de29..e8a9e78 100644 --- a/forklet/infrastructure/test_rate_limiter.py +++ b/forklet/infrastructure/test_rate_limiter.py @@ -0,0 +1,195 @@ +# tests/infrastructure/test_rate_limiter.py + +import time +import random +import asyncio +from datetime import datetime, timedelta +from unittest.mock import patch, AsyncMock + +import pytest + +# Adjust this import path to match your project's structure +from forklet.infrastructure.rate_limiter import RateLimiter, RateLimitInfo + +# Mark all tests in this file as asyncio +pytestmark = pytest.mark.asyncio + + +## 1. RateLimitInfo Helper Class Tests +# ------------------------------------ + +def test_rate_limit_info_is_exhausted(): + """Test the 'is_exhausted' property logic.""" + info = RateLimitInfo() + info.remaining = 11 + assert not info.is_exhausted, "Should not be exhausted when remaining is > 10" + + info.remaining = 10 + assert info.is_exhausted, "Should be exhausted when remaining is <= 10" + + info.remaining = 0 + assert info.is_exhausted, "Should be exhausted when remaining is 0" + + +def test_rate_limit_info_reset_in_seconds(): + """Test the calculation of seconds until reset.""" + info = RateLimitInfo() + + # Mock datetime.now() to control the current time for the test + mock_now = datetime(2025, 10, 2, 12, 0, 0) + with patch('forklet.infrastructure.rate_limiter.datetime', autospec=True) as mock_datetime: + mock_datetime.now.return_value = mock_now + + # Set reset_time 30 seconds into the future + info.reset_time = mock_now + timedelta(seconds=30) + assert info.reset_in_seconds == 30.0 + + # Set reset_time in the past + info.reset_time = mock_now - timedelta(seconds=30) + assert info.reset_in_seconds == 0.0, "Should not return negative time" + + # No reset time set + info.reset_time = None + assert info.reset_in_seconds == 0.0 + + +## 2. RateLimiter Initialization Test +# ------------------------------------ + +def test_ratelimiter_initialization(): + """Ensure the RateLimiter initializes with correct default values.""" + rl = RateLimiter(default_delay=0.5, max_delay=30.0, adaptive=False) + assert rl.default_delay == 0.5 + assert rl.max_delay == 30.0 + assert not rl.adaptive + + +## 3. Update from Headers Tests +# ------------------------------ + +async def test_update_rate_limit_info_sets_values_correctly(): + """Test that rate limit info is correctly parsed from headers.""" + rl = RateLimiter() + reset_timestamp = int(time.time()) + 60 + + headers = { + "x-ratelimit-limit": "5000", + "x-ratelimit-remaining": "4500", + "x-ratelimit-used": "500", + "x-ratelimit-reset": str(reset_timestamp), + } + + await rl.update_rate_limit_info(headers) + info = rl.rate_limit_info + + assert info.limit == 5000 + assert info.remaining == 4500 + assert info.used == 500 + assert info.reset_time == datetime.fromtimestamp(reset_timestamp) + assert not info.is_exhausted + + +async def test_update_rate_limit_increments_consecutive_limits(): + """Test that _consecutive_limits is handled correctly.""" + rl = RateLimiter() + exhausted_headers = {"x-ratelimit-remaining": "5"} + ok_headers = {"x-ratelimit-remaining": "100"} + + await rl.update_rate_limit_info(exhausted_headers) + assert rl._consecutive_limits == 1 + + await rl.update_rate_limit_info(exhausted_headers) + assert rl._consecutive_limits == 2 + + await rl.update_rate_limit_info(ok_headers) + assert rl._consecutive_limits == 0, "Counter should reset when not exhausted" + + +## 4. Acquire Logic and Wait Behavior Tests +# ------------------------------------------ + +@patch('asyncio.sleep', new_callable=AsyncMock) +async def test_acquire_waits_when_primary_rate_limit_exhausted(mock_sleep): + """Test that acquire() waits for reset_in_seconds when exhausted.""" + rl = RateLimiter() + + # Mock datetime.now() to control time + mock_now = datetime(2025, 10, 2, 12, 0, 0) + with patch('forklet.infrastructure.rate_limiter.datetime', autospec=True) as mock_datetime: + mock_datetime.now.return_value = mock_now + + # Set state to exhausted, with reset 15 seconds in the future + rl.rate_limit_info.remaining = 5 + rl.rate_limit_info.reset_time = mock_now + timedelta(seconds=15) + + await rl.acquire() + + # Check that it slept for the primary rate limit duration + mock_sleep.assert_any_call(15.0) + + +@patch('asyncio.sleep', new_callable=AsyncMock) +async def test_acquire_uses_adaptive_delay(mock_sleep): + """Test that acquire() uses the calculated adaptive delay.""" + rl = RateLimiter(default_delay=1.0) + + # Mock time.time() for delay calculation + with patch('time.time', side_effect=[1000.0, 1000.1]): + # Ensure rate limit is not exhausted + rl.rate_limit_info.remaining = 2000 + + await rl.acquire() + + # Check that sleep was called. The exact value has jitter, so we check if it was called. + mock_sleep.assert_called() + # The first call to time.time() is at the start of acquire(), + # the second is for _last_request. The delay calculation uses the first one. + # Expected delay is around 1.0 seconds. + assert mock_sleep.call_args[0][0] > 0.5 + + +async def test_acquire_updates_last_request_time(): + """Test that acquire() correctly updates the _last_request timestamp.""" + rl = RateLimiter() + + with patch('time.time', return_value=12345.0) as mock_time: + # Patch sleep to make the test run instantly + with patch('asyncio.sleep'): + await rl.acquire() + assert rl._last_request == 12345.0 + + +## 5. Task Safety Test +# --------------------- + +async def test_update_rate_limit_info_is_task_safe(): + """Ensure concurrent updates do not corrupt the RateLimiter's state.""" + rl = RateLimiter() + num_tasks = 50 + + async def worker(headers): + # Add a small, random delay to increase the chance of race conditions if unlocked + await asyncio.sleep(0.01 * random.random()) + await rl.update_rate_limit_info(headers) + + # Create many concurrent tasks + all_headers = [] + for i in range(num_tasks): + headers = { + "x-ratelimit-limit": str(5000 + i), + "x-ratelimit-remaining": str(4000 + i), + } + all_headers.append(headers) + + tasks = [asyncio.create_task(worker(h)) for h in all_headers] + await asyncio.gather(*tasks) + + # The final state should be internally consistent, belonging to one of the updates. + # If limit is 5000+i, remaining must be 4000+i. + final_limit = rl.rate_limit_info.limit + final_remaining = rl.rate_limit_info.remaining + + # Calculate what 'i' must have been based on the final limit + i = final_limit - 5000 + expected_remaining = 4000 + i + assert final_remaining == expected_remaining, "Inconsistent state suggests a race condition" \ No newline at end of file From 1e637108aa7bdf271a701aad623320b575f5baa5 Mon Sep 17 00:00:00 2001 From: Arjun Dixit1 Date: Fri, 3 Oct 2025 00:43:04 +0530 Subject: [PATCH 3/4] fix: Relocate test file to correct directory --- {forklet => tests}/infrastructure/test_rate_limiter.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {forklet => tests}/infrastructure/test_rate_limiter.py (100%) diff --git a/forklet/infrastructure/test_rate_limiter.py b/tests/infrastructure/test_rate_limiter.py similarity index 100% rename from forklet/infrastructure/test_rate_limiter.py rename to tests/infrastructure/test_rate_limiter.py From 0689acac13c0072ef4053de822d7d607df272b2a Mon Sep 17 00:00:00 2001 From: Arjun Dixit1 Date: Tue, 7 Oct 2025 00:30:55 +0530 Subject: [PATCH 4/4] feat: Add unit tests for DownloadOrchestrator --- tests/core/test_download_orchestrator.py | 148 ++++++++++++++++++++++ tests/infrastructure/test_rate_limiter.py | 47 +++---- 2 files changed, 167 insertions(+), 28 deletions(-) create mode 100644 tests/core/test_download_orchestrator.py diff --git a/tests/core/test_download_orchestrator.py b/tests/core/test_download_orchestrator.py new file mode 100644 index 0000000..4b36487 --- /dev/null +++ b/tests/core/test_download_orchestrator.py @@ -0,0 +1,148 @@ +import pytest +from unittest.mock import MagicMock, AsyncMock, patch +from pathlib import Path + +# Assume the class is located at forklet.core.orchestrator.DownloadOrchestrator +# Make sure this import path is correct for your project structure. +from forklet.core.orchestrator import DownloadOrchestrator +from forklet.models import GitHubFile # Import the actual model if available for type hinting + +# --- Test Fixtures for Setup --- + +@pytest.fixture +def mock_services(): + """Creates mock objects for services used by the orchestrator.""" + github_service = MagicMock() + download_service = MagicMock() + github_service.get_repository_tree = AsyncMock() + github_service.get_file_content = AsyncMock() + download_service.save_content = AsyncMock(return_value=128) # Return bytes_written + download_service.ensure_directory = AsyncMock() + return github_service, download_service + +@pytest.fixture +def orchestrator(mock_services): + """Initializes the DownloadOrchestrator with mocked services.""" + github_service, download_service = mock_services + return DownloadOrchestrator( + github_service=github_service, + download_service=download_service, + max_concurrent_downloads=5 + ) + +@pytest.fixture +def mock_request(): + """Creates a mock DownloadRequest object for use in tests.""" + request = MagicMock() + request.repository.owner = "test-owner" + request.repository.name = "test-repo" + request.repository.display_name = "test-owner/test-repo" + request.git_ref = "main" + request.filters = [] + request.destination = Path("/fake/destination") + request.create_destination = True + request.overwrite_existing = False + request.preserve_structure = True + request.show_progress_bars = False + return request + +# --- Test Cases --- + +class TestDownloadOrchestrator: + + # --- Initialization Tests --- + def test_initialization_sets_properties_correctly(self, orchestrator): + """Verify that max_concurrent_downloads is correctly set.""" + assert orchestrator.max_concurrent_downloads == 5 + assert orchestrator._semaphore._value == 5 + assert not orchestrator._is_cancelled + + # --- execute_download Tests --- + @pytest.mark.asyncio + async def test_execute_download_success(self, orchestrator, mock_services, mock_request): + """Simulate a successful download with mocked services.""" + github_service, _ = mock_services + mock_file_list = [MagicMock(spec=GitHubFile, path="file1.txt", size=100)] + github_service.get_repository_tree.return_value = mock_file_list + + # Patch the concurrent downloader AND the filter engine + with patch.object(orchestrator, '_download_files_concurrently', new_callable=AsyncMock) as mock_downloader, \ + patch('forklet.core.orchestrator.FilterEngine') as mock_filter_engine: + + # Configure the mocks + mock_downloader.return_value = (["file1.txt"], {}) + mock_filter_engine.return_value.filter_files.return_value.included_files = mock_file_list + + # Act + result = await orchestrator.execute_download(request=mock_request) + + # Assert + mock_downloader.assert_awaited_once() + assert result.status.value == "completed" + + @pytest.mark.asyncio + async def test_execute_download_repo_fetch_fails(self, orchestrator, mock_services, mock_request): + """Test error handling when repository tree fetch fails.""" + github_service, _ = mock_services + github_service.get_repository_tree.side_effect = Exception("API limit reached") + + result = await orchestrator.execute_download(request=mock_request) + + # Corrected assertion to check for uppercase 'FAILED' + assert result.status.value == "failed" + assert "API limit reached" in result.error_message + + # --- _download_single_file Tests --- + @pytest.mark.asyncio + async def test_download_single_file_skips_if_exists(self, orchestrator, mock_services, mock_request): + """Test skip logic when file already exists and overwrite_existing=False.""" + _, download_service = mock_services + mock_request.overwrite_existing = False + + mock_progress = MagicMock() + mock_stats = MagicMock() + mock_file = MagicMock(spec=GitHubFile, path="path/to/file.txt") + + with patch('pathlib.Path.exists', return_value=True): + result = await orchestrator._download_single_file(mock_file, mock_request, mock_progress, mock_stats) + assert result is None + download_service.save_content.assert_not_called() + + @pytest.mark.asyncio + async def test_download_single_file_saves_content(self, orchestrator, mock_services, mock_request): + """Ensure file content is written via DownloadService.save_content.""" + github_service, download_service = mock_services + github_service.get_file_content.return_value = b"hello world" + mock_request.overwrite_existing = True + + mock_progress = MagicMock() + mock_stats = MagicMock() + mock_file = MagicMock(spec=GitHubFile, path="path/to/file.txt", download_url="http://fake.url/content") + target_path = mock_request.destination / mock_file.path + + with patch('pathlib.Path.exists', return_value=False): + await orchestrator._download_single_file(mock_file, mock_request, mock_progress, mock_stats) + download_service.save_content.assert_awaited_once_with( + b"hello world", target_path, show_progress=mock_request.show_progress_bars + ) + + # --- Control Methods Tests --- + def test_cancel_sets_flag_and_logs(self, orchestrator): + """Test cancel() -> sets _is_cancelled=True and logs.""" + with patch('forklet.core.orchestrator.logger') as mock_logger: + orchestrator.cancel() + assert orchestrator._is_cancelled is True + mock_logger.info.assert_called_with("Download cancelled by user") + + @pytest.mark.asyncio + async def test_pause_and_resume_run_without_errors(self, orchestrator): + """Test that pause() and resume() run without errors.""" + try: + await orchestrator.pause() + await orchestrator.resume() + except Exception as e: + pytest.fail(f"pause() or resume() raised an exception: {e}") + + def test_get_current_progress_returns_none(self, orchestrator): + """Test get_current_progress() -> returns None (until implemented).""" + assert orchestrator.get_current_progress() is None \ No newline at end of file diff --git a/tests/infrastructure/test_rate_limiter.py b/tests/infrastructure/test_rate_limiter.py index e8a9e78..6f9a4ef 100644 --- a/tests/infrastructure/test_rate_limiter.py +++ b/tests/infrastructure/test_rate_limiter.py @@ -11,8 +11,6 @@ # Adjust this import path to match your project's structure from forklet.infrastructure.rate_limiter import RateLimiter, RateLimitInfo -# Mark all tests in this file as asyncio -pytestmark = pytest.mark.asyncio ## 1. RateLimitInfo Helper Class Tests @@ -35,20 +33,16 @@ def test_rate_limit_info_reset_in_seconds(): """Test the calculation of seconds until reset.""" info = RateLimitInfo() - # Mock datetime.now() to control the current time for the test mock_now = datetime(2025, 10, 2, 12, 0, 0) with patch('forklet.infrastructure.rate_limiter.datetime', autospec=True) as mock_datetime: mock_datetime.now.return_value = mock_now - # Set reset_time 30 seconds into the future info.reset_time = mock_now + timedelta(seconds=30) assert info.reset_in_seconds == 30.0 - # Set reset_time in the past info.reset_time = mock_now - timedelta(seconds=30) assert info.reset_in_seconds == 0.0, "Should not return negative time" - # No reset time set info.reset_time = None assert info.reset_in_seconds == 0.0 @@ -66,7 +60,7 @@ def test_ratelimiter_initialization(): ## 3. Update from Headers Tests # ------------------------------ - +@pytest.mark.asyncio async def test_update_rate_limit_info_sets_values_correctly(): """Test that rate limit info is correctly parsed from headers.""" rl = RateLimiter() @@ -88,7 +82,7 @@ async def test_update_rate_limit_info_sets_values_correctly(): assert info.reset_time == datetime.fromtimestamp(reset_timestamp) assert not info.is_exhausted - +@pytest.mark.asyncio async def test_update_rate_limit_increments_consecutive_limits(): """Test that _consecutive_limits is handled correctly.""" rl = RateLimiter() @@ -109,51 +103,53 @@ async def test_update_rate_limit_increments_consecutive_limits(): # ------------------------------------------ @patch('asyncio.sleep', new_callable=AsyncMock) +@pytest.mark.asyncio async def test_acquire_waits_when_primary_rate_limit_exhausted(mock_sleep): """Test that acquire() waits for reset_in_seconds when exhausted.""" rl = RateLimiter() - # Mock datetime.now() to control time mock_now = datetime(2025, 10, 2, 12, 0, 0) with patch('forklet.infrastructure.rate_limiter.datetime', autospec=True) as mock_datetime: mock_datetime.now.return_value = mock_now - # Set state to exhausted, with reset 15 seconds in the future rl.rate_limit_info.remaining = 5 rl.rate_limit_info.reset_time = mock_now + timedelta(seconds=15) await rl.acquire() - # Check that it slept for the primary rate limit duration mock_sleep.assert_any_call(15.0) @patch('asyncio.sleep', new_callable=AsyncMock) +@pytest.mark.asyncio async def test_acquire_uses_adaptive_delay(mock_sleep): - """Test that acquire() uses the calculated adaptive delay.""" + """Test that acquire() uses the calculated adaptive delay on the second call.""" rl = RateLimiter(default_delay=1.0) - # Mock time.time() for delay calculation - with patch('time.time', side_effect=[1000.0, 1000.1]): + # Mock time.time() to simulate time passing + with patch('time.time', side_effect=[1000.0, 1000.1, 1000.2, 1000.3]) as mock_time: # Ensure rate limit is not exhausted - rl.rate_limit_info.remaining = 2000 + rl.rate_limit_info.remaining = 2000 + + # FIRST call: This sets _last_request, but calculates a delay of 0. + await rl.acquire() + mock_sleep.assert_not_called() # No sleep on the first call + assert rl._last_request == 1000.1 + # SECOND call: This call is close to the first one, triggering the delay. await rl.acquire() - # Check that sleep was called. The exact value has jitter, so we check if it was called. + # Assert that sleep was finally called on the second run mock_sleep.assert_called() - # The first call to time.time() is at the start of acquire(), - # the second is for _last_request. The delay calculation uses the first one. - # Expected delay is around 1.0 seconds. - assert mock_sleep.call_args[0][0] > 0.5 - + # The delay should be > 0 because elapsed time (0.1s) < default_delay (1.0s) + assert mock_sleep.call_args[0][0] > 0 +@pytest.mark.asyncio async def test_acquire_updates_last_request_time(): """Test that acquire() correctly updates the _last_request timestamp.""" rl = RateLimiter() with patch('time.time', return_value=12345.0) as mock_time: - # Patch sleep to make the test run instantly with patch('asyncio.sleep'): await rl.acquire() assert rl._last_request == 12345.0 @@ -161,18 +157,16 @@ async def test_acquire_updates_last_request_time(): ## 5. Task Safety Test # --------------------- - +@pytest.mark.asyncio async def test_update_rate_limit_info_is_task_safe(): """Ensure concurrent updates do not corrupt the RateLimiter's state.""" rl = RateLimiter() num_tasks = 50 async def worker(headers): - # Add a small, random delay to increase the chance of race conditions if unlocked await asyncio.sleep(0.01 * random.random()) await rl.update_rate_limit_info(headers) - # Create many concurrent tasks all_headers = [] for i in range(num_tasks): headers = { @@ -184,12 +178,9 @@ async def worker(headers): tasks = [asyncio.create_task(worker(h)) for h in all_headers] await asyncio.gather(*tasks) - # The final state should be internally consistent, belonging to one of the updates. - # If limit is 5000+i, remaining must be 4000+i. final_limit = rl.rate_limit_info.limit final_remaining = rl.rate_limit_info.remaining - # Calculate what 'i' must have been based on the final limit i = final_limit - 5000 expected_remaining = 4000 + i assert final_remaining == expected_remaining, "Inconsistent state suggests a race condition" \ No newline at end of file