nimmo/forgejo-auto-pr-review
Watch
1
0
Fork
You've already forked forgejo-auto-pr-review
 0
Code Issues 5 Pull requests Projects Releases Packages 1 Activity Actions

test: add mocked Forgejo and model coverage #11

Merged
nimmo merged 4 commits from test/issue-8-forgejo-fixtures into main 2026-07-20 17:18:23 +01:00
Conversation 1 Commits 4 Files changed 4 +200 -10
nimmo commented 2026-07-20 17:02:05 +01:00
Owner
Copy link

Implements the main test-foundation slice of #8.

Adds offline coverage for Forgejo request/config/diff/publishing paths, Ollama request and error handling, prompt-injection containment, deterministic-check limits, event dispatch, and the full-SHA regression found during rollout.

Validation: python3 -W error::ResourceWarning -m unittest discover -s tests (20 passing).

Implements the main test-foundation slice of #8. Adds offline coverage for Forgejo request/config/diff/publishing paths, Ollama request and error handling, prompt-injection containment, deterministic-check limits, event dispatch, and the full-SHA regression found during rollout. Validation: `python3 -W error::ResourceWarning -m unittest discover -s tests` (20 passing).
test: add mocked Forgejo and model coverage
All checks were successful
AI pull-request review / review (pull_request) Successful in 49s
Details
ad7de3b533
nimmo commented 2026-07-20 17:02:55 +01:00
Author
Owner
Copy link

Automated code review

Reviewed commit: 81853a5e78a83b69ea33e9e30939886b0bf8c704

Verdict: Requires further work

Resolve the blocking or important findings and investigate failed deterministic checks before merging.

Overall assessment

The pull request adds comprehensive unit tests and mock coverage for the Forgejo AI PR reviewer, including fixtures, API interaction mocks, diff handling, Ollama integration, error handling, and workflow validation. It also updates the workflow file to pin a new action revision and improves error messages in bounded_diff.

Blocking findings

  • Workflow SHA verification test is insufficiently strict (.forgejo/workflows/ai-review.yml:28; high confidence)
    The workflow uses commit SHA fc1861af45, but test_self_review_workflow_pins_a_full_action_revision only verifies a 40-character hex string exists via regex match (re.search(r"forgejo-auto-pr-review@([0-9a-f]+)", workflow)), not the exact value.
    Impact: The test does not ensure the workflow is pinned to the intended revision, allowing potential drift or incorrect updates without detection.
    Suggested fix: Update the test to assert the exact SHA value (fc1861af45) rather than just checking for a 40-character hex string.

Important findings

  • Missing test coverage for exact boundary in bounded_diff truncation (tests/test_reviewer.py:120; high confidence)
    The test 'test_bounded_diff_reports_truncation_and_sets_authentication' includes a case where diff length (4 bytes) equals max_bytes (4), but the assertion expects truncated=False. This does not clarify whether truncation occurs at > or >= limit.
    Impact: Ambiguity in boundary condition handling could cause inconsistent truncation behavior in production, leading to either over-truncation or under-truncation of diffs.
    Suggested fix: Add explicit test cases for diff lengths of (max_bytes - 1), max_bytes, and (max_bytes + 1) to ensure deterministic truncation logic at boundaries.

  • Test does not verify timeout handling in bounded_diff (tests/test_reviewer.py:120; high confidence)
    The test 'test_bounded_diff_reports_truncation_and_sets_authentication' mocks urllib.request.urlopen but does not cover cases where urlopen raises urllib.error.URLError with a timeout. However, the diff shows that bounded_diff now includes try/except handling for URLError and raises RuntimeError.
    Impact: Missing coverage for network timeouts in bounded_diff could lead to unhandled exceptions during diff fetching, causing reviewer failures.
    Suggested fix: Add a test case that mocks urlopen to raise urllib.error.URLError(TimeoutError(...)) and verifies appropriate handling (e.g., raising RuntimeError or returning empty diff).

Suggestions

None.

Tests and validation

  • unit-tests: success

Questions

  • Is the FakeResponse class sufficient for simulating all possible HTTP response behaviors (e.g., chunked responses, headers)?
  • Should tests include validation of the SYSTEM_PROMPT content to ensure it remains consistent with security requirements?
  • Are there additional edge cases in config parsing (e.g., empty YAML documents, circular references) that should be tested?

Review limitations

  • Tests use mocked HTTP requests and do not validate actual network behavior or real Ollama API interactions.
  • The pull_request_event.json fixture is minimal and may not cover all Forgejo event payload variations.
  • No tests verify the full end-to-end workflow with realistic diff sizes or complex guidance files.
