From 77a7f667cb2b89e0e3866a8eb24f342c94bd1955 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 10:18:34 +0000 Subject: [PATCH] refactor(#154): extract baseSubprocessArgs helper in main_test.go subprocess tests --- cmd/review-bot/main_test.go | 106 +++++++++++++++++------------------- 1 file changed, 50 insertions(+), 56 deletions(-) diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index c7a3675..6b8d75b 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -880,16 +880,9 @@ func TestMainSubprocess_MissingFlags(t *testing.T) { func TestMainSubprocess_InvalidReviewerName(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) - os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", - "--repo", "owner/repo", - "--pr", "1", + os.Args = append(baseSubprocessArgs(), "--reviewer-name", "invalid name", - "--reviewer-token", "tok", - "--llm-base-url", "http://localhost", - "--llm-api-key", "key", - "--llm-model", "model", - } + ) main() return } @@ -908,15 +901,15 @@ func TestMainSubprocess_InvalidReviewerName(t *testing.T) { func TestMainSubprocess_InvalidRepo(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) - os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", - "--repo", "invalidrepo", - "--pr", "1", - "--reviewer-token", "tok", - "--llm-base-url", "http://localhost", - "--llm-api-key", "key", - "--llm-model", "model", + args := baseSubprocessArgs() + // Replace the canonical --repo value with an invalid one. + for i, a := range args { + if a == "--repo" && i+1 < len(args) { + args[i+1] = "invalidrepo" + break + } } + os.Args = args main() return } @@ -935,15 +928,15 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) { func TestMainSubprocess_InvalidPRNumber(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) - os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", - "--repo", "owner/repo", - "--pr", "notanumber", - "--reviewer-token", "tok", - "--llm-base-url", "http://localhost", - "--llm-api-key", "key", - "--llm-model", "model", + args := baseSubprocessArgs() + // Replace the canonical --pr value with a non-numeric string. + for i, a := range args { + if a == "--pr" && i+1 < len(args) { + args[i+1] = "notanumber" + break + } } + os.Args = args main() return } @@ -962,16 +955,9 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) { func TestMainSubprocess_InvalidTemperature(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) - os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", - "--repo", "owner/repo", - "--pr", "1", - "--reviewer-token", "tok", - "--llm-base-url", "http://localhost", - "--llm-api-key", "key", - "--llm-model", "model", + os.Args = append(baseSubprocessArgs(), "--llm-temperature", "5.0", - } + ) main() return } @@ -990,16 +976,9 @@ func TestMainSubprocess_InvalidTemperature(t *testing.T) { func TestMainSubprocess_InvalidProvider(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) - os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", - "--repo", "owner/repo", - "--pr", "1", - "--reviewer-token", "tok", - "--llm-base-url", "http://localhost", - "--llm-api-key", "key", - "--llm-model", "model", + os.Args = append(baseSubprocessArgs(), "--llm-provider", "invalid-provider", - } + ) main() return } @@ -1015,6 +994,25 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) { } } +// baseSubprocessArgs returns the base set of required flags for subprocess tests +// that need a fully-configured main() invocation. Each test appends its own +// test-specific flags on top of this base. +// +// Using a helper here means that when the set of required flags changes, only +// this function needs updating (instead of every test that passes all flags). +func baseSubprocessArgs() []string { + return []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", + } +} + // cleanEnv returns environ without any GITEA/LLM/REVIEWER/VCS env vars that would // interfere with testing missing-flag scenarios. func cleanEnv() []string { @@ -1389,13 +1387,14 @@ func TestFetchPatterns_MultipleRepos(t *testing.T) { func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + // Note: cannot use baseSubprocessArgs() here because --llm-base-url and + // --llm-api-key are intentionally omitted to test the missing-URL error. 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 @@ -1417,6 +1416,8 @@ func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) { func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + // Note: cannot use baseSubprocessArgs() here because aicore provider + // does not require --llm-base-url / --llm-api-key; those are omitted. os.Args = []string{"review-bot", "--vcs-url", "https://gitea.example.com", "--repo", "owner/repo", @@ -1446,17 +1447,10 @@ func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) { 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", + os.Args = append(baseSubprocessArgs(), "--persona", "security", "--persona-file", "custom.json", - } + ) main() return } @@ -1477,9 +1471,9 @@ func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) { 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. + // Note: cannot use baseSubprocessArgs() here because --vcs-url must be + // omitted — this test verifies that GITEA_URL env var is picked up as a + // deprecated fallback when --vcs-url is absent. os.Args = []string{"review-bot", // No --vcs-url: should fall back to GITEA_URL env var "--repo", "owner/repo",