vrr/open-code-review
2
0
Fork 0
mirror of https://github.com/alibaba/open-code-review.git synced 2026-08-20 14:14:30 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

refactor(test): unify config compat helpers to use cobra subcommand tree (#656)

Browse source 
runConfig manually dispatched config subcommands and parseConfigArgs did manual arg slicing, duplicating the routing the production config command already defines with cobra. Rebuild runConfig as a fresh cobra tree mirroring production (matching the runSession pattern), and drop parseConfigArgs, configAction, and configParseError, which are no longer used. Dispatch behavior is unchanged and stays covered by the existing runConfig/config_dispatch tests.

Closes #638
This commit is contained in:
Do Tuan Anh 2026-08-01 10:22:15 +07:00 • committed by GitHub
parent e78474478f
commit 575dfee97e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 33 additions and 128 deletions
Download patch file Download diff file Expand all files Collapse all files

90
cmd/opencodereview/compat_test.go
View file

@ -20,35 +20,6 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
return opts, err
}
// parseConfigArgs provides test compatibility for config argument parsing.
type configAction struct {
subCmd string
key string
value string
}
func parseConfigArgs(args []string) (configAction, error) {
if len(args) == 0 {
return configAction{}, &configParseError{"usage: ocr config set <key> <value>\ne.g., ocr config set llm.model claude-opus-4-6"}
}
subCmd := args[0]
switch subCmd {
case "set":
if len(args) < 3 {
return configAction{}, &configParseError{"usage: ocr config set <key> <value>\ne.g., ocr config set llm.model claude-opus-4-6"}
}
return configAction{subCmd: "set", key: args[1], value: args[2]}, nil
case "unset":
if len(args) < 2 {
return configAction{}, &configParseError{"usage: ocr config unset <provider|custom_providers.<name>|mcp_servers.<name>>\nexamples:\n ocr config unset provider\n ocr config unset custom_providers.my-provider\n ocr config unset mcp_servers.github"}
}
return configAction{subCmd: "unset", key: args[1]}, nil
default:
return configAction{}, &configParseError{"unknown config sub-command: " + subCmd + "\nAvailable: set, unset, provider, model"}
}
}
// parseScanFlags provides test compatibility: parses args through a fresh
// cobra command instance and returns the resulting scanOptions.
func parseScanFlags(args []string) (scanOptions, error) {
@ -68,10 +39,6 @@ func parseScanFlags(args []string) (scanOptions, error) {
return opts, err
}
type configParseError struct{ msg string }
func (e *configParseError) Error() string { return e.msg }
// runSessionListCompat provides test compatibility for old-style runSessionList([]string{...}) calls.
func runSessionListCompat(args []string) error {
cmd := &cobra.Command{
@ -129,38 +96,37 @@ func runSession(args []string) error {
return cmd.Execute()
}
// Ensure configParseError implements the error interface.
var _ error = (*configParseError)(nil)
// runConfig provides test compatibility: dispatches config subcommands.
// runConfig provides test compatibility: dispatches config subcommands through a
// fresh cobra command tree that mirrors the production config command, so the
// test compat layer cannot drift from production routing.
func runConfig(args []string) error {
if len(args) == 0 {
return nil
cmd := &cobra.Command{Use: "config", SilenceUsage: true, SilenceErrors: true}
setCmd := &cobra.Command{
Use: "set", Args: cobra.ExactArgs(2),
SilenceUsage: true, SilenceErrors: true,
RunE: func(cmd *cobra.Command, a []string) error { return runConfigSet(a[0], a[1]) },
}
switch args[0] {
case "provider":
if len(args) != 1 {
return &configParseError{"config provider does not accept arguments; use 'ocr config set provider <name>' for non-interactive setup"}
}
return runConfigProvider()
case "model":
if len(args) != 1 {
return &configParseError{"config model does not accept arguments; use 'ocr config set model <name>' for non-interactive setup"}
}
return runConfigModel()
unsetCmd := &cobra.Command{
Use: "unset", Args: cobra.ExactArgs(1),
SilenceUsage: true, SilenceErrors: true,
RunE: func(cmd *cobra.Command, a []string) error { return runConfigUnset(a[0]) },
}
action, err := parseConfigArgs(args)
if err != nil {
return err
providerCmd := &cobra.Command{
Use: "provider", Args: cobra.NoArgs,
SilenceUsage: true, SilenceErrors: true,
RunE: func(cmd *cobra.Command, a []string) error { return runConfigProvider() },
}
switch action.subCmd {
case "set":
return runConfigSet(action.key, action.value)
case "unset":
return runConfigUnset(action.key)
default:
return &configParseError{"unknown config sub-command: " + action.subCmd}
modelCmd := &cobra.Command{
Use: "model", Args: cobra.NoArgs,
SilenceUsage: true, SilenceErrors: true,
RunE: func(cmd *cobra.Command, a []string) error { return runConfigModel() },
}
cmd.AddCommand(setCmd, unsetCmd, providerCmd, modelCmd)
// A nil args slice makes cobra fall back to os.Args; pass an explicit empty
// slice so `runConfig(nil)` shows usage instead of reading the test binary's args.
if args == nil {
args = []string{}
}
cmd.SetArgs(args)
return cmd.Execute()
}

22
cmd/opencodereview/config_cmd_test.go
View file

@ -369,25 +369,9 @@ func TestSetConfigValueCustomProviderExtraHeaders(t *testing.T) {
// --- unset tests ---
func TestParseConfigArgsUnset(t *testing.T) {
action, err := parseConfigArgs([]string{"unset", "custom_providers.my-gateway"})
if err != nil {
t.Fatalf("parseConfigArgs: %v", err)
}
if action.subCmd != "unset" {
t.Errorf("subCmd = %q, want %q", action.subCmd, "unset")
}
if action.key != "custom_providers.my-gateway" {
t.Errorf("key = %q, want %q", action.key, "custom_providers.my-gateway")
}
}
func TestParseConfigArgsUnsetMissingKey(t *testing.T) {
_, err := parseConfigArgs([]string{"unset"})
if err == nil {
t.Fatal("expected error for missing key")
}
}
// parseConfigArgs unset tests were removed with the helper; unset dispatch is
// covered by the TestRunConfigUnset_* tests and config_dispatch_test.go, and the
// deeper unset behavior by the TestDeleteCustomProvider / provider tests below.
func TestUnsetCustomProvider(t *testing.T) {
dir := t.TempDir()

49
cmd/opencodereview/flags_test.go
View file

@ -147,50 +147,5 @@ func TestParseReviewFlags_ShortFlags(t *testing.T) {
}
}
func TestParseConfigArgs_Empty(t *testing.T) {
_, err := parseConfigArgs(nil)
if err == nil {
t.Fatal("expected error for empty args")
}
}
func TestParseConfigArgs_Set(t *testing.T) {
act, err := parseConfigArgs([]string{"set", "llm.model", "gpt-4"})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if act.subCmd != "set" || act.key != "llm.model" || act.value != "gpt-4" {
t.Errorf("got %+v", act)
}
}
func TestParseConfigArgs_SetMissingValue(t *testing.T) {
_, err := parseConfigArgs([]string{"set", "llm.model"})
if err == nil {
t.Fatal("expected error for missing value")
}
}
func TestParseConfigArgs_Unset(t *testing.T) {
act, err := parseConfigArgs([]string{"unset", "custom_providers.foo"})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if act.subCmd != "unset" || act.key != "custom_providers.foo" {
t.Errorf("got %+v", act)
}
}
func TestParseConfigArgs_UnsetMissingKey(t *testing.T) {
_, err := parseConfigArgs([]string{"unset"})
if err == nil {
t.Fatal("expected error for missing key")
}
}
func TestParseConfigArgs_UnknownSubCmd(t *testing.T) {
_, err := parseConfigArgs([]string{"delete", "foo"})
if err == nil {
t.Fatal("expected error for unknown subcommand")
}
}
// Config argument parsing is exercised through runConfig (see
// config_dispatch_test.go), which now routes via the production cobra tree.