From d75e737f07baf82ba66aef78760ced91b4d90a8e Mon Sep 17 00:00:00 2001 From: claw Date: Thu, 14 May 2026 21:15:12 -0700 Subject: [PATCH] test(#139): improve cmd/review-bot coverage from 44.6% to 49.3% MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add tests for previously uncovered paths: - TestIsValidateError_Nil: isValidateError(nil, ...) returns false - TestValidateURL_EmptyHost: URL with no hostname (https://) → code-2 error - TestRunValidateURL_Success: success path (OK output + exit 0) via example.com - TestMainSubprocess_MissingLLMBaseURL: --llm-base-url required for openai provider - TestMainSubprocess_MissingAICoreCredentials: aicore creds required for provider=aicore - TestMainSubprocess_ConflictingPersonaFlags: --persona and --persona-file are mutually exclusive - TestMainSubprocess_DeprecatedGiteaURLEnv: GITEA_URL env var emits deprecation warning All tests pass; no production code changes; dep check clean. --- cmd/review-bot/main_test.go | 123 +++++++++++++++++++++++++++++ cmd/review-bot/validateurl_test.go | 57 +++++++++++++ 2 files changed, 180 insertions(+) diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 8ef293c..22b7a6c 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -1383,3 +1383,126 @@ func TestFetchPatterns_MultipleRepos(t *testing.T) { t.Errorf("expected Elixir pipes content, got: %q", got) } } + +// TestMainSubprocess_MissingLLMBaseURL confirms that --llm-base-url is required +// when provider=openai (the default). +func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--vcs-url", "https://gitea.example.com", + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-model", "gpt-4", + // --llm-base-url and --llm-api-key intentionally omitted + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_MissingLLMBaseURL") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit when llm-base-url is missing") + } + if !strings.Contains(string(out), "llm-base-url") { + t.Errorf("expected error mentioning llm-base-url, got: %s", out) + } +} + +// TestMainSubprocess_MissingAICoreCredentials confirms that aicore-specific credentials +// are required when provider=aicore. +func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--vcs-url", "https://gitea.example.com", + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-model", "gpt-4", + "--llm-provider", "aicore", + // aicore-client-id, aicore-client-secret, aicore-auth-url, aicore-api-url omitted + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_MissingAICoreCredentials") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit when aicore credentials are missing") + } + if !strings.Contains(string(out), "AI Core credentials") { + t.Errorf("expected error about AI Core credentials, got: %s", out) + } +} + +// TestMainSubprocess_ConflictingPersonaFlags confirms that --persona and --persona-file +// cannot be used together. +func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--vcs-url", "https://gitea.example.com", + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-base-url", "https://api.example.com", + "--llm-api-key", "key", + "--llm-model", "gpt-4", + "--persona", "security", + "--persona-file", "custom.json", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_ConflictingPersonaFlags") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with both --persona and --persona-file set") + } + if !strings.Contains(string(out), "mutually exclusive") { + t.Errorf("expected error about mutually exclusive flags, got: %s", out) + } +} + +// TestMainSubprocess_DeprecatedGiteaURLEnv confirms that GITEA_URL env var still works +// as a deprecated fallback for VCS_URL. +func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + // Set required flags but omit --vcs-url; GITEA_URL should be picked up. + // The test will exit with an error after VCS init (no PR to fetch), but + // the deprecation warning must appear. + os.Args = []string{"review-bot", + // No --vcs-url: should fall back to GITEA_URL env var + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-base-url", "https://api.example.com", + "--llm-api-key", "key", + "--llm-model", "gpt-4", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_DeprecatedGiteaURLEnv") + // Inject GITEA_URL but NOT VCS_URL. + env := append(cleanEnv(), + "TEST_SUBPROCESS_MAIN=1", + "GITEA_URL=https://gitea.example.com", + ) + cmd.Env = env + out, _ := cmd.CombinedOutput() + // The process will fail (no real server), but the deprecation warning must appear. + if !strings.Contains(string(out), "deprecated") { + t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out) + } +} diff --git a/cmd/review-bot/validateurl_test.go b/cmd/review-bot/validateurl_test.go index aca1cfb..136d141 100644 --- a/cmd/review-bot/validateurl_test.go +++ b/cmd/review-bot/validateurl_test.go @@ -125,3 +125,60 @@ func TestRunValidateURL_WithCapture(t *testing.T) { t.Errorf("expected error about https in stderr, got %q", errBuf.String()) } } + +// TestIsValidateError_Nil confirms that isValidateError returns false for a nil error. +func TestIsValidateError_Nil(t *testing.T) { + var ve *validateError + if isValidateError(nil, &ve) { + t.Error("isValidateError(nil, ...) should return false") + } +} + +// TestValidateURL_EmptyHost confirms that a URL with no hostname returns a code-2 error. +func TestValidateURL_EmptyHost(t *testing.T) { + // "https://" parses fine but has no hostname. + err := validateURL("https://") + if err == nil { + t.Fatal("expected error for URL with no host, got nil") + } + var ve *validateError + if !isValidateError(err, &ve) { + t.Fatalf("expected *validateError, got %T: %v", err, err) + } + if ve.code != 2 { + t.Errorf("expected code 2, got %d (msg=%s)", ve.code, ve.message) + } + if !strings.Contains(ve.message, "no host") { + t.Errorf("expected 'no host' in error message, got %q", ve.message) + } +} + +// TestRunValidateURL_Success confirms that a resolvable public URL prints "OK" and returns 0. +// This test requires external DNS; it is skipped in environments without network access. +func TestRunValidateURL_Success(t *testing.T) { + // Pre-check: validate that DNS is available before exercising the success path. + err := validateURL("https://example.com/") + if err != nil { + t.Skipf("skipping success-path test: DNS unavailable or example.com blocked (%v)", err) + } + + var outBuf, errBuf bytes.Buffer + origOut, origErr := outWriter, errWriter + outWriter = &outBuf + errWriter = &errBuf + defer func() { + outWriter = origOut + errWriter = origErr + }() + + code := runValidateURL([]string{"https://example.com/"}) + if code != 0 { + t.Errorf("expected exit code 0 for safe URL, got %d (stderr: %s)", code, errBuf.String()) + } + if !strings.Contains(outBuf.String(), "OK:") { + t.Errorf("expected 'OK:' in stdout, got %q", outBuf.String()) + } + if errBuf.Len() != 0 { + t.Errorf("expected no stderr for safe URL, got %q", errBuf.String()) + } +}