Compare commits

...

3 Commits

Author SHA1 Message Date
Rodin 64d82cd561 nit(#154): add t.Fatal guard if baseSubprocessArgs flag not found
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 16s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 26s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 53s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m10s
Address sonnet NIT: if --repo or --pr is ever removed from
baseSubprocessArgs(), the mutation loop silently no-ops and the test
becomes meaningless. Adding a found guard and t.Fatal makes the
regression immediately visible.
2026-05-15 08:00:27 -07:00
Rodin 2892dff95d refactor(#154): use flag-name lookup in InvalidRepo and InvalidPRNumber tests
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 37s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m1s
Make the override code robust against base-arg order changes: search for
the flag name ('--repo', '--pr') and replace the following value, rather
than searching for the value string itself.

This prevents a subtle bug if the same value appeared elsewhere in the
base args before the target flag.
2026-05-15 01:51:19 -07:00
Rodin e718cb841a refactor(#154): extract baseSubprocessArgs helper in main_test.go subprocess tests
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 44s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m40s
Reduces duplication across 7 subprocess test functions that repeated the
same set of required flags verbatim. When required flags change, only
baseSubprocessArgs() needs updating.

Changes:
- Add baseSubprocessArgs() helper (analogous to cleanEnv())
- InvalidReviewerName, InvalidTemperature, InvalidProvider,
  ConflictingPersonaFlags: use append(baseSubprocessArgs(), ...)
- InvalidRepo: mutate a copy from baseSubprocessArgs() to set invalid --repo
- InvalidPRNumber: mutate a copy from baseSubprocessArgs() to set invalid --pr
- MissingLLMBaseURL, MissingAICoreCredentials, DeprecatedGiteaURLEnv:
  keep inline args (intentionally omit base flags); add comment explaining why
- All 5 old tests migrated from deprecated --gitea-url to canonical --vcs-url

All subprocess tests pass. go vet clean.
2026-05-15 01:50:15 -07:00
+60 -56
View File
@@ -880,16 +880,9 @@ func TestMainSubprocess_MissingFlags(t *testing.T) {
func TestMainSubprocess_InvalidReviewerName(t *testing.T) { func TestMainSubprocess_InvalidReviewerName(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot", os.Args = append(baseSubprocessArgs(),
"--gitea-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-name", "invalid name", "--reviewer-name", "invalid name",
"--reviewer-token", "tok", )
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
}
main() main()
return return
} }
@@ -908,15 +901,20 @@ func TestMainSubprocess_InvalidReviewerName(t *testing.T) {
func TestMainSubprocess_InvalidRepo(t *testing.T) { func TestMainSubprocess_InvalidRepo(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot", args := baseSubprocessArgs()
"--gitea-url", "http://localhost", // Replace the canonical --repo value with an invalid one.
"--repo", "invalidrepo", found := false
"--pr", "1", for i, a := range args {
"--reviewer-token", "tok", if a == "--repo" && i+1 < len(args) {
"--llm-base-url", "http://localhost", args[i+1] = "invalidrepo"
"--llm-api-key", "key", found = true
"--llm-model", "model", break
} }
}
if !found {
t.Fatal("baseSubprocessArgs() does not contain --repo; test is broken")
}
os.Args = args
main() main()
return return
} }
@@ -935,15 +933,20 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) {
func TestMainSubprocess_InvalidPRNumber(t *testing.T) { func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot", args := baseSubprocessArgs()
"--gitea-url", "http://localhost", // Replace the canonical --pr value with a non-numeric string.
"--repo", "owner/repo", found := false
"--pr", "notanumber", for i, a := range args {
"--reviewer-token", "tok", if a == "--pr" && i+1 < len(args) {
"--llm-base-url", "http://localhost", args[i+1] = "notanumber"
"--llm-api-key", "key", found = true
"--llm-model", "model", break
} }
}
if !found {
t.Fatal("baseSubprocessArgs() does not contain --pr; test is broken")
}
os.Args = args
main() main()
return return
} }
@@ -962,16 +965,9 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
func TestMainSubprocess_InvalidTemperature(t *testing.T) { func TestMainSubprocess_InvalidTemperature(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot", os.Args = append(baseSubprocessArgs(),
"--gitea-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
"--llm-temperature", "5.0", "--llm-temperature", "5.0",
} )
main() main()
return return
} }
@@ -990,16 +986,9 @@ func TestMainSubprocess_InvalidTemperature(t *testing.T) {
func TestMainSubprocess_InvalidProvider(t *testing.T) { func TestMainSubprocess_InvalidProvider(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot", os.Args = append(baseSubprocessArgs(),
"--gitea-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
"--llm-provider", "invalid-provider", "--llm-provider", "invalid-provider",
} )
main() main()
return return
} }
@@ -1015,6 +1004,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 // cleanEnv returns environ without any GITEA/LLM/REVIEWER/VCS env vars that would
// interfere with testing missing-flag scenarios. // interfere with testing missing-flag scenarios.
func cleanEnv() []string { func cleanEnv() []string {
@@ -1389,13 +1397,14 @@ func TestFetchPatterns_MultipleRepos(t *testing.T) {
func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) { func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) 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", os.Args = []string{"review-bot",
"--vcs-url", "https://gitea.example.com", "--vcs-url", "https://gitea.example.com",
"--repo", "owner/repo", "--repo", "owner/repo",
"--pr", "1", "--pr", "1",
"--reviewer-token", "tok", "--reviewer-token", "tok",
"--llm-model", "gpt-4", "--llm-model", "gpt-4",
// --llm-base-url and --llm-api-key intentionally omitted
} }
main() main()
return return
@@ -1417,6 +1426,8 @@ func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) {
func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) { func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) 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", os.Args = []string{"review-bot",
"--vcs-url", "https://gitea.example.com", "--vcs-url", "https://gitea.example.com",
"--repo", "owner/repo", "--repo", "owner/repo",
@@ -1446,17 +1457,10 @@ func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) {
func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) { func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot", os.Args = append(baseSubprocessArgs(),
"--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", "security",
"--persona-file", "custom.json", "--persona-file", "custom.json",
} )
main() main()
return return
} }
@@ -1477,9 +1481,9 @@ func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) {
func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) { func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
// Set required flags but omit --vcs-url; GITEA_URL should be picked up. // Note: cannot use baseSubprocessArgs() here because --vcs-url must be
// The test will exit with an error after VCS init (no PR to fetch), but // omitted — this test verifies that GITEA_URL env var is picked up as a
// the deprecation warning must appear. // deprecated fallback when --vcs-url is absent.
os.Args = []string{"review-bot", os.Args = []string{"review-bot",
// No --vcs-url: should fall back to GITEA_URL env var // No --vcs-url: should fall back to GITEA_URL env var
"--repo", "owner/repo", "--repo", "owner/repo",