From 8170237dabe389cc47f264a622358bb807092b63 Mon Sep 17 00:00:00 2001 From: Omri Ariav Date: Fri, 20 Feb 2026 00:00:53 +0200 Subject: [PATCH] test: add command-level validation tests, move validation before client creation Add validation_test.go with tests that exercise run* functions through Cobra commands, catching validation errors before any API call: - Forms: --file + --title conflict, no-flags error - Sheets: --value required for conditional format rules, negative --index - Slides: invalid --size rejection - Contacts: update with no field flags Move flag validation before client.NewFactory() in all affected commands so validation errors fire before OAuth credential checks. This improves UX (faster, clearer errors) and makes the validations testable without a mock server. Co-Authored-By: Claude Opus 4.6 --- cmd/contacts.go | 39 +++++----- cmd/forms.go | 30 +++---- cmd/sheets.go | 40 +++++----- cmd/slides.go | 17 ++-- cmd/validation_test.go | 173 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 241 insertions(+), 58 deletions(-) create mode 100644 cmd/validation_test.go diff --git a/cmd/contacts.go b/cmd/contacts.go index c686235..ce70bbb 100644 --- a/cmd/contacts.go +++ b/cmd/contacts.go @@ -388,6 +388,26 @@ func runContactsDelete(cmd *cobra.Command, args []string) error { func runContactsUpdate(cmd *cobra.Command, args []string) error { p := printer.New(os.Stdout, GetFormat()) + + // Validate flags before creating API client + var updateFields []string + if cmd.Flags().Changed("name") { + updateFields = append(updateFields, "names") + } + if cmd.Flags().Changed("email") { + updateFields = append(updateFields, "emailAddresses") + } + if cmd.Flags().Changed("phone") { + updateFields = append(updateFields, "phoneNumbers") + } + if cmd.Flags().Changed("organization") || cmd.Flags().Changed("title") { + updateFields = append(updateFields, "organizations") + } + + if len(updateFields) == 0 { + return p.PrintError(fmt.Errorf("at least one field to update must be specified (--name, --email, --phone, --organization, --title)")) + } + ctx := context.Background() factory, err := client.NewFactory(ctx) @@ -409,25 +429,6 @@ func runContactsUpdate(cmd *cobra.Command, args []string) error { title, _ := cmd.Flags().GetString("title") etag, _ := cmd.Flags().GetString("etag") - // Determine which fields the user wants to update - var updateFields []string - if cmd.Flags().Changed("name") { - updateFields = append(updateFields, "names") - } - if cmd.Flags().Changed("email") { - updateFields = append(updateFields, "emailAddresses") - } - if cmd.Flags().Changed("phone") { - updateFields = append(updateFields, "phoneNumbers") - } - if cmd.Flags().Changed("organization") || cmd.Flags().Changed("title") { - updateFields = append(updateFields, "organizations") - } - - if len(updateFields) == 0 { - return p.PrintError(fmt.Errorf("at least one field to update must be specified (--name, --email, --phone, --organization, --title)")) - } - // Fetch existing contact — include metadata for source/etag required by the API fetchFields := append(updateFields, "metadata") existing, err := svc.People.Get(resourceName). diff --git a/cmd/forms.go b/cmd/forms.go index af34c9b..0fdb2d7 100644 --- a/cmd/forms.go +++ b/cmd/forms.go @@ -436,6 +436,23 @@ func runFormsCreate(cmd *cobra.Command, args []string) error { func runFormsUpdate(cmd *cobra.Command, args []string) error { p := printer.New(os.Stdout, GetFormat()) + + // Validate flags before creating API client + filePath, _ := cmd.Flags().GetString("file") + title, _ := cmd.Flags().GetString("title") + description, _ := cmd.Flags().GetString("description") + + hasFile := cmd.Flags().Changed("file") + hasTitle := cmd.Flags().Changed("title") + hasDescription := cmd.Flags().Changed("description") + + if hasFile && (hasTitle || hasDescription) { + return p.PrintError(fmt.Errorf("--file cannot be combined with --title or --description")) + } + if !hasFile && !hasTitle && !hasDescription { + return p.PrintError(fmt.Errorf("provide --file, --title, or --description")) + } + ctx := context.Background() factory, err := client.NewFactory(ctx) @@ -449,17 +466,6 @@ func runFormsUpdate(cmd *cobra.Command, args []string) error { } formID := args[0] - filePath, _ := cmd.Flags().GetString("file") - title, _ := cmd.Flags().GetString("title") - description, _ := cmd.Flags().GetString("description") - - hasFile := cmd.Flags().Changed("file") - hasTitle := cmd.Flags().Changed("title") - hasDescription := cmd.Flags().Changed("description") - - if hasFile && (hasTitle || hasDescription) { - return p.PrintError(fmt.Errorf("--file cannot be combined with --title or --description")) - } var batchReq forms.BatchUpdateFormRequest @@ -500,8 +506,6 @@ func runFormsUpdate(cmd *cobra.Command, args []string) error { } batchReq.Requests = requests - } else { - return p.PrintError(fmt.Errorf("provide --file, --title, or --description")) } resp, err := svc.Forms.BatchUpdate(formID, &batchReq).Do() diff --git a/cmd/sheets.go b/cmd/sheets.go index 18d67b1..fe84fa6 100644 --- a/cmd/sheets.go +++ b/cmd/sheets.go @@ -2815,6 +2815,21 @@ func mapConditionType(rule string) (string, error) { func runSheetsAddConditionalFormat(cmd *cobra.Command, args []string) error { p := printer.New(os.Stdout, GetFormat()) + + // Validate flags before creating API client + rule, _ := cmd.Flags().GetString("rule") + value, _ := cmd.Flags().GetString("value") + + conditionType, err := mapConditionType(rule) + if err != nil { + return p.PrintError(err) + } + + needsValue := map[string]bool{">": true, "<": true, "=": true, "!=": true, "contains": true, "not-contains": true, "formula": true} + if needsValue[rule] && value == "" { + return p.PrintError(fmt.Errorf("--value is required for rule type %q", rule)) + } + ctx := context.Background() factory, err := client.NewFactory(ctx) @@ -2829,24 +2844,11 @@ func runSheetsAddConditionalFormat(cmd *cobra.Command, args []string) error { spreadsheetID := args[0] rangeStr := args[1] - rule, _ := cmd.Flags().GetString("rule") - value, _ := cmd.Flags().GetString("value") bgColor, _ := cmd.Flags().GetString("bg-color") textColor, _ := cmd.Flags().GetString("color") bold, _ := cmd.Flags().GetBool("bold") italic, _ := cmd.Flags().GetBool("italic") - conditionType, err := mapConditionType(rule) - if err != nil { - return p.PrintError(err) - } - - // Validate --value is provided for rules that require it - needsValue := map[string]bool{">": true, "<": true, "=": true, "!=": true, "contains": true, "not-contains": true, "formula": true} - if needsValue[rule] && value == "" { - return p.PrintError(fmt.Errorf("--value is required for rule type %q", rule)) - } - _, gridRange, err := parseRange(svc, spreadsheetID, rangeStr) if err != nil { return p.PrintError(err) @@ -3024,6 +3026,13 @@ func runSheetsListConditionalFormats(cmd *cobra.Command, args []string) error { func runSheetsDeleteConditionalFormat(cmd *cobra.Command, args []string) error { p := printer.New(os.Stdout, GetFormat()) + + // Validate flags before creating API client + index, _ := cmd.Flags().GetInt64("index") + if index < 0 { + return p.PrintError(fmt.Errorf("--index must be >= 0, got %d", index)) + } + ctx := context.Background() factory, err := client.NewFactory(ctx) @@ -3038,11 +3047,6 @@ func runSheetsDeleteConditionalFormat(cmd *cobra.Command, args []string) error { spreadsheetID := args[0] sheetName, _ := cmd.Flags().GetString("sheet") - index, _ := cmd.Flags().GetInt64("index") - - if index < 0 { - return p.PrintError(fmt.Errorf("--index must be >= 0, got %d", index)) - } sheetID, err := getSheetID(svc, spreadsheetID, sheetName) if err != nil { diff --git a/cmd/slides.go b/cmd/slides.go index a6db179..a61226a 100644 --- a/cmd/slides.go +++ b/cmd/slides.go @@ -3108,6 +3108,15 @@ func runSlidesUngroup(cmd *cobra.Command, args []string) error { func runSlidesThumbnail(cmd *cobra.Command, args []string) error { p := printer.New(os.Stdout, GetFormat()) + + // Validate flags before creating API client + size, _ := cmd.Flags().GetString("size") + validSizes := map[string]bool{"SMALL": true, "MEDIUM": true, "LARGE": true} + sizeUpper := strings.ToUpper(size) + if !validSizes[sizeUpper] { + return p.PrintError(fmt.Errorf("invalid size '%s': must be SMALL, MEDIUM, or LARGE", size)) + } + ctx := context.Background() factory, err := client.NewFactory(ctx) @@ -3122,16 +3131,8 @@ func runSlidesThumbnail(cmd *cobra.Command, args []string) error { presentationID := args[0] slideFlag, _ := cmd.Flags().GetString("slide") - size, _ := cmd.Flags().GetString("size") downloadPath, _ := cmd.Flags().GetString("download") - // Validate size before any API calls - validSizes := map[string]bool{"SMALL": true, "MEDIUM": true, "LARGE": true} - sizeUpper := strings.ToUpper(size) - if !validSizes[sizeUpper] { - return p.PrintError(fmt.Errorf("invalid size '%s': must be SMALL, MEDIUM, or LARGE", size)) - } - // Resolve slide flag: could be a slide object ID or a 1-based number pageObjectID := slideFlag if num, err := strconv.Atoi(slideFlag); err == nil && num > 0 { diff --git a/cmd/validation_test.go b/cmd/validation_test.go new file mode 100644 index 0000000..888aae4 --- /dev/null +++ b/cmd/validation_test.go @@ -0,0 +1,173 @@ +package cmd + +import ( + "bytes" + "encoding/json" + "os" + "strings" + "testing" +) + +// captureOutput captures stdout during a function call and returns the output. +func captureOutput(t *testing.T, fn func()) string { + t.Helper() + old := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create pipe: %v", err) + } + os.Stdout = w + + fn() + + w.Close() + os.Stdout = old + + var buf bytes.Buffer + buf.ReadFrom(r) + return buf.String() +} + +// extractError parses JSON output and returns the "error" field if present. +func extractError(output string) string { + var result map[string]interface{} + if err := json.Unmarshal([]byte(strings.TrimSpace(output)), &result); err != nil { + return "" + } + if errMsg, ok := result["error"].(string); ok { + return errMsg + } + return "" +} + +// --- Forms validation tests --- + +func TestFormsUpdate_Validation(t *testing.T) { + // Test no-flags case first (before any flags are mutated on the shared command) + t.Run("no_flags", func(t *testing.T) { + cmd := findSubcommand(formsCmd, "update") + if cmd == nil { + t.Fatal("forms update command not found") + } + + output := captureOutput(t, func() { + cmd.RunE(cmd, []string{"form-id"}) + }) + + errMsg := extractError(output) + if !strings.Contains(errMsg, "provide --file, --title, or --description") { + t.Errorf("expected missing-flags error, got output: %s", output) + } + }) + + t.Run("file_and_title_conflict", func(t *testing.T) { + cmd := findSubcommand(formsCmd, "update") + if cmd == nil { + t.Fatal("forms update command not found") + } + + cmd.Flags().Set("file", "test.json") + cmd.Flags().Set("title", "New Title") + + output := captureOutput(t, func() { + cmd.RunE(cmd, []string{"form-id"}) + }) + + errMsg := extractError(output) + if !strings.Contains(errMsg, "cannot be combined") { + t.Errorf("expected conflict error, got output: %s", output) + } + }) +} + +// --- Sheets validation tests --- + +func TestSheetsAddConditionalFormat_ValueRequired(t *testing.T) { + rulesNeedingValue := []string{">", "<", "=", "!=", "contains", "not-contains", "formula"} + for _, rule := range rulesNeedingValue { + t.Run(rule, func(t *testing.T) { + cmd := findSubcommand(sheetsCmd, "add-conditional-format") + if cmd == nil { + t.Fatal("sheets add-conditional-format command not found") + } + + cmd.Flags().Set("rule", rule) + defer cmd.Flags().Set("rule", "") + + output := captureOutput(t, func() { + cmd.RunE(cmd, []string{"spreadsheet-id", "Sheet1!A1:A10"}) + }) + + errMsg := extractError(output) + if !strings.Contains(errMsg, "--value is required") { + t.Errorf("expected --value required error for rule %q, got output: %s", rule, output) + } + }) + } +} + +func TestSheetsDeleteConditionalFormat_NegativeIndex(t *testing.T) { + cmd := findSubcommand(sheetsCmd, "delete-conditional-format") + if cmd == nil { + t.Fatal("sheets delete-conditional-format command not found") + } + + cmd.Flags().Set("sheet", "Sheet1") + cmd.Flags().Set("index", "-1") + defer func() { + cmd.Flags().Set("sheet", "") + cmd.Flags().Set("index", "0") + }() + + output := captureOutput(t, func() { + cmd.RunE(cmd, []string{"spreadsheet-id"}) + }) + + errMsg := extractError(output) + if !strings.Contains(errMsg, "--index must be >= 0") { + t.Errorf("expected negative index error, got output: %s", output) + } +} + +// --- Slides validation tests --- + +func TestSlidesThumbnail_InvalidSize(t *testing.T) { + cmd := findSubcommand(slidesCmd, "thumbnail") + if cmd == nil { + t.Fatal("slides thumbnail command not found") + } + + cmd.Flags().Set("slide", "1") + cmd.Flags().Set("size", "XLARGE") + defer func() { + cmd.Flags().Set("slide", "") + cmd.Flags().Set("size", "MEDIUM") + }() + + output := captureOutput(t, func() { + cmd.RunE(cmd, []string{"presentation-id"}) + }) + + errMsg := extractError(output) + if !strings.Contains(errMsg, "invalid size") { + t.Errorf("expected invalid size error, got output: %s", output) + } +} + +// --- Contacts validation tests --- + +func TestContactsUpdate_NoFlags(t *testing.T) { + cmd := findSubcommand(contactsCmd, "update") + if cmd == nil { + t.Fatal("contacts update command not found") + } + + output := captureOutput(t, func() { + cmd.RunE(cmd, []string{"people/c123"}) + }) + + errMsg := extractError(output) + if !strings.Contains(errMsg, "at least one field") { + t.Errorf("expected missing-flags error, got output: %s", output) + } +}