feat: improve test coverage for cmd/review-bot #37
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: rodin/review-bot#37
Reference in New Issue
Block a user
Delete Branch "feat/32-test-coverage"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Increases test coverage for
cmd/review-botfrom 16.7% to 42.3%, exceeding the 40% target.Partially addresses #32
Changes
Unit tests for pure functions:
isPatternFile— extension matching logicevaluateCIStatus— CI status evaluation (all pass, failures, empty, mixed, pending)envOrDefault/envOrDefaultFloat/envOrDefaultInt/envOrDefaultBool— env var parsingextractSentinelName— edge casesSubprocess tests for
main()error paths:--versionflagIntegration test scaffold (
cmd/review-bot/integration_test.go)://go:build integrationMakefile with build/test/lint/coverage targets
Added
coverage.outto.gitignoreCoverage
Verification
Security Review
Summary
This PR adds unit and integration tests plus simple tooling (Makefile, .gitignore) without modifying production code. The changes do not introduce security-relevant behavior; integration tests are gated behind a build tag and do not log secrets.
Recommendation
APPROVE — Approve the changes. The added tests and tooling are safe from a security perspective and CI has passed. Integration tests are opt-in via build tags and avoid exposing sensitive environment variables or credentials in logs.
Review by security
Gpt Review
Summary
This PR substantially increases test coverage using idiomatic, thorough, and maintainable test code for cmd/review-bot. No issues found.
Recommendation
APPROVE — The changes meet repository conventions, follow Go testing idioms, and improve project health by raising test coverage above the stated goal. All new tests are well structured, isolated, and cover normal and edge cases; subprocess/integration patterns are idiomatic. CI passes and no code quality issues are present. This PR is ready to merge.
Review by gpt
Security Review
Summary
The changes add unit and integration tests, a Makefile, and a minor .gitignore update. No production code paths were modified, and CI has passed. The additions are generally safe; only low-risk information exposure considerations exist in integration test logs.
Findings
cmd/review-bot/integration_test.gocmd/review-bot/integration_test.goRecommendation
APPROVE — The PR primarily adds tests and build tooling with no changes to production logic. Security impact is minimal. To harden further, consider truncating or redacting the review body in integration test logs and ensure integration tests are constrained to non-production environments with least-privilege tokens. With CI passing and no exploitable vulnerabilities introduced, this change can be merged.
Review by security
@@ -0,0 +100,4 @@if body == "" {t.Fatal("formatted review body is empty")}t.Logf("Review body:\n%s", body)[MINOR] Integration test logs the full formatted review body (which may include PR content and LLM output). If run in shared environments with verbose logs or on failure, this could expose potentially sensitive repository content in logs. Consider truncating or omitting the full body in logs.
@@ -0,0 +130,4 @@// Post a test reviewsentinel := "<!-- review-bot:integration-test -->"testBody := "# Integration Test Review\n\nThis is a test review.\n\n" + sentinelposted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, nil)[MINOR] Integration test posts a real review to the configured Gitea instance. While it attempts cleanup, running against a production repo or with high-privilege tokens could cause unintended side effects. Ensure tests use a dedicated test repo and least-privilege tokens, or add explicit safeguards (e.g., environment variable checks or repo whitelisting).
Sonnet Review
Summary
Solid test additions significantly improve coverage with clear separation between unit and integration tests. CI is passing and the changes adhere to repository conventions and Go testing idioms.
Findings
cmd/review-bot/integration_test.gocmd/review-bot/integration_test.gocmd/review-bot/main_test.goRecommendation
APPROVE — The additions are well-structured and comprehensive, covering pure functions, subprocess error paths, and providing an opt-in integration suite via build tags. Tests follow table-driven patterns and avoid external dependencies in unit tests, aligning with repository conventions. To further improve robustness, add timeouts to the integration test contexts and ensure subtest names are always non-empty for clarity. Otherwise, the PR is ready to merge.
Review by sonnet
@@ -0,0 +53,4 @@t.Fatalf("Invalid repo format %q", giteaRepo)}ctx := context.Background()[MINOR] Integration tests use context.Background() for network calls; consider context.WithTimeout to avoid potential indefinite hangs against external services.
@@ -0,0 +124,4 @@}owner, repoName := parts[0], parts[1]ctx := context.Background()[MINOR] Integration tests use context.Background() without a timeout when posting and listing reviews; using context.WithTimeout would make tests more robust to slow or unresponsive endpoints.
@@ -368,0 +388,4 @@{"doc.pdf", false},{"patterns.Yml", true},{"deep/path/file.yaml", true},}[NIT] Subtest name derives from the file path (t.Run(tc.path)) and one case is an empty string; using a descriptive, non-empty subtest name would improve readability and avoid odd test names.