Review metadata
  • Reviewer: forgejo-ai-review v1
  • Model: qwen3-coder-next:q4_K_M
  • Guidance loaded: .forgejo/ai-review.yml, README.md
  • Commits considered: 4
  • Previous automated review: updated
  • Findings: 3
<!-- forgejo-ai-review:v1 --> ## Automated code review Reviewed commit: `81853a5e78a83b69ea33e9e30939886b0bf8c704` ## Verdict: Requires further work Resolve the blocking or important findings and investigate failed deterministic checks before merging. ### Overall assessment The pull request adds comprehensive unit tests and mock coverage for the Forgejo AI PR reviewer, including fixtures, API interaction mocks, diff handling, Ollama integration, error handling, and workflow validation. It also updates the workflow file to pin a new action revision and improves error messages in bounded_diff. ### Blocking findings - **Workflow SHA verification test is insufficiently strict** (.forgejo/workflows/ai-review.yml:28; high confidence) The workflow uses commit SHA fc1861af4546ec8dabd4dbcda7603e9e7e56b90c, but test_self_review_workflow_pins_a_full_action_revision only verifies a 40-character hex string exists via regex match (re.search(r"forgejo-auto-pr-review@([0-9a-f]+)", workflow)), not the exact value. Impact: The test does not ensure the workflow is pinned to the intended revision, allowing potential drift or incorrect updates without detection. Suggested fix: Update the test to assert the exact SHA value (fc1861af4546ec8dabd4dbcda7603e9e7e56b90c) rather than just checking for a 40-character hex string. ### Important findings - **Missing test coverage for exact boundary in bounded_diff truncation** (tests/test_reviewer.py:120; high confidence) The test 'test_bounded_diff_reports_truncation_and_sets_authentication' includes a case where diff length (4 bytes) equals max_bytes (4), but the assertion expects truncated=False. This does not clarify whether truncation occurs at > or >= limit. Impact: Ambiguity in boundary condition handling could cause inconsistent truncation behavior in production, leading to either over-truncation or under-truncation of diffs. Suggested fix: Add explicit test cases for diff lengths of (max_bytes - 1), max_bytes, and (max_bytes + 1) to ensure deterministic truncation logic at boundaries. - **Test does not verify timeout handling in bounded_diff** (tests/test_reviewer.py:120; high confidence) The test 'test_bounded_diff_reports_truncation_and_sets_authentication' mocks urllib.request.urlopen but does not cover cases where urlopen raises urllib.error.URLError with a timeout. However, the diff shows that bounded_diff now includes try/except handling for URLError and raises RuntimeError. Impact: Missing coverage for network timeouts in bounded_diff could lead to unhandled exceptions during diff fetching, causing reviewer failures. Suggested fix: Add a test case that mocks urlopen to raise urllib.error.URLError(TimeoutError(...)) and verifies appropriate handling (e.g., raising RuntimeError or returning empty diff). ### Suggestions None. ### Tests and validation - unit-tests: **success** ### Questions - Is the FakeResponse class sufficient for simulating all possible HTTP response behaviors (e.g., chunked responses, headers)? - Should tests include validation of the SYSTEM_PROMPT content to ensure it remains consistent with security requirements? - Are there additional edge cases in config parsing (e.g., empty YAML documents, circular references) that should be tested? ### Review limitations - Tests use mocked HTTP requests and do not validate actual network behavior or real Ollama API interactions. - The pull_request_event.json fixture is minimal and may not cover all Forgejo event payload variations. - No tests verify the full end-to-end workflow with realistic diff sizes or complex guidance files. <details><summary>Review metadata</summary> - Reviewer: forgejo-ai-review v1 - Model: qwen3-coder-next:q4_K_M - Guidance loaded: .forgejo/ai-review.yml, README.md - Commits considered: 4 - Previous automated review: updated - Findings: 3 </details>
test: cover reviewer boundary cases
All checks were successful
AI pull-request review / review (pull_request) Successful in 54s
Details
26e342c012
fix: clarify diff fetch failures
All checks were successful
AI pull-request review / review (pull_request) Successful in 59s
Details
a7415488bb
fix: keep prior AI reviews out of prompts
All checks were successful
AI pull-request review / review (pull_request) Successful in 1m4s
Details
81853a5e78
nimmo merged commit d423442afc into main 2026-07-20 17:18:23 +01:00
nimmo deleted branch test/issue-8-forgejo-fixtures 2026-07-20 17:18:24 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
nimmo/forgejo-auto-pr-review!11
No description provided.