Overview
PR #3 added a solid test foundation with 9 tests, but left some critical paths untested. This issue tracks follow-up tests to improve coverage of rate-limit handling, global state management, and edge cases.
Missing Test Coverage
1. retry-after header priority in wait_for_rate_limit (High Priority)
The production code in main.py lines 19-24 prioritizes the Retry-After header over X-RateLimit-Reset, but no test validates this behavior:
retry_after = response.headers.get("retry-after")
if retry_after and retry_after.isdigit():
wait = int(retry_after) + 2
Missing test: test_retry_after_header_takes_priority_over_reset — verify that when both headers are present, Retry-After is used.
2. Global state pollution risk (High Priority)
The _last_reset_time global variable is never reset between tests. If tests run in different orders or if one test sets the global state, subsequent tests may fail.
Current state:
# main.py
_last_reset_time: int = 0 # global, never reset in tests
Missing fixture: Create a pytest fixture that resets _last_reset_time to 0 before each test:
@pytest.fixture(autouse=True)
def reset_rate_limit_state(monkeypatch):
"""Reset global rate-limit state before each test."""
import main
monkeypatch.setattr(main, "_last_reset_time", 0)
3. _check_rate_limit_before_request() function (High Priority)
This function manages the cached rate-limit state and is called on every request, but has zero coverage.
Code path (main.py lines 35-42):
def _check_rate_limit_before_request():
"""Sleep until the cached rate-limit reset time, if we know one is pending."""
global _last_reset_time
now = int(time.time())
if _last_reset_time > now:
wait = _last_reset_time - now + 2
print(...)
time.sleep(wait)
Missing test: test_check_rate_limit_before_request_waits_when_time_in_future — verify that the function sleeps until the cached reset time expires.
Missing test: test_check_rate_limit_before_request_skips_when_time_expired — verify that it skips sleep when reset time is in the past.
4. Success response with zero remaining quota (Medium Priority)
When a 200 response arrives with X-RateLimit-Remaining: 0, the code should preemptively cache the reset time for the next request (lines 53-58):
if remaining is not None and int(remaining) == 0 and reset_ts is not None:
_last_reset_time = int(reset_ts)
print("Rate limit will be exhausted...")
Missing test: test_success_with_zero_remaining_caches_reset_time — verify the cache is updated so the next request will wait.
5. Deduplication and PR filtering in integration test (Medium Priority)
test_produces_output_file doesn't validate the actual parsing logic. Line 153 filters out duplicates and pull requests:
if issue["id"] in seen_ids or "pull_request" in issue:
continue
Missing test: test_deduplication_filters_duplicate_and_pr_issues — verify duplicate issues and PR results are filtered from output.
6. Timeout vs. ConnectionError backoff differentiation (Medium Priority)
The code has different retry delays:
Timeout (line 70): 5s between retries
ConnectionError (line 73): 10s between retries
Currently there's only one combined test. Should have separate tests to ensure backoff strategy is correct.
Missing tests:
test_timeout_uses_5s_backoff
test_connection_error_uses_10s_backoff
7. Polite delay between pages and queries (Low Priority)
Lines 180 & 194 have time.sleep(6) calls that should be validated. A future refactor could accidentally remove these.
Optional test: Verify time.sleep is called with the correct delays during pagination and between queries.
8. Fixture cleanup: empty_env needs yield (Low Priority)
The empty_env fixture in tests/conftest.py doesn't have a yield statement, so it doesn't properly execute the test.
Fix:
@pytest.fixture
def empty_env(monkeypatch):
"""Remove GITHUB_TOKEN so the unauthenticated path is exercised."""
monkeypatch.delenv("GITHUB_TOKEN", raising=False)
yield # <-- missing
Acceptance Criteria
Related
Overview
PR #3 added a solid test foundation with 9 tests, but left some critical paths untested. This issue tracks follow-up tests to improve coverage of rate-limit handling, global state management, and edge cases.
Missing Test Coverage
1.
retry-afterheader priority inwait_for_rate_limit(High Priority)The production code in
main.pylines 19-24 prioritizes theRetry-Afterheader overX-RateLimit-Reset, but no test validates this behavior:Missing test:
test_retry_after_header_takes_priority_over_reset— verify that when both headers are present,Retry-Afteris used.2. Global state pollution risk (High Priority)
The
_last_reset_timeglobal variable is never reset between tests. If tests run in different orders or if one test sets the global state, subsequent tests may fail.Current state:
Missing fixture: Create a pytest fixture that resets
_last_reset_timeto 0 before each test:3.
_check_rate_limit_before_request()function (High Priority)This function manages the cached rate-limit state and is called on every request, but has zero coverage.
Code path (main.py lines 35-42):
Missing test:
test_check_rate_limit_before_request_waits_when_time_in_future— verify that the function sleeps until the cached reset time expires.Missing test:
test_check_rate_limit_before_request_skips_when_time_expired— verify that it skips sleep when reset time is in the past.4. Success response with zero remaining quota (Medium Priority)
When a 200 response arrives with
X-RateLimit-Remaining: 0, the code should preemptively cache the reset time for the next request (lines 53-58):Missing test:
test_success_with_zero_remaining_caches_reset_time— verify the cache is updated so the next request will wait.5. Deduplication and PR filtering in integration test (Medium Priority)
test_produces_output_filedoesn't validate the actual parsing logic. Line 153 filters out duplicates and pull requests:Missing test:
test_deduplication_filters_duplicate_and_pr_issues— verify duplicate issues and PR results are filtered from output.6. Timeout vs. ConnectionError backoff differentiation (Medium Priority)
The code has different retry delays:
Timeout(line 70): 5s between retriesConnectionError(line 73): 10s between retriesCurrently there's only one combined test. Should have separate tests to ensure backoff strategy is correct.
Missing tests:
test_timeout_uses_5s_backofftest_connection_error_uses_10s_backoff7. Polite delay between pages and queries (Low Priority)
Lines 180 & 194 have
time.sleep(6)calls that should be validated. A future refactor could accidentally remove these.Optional test: Verify
time.sleepis called with the correct delays during pagination and between queries.8. Fixture cleanup:
empty_envneeds yield (Low Priority)The
empty_envfixture intests/conftest.pydoesn't have ayieldstatement, so it doesn't properly execute the test.Fix:
Acceptance Criteria
test_retry_after_header_takes_priority_over_resetadded and passingreset_rate_limit_statefixture added and applied to all teststest_check_rate_limit_before_request_waits_when_time_in_futureadded and passingtest_check_rate_limit_before_request_skips_when_time_expiredadded and passingtest_success_with_zero_remaining_caches_reset_timeadded and passingtest_deduplication_filters_duplicate_and_pr_issuesadded and passingempty_envfixture fixed withyieldstatementRelated