Refine ignored handling (#45)

* Add ignore detalis in the summary

* Update string handling to remove space and empty input

* Adjust test case and validation as trim logic moved to option

* Move trimming logic out from validator

Author: rytswd

internal/validators/status/option.go

@@ -34,8 +34,19 @@ func WithGitHubRef(ref string) Option {
 func WithIgnoredJobs(names string) Option {
 	return func(s *statusValidator) {
 		// TODO: Add more input validation, such as "," should not be a valid input.
-		if len(names) != 0 {
-			s.ignoredJobs = strings.Split(names, ",")
+		if len(names) == 0 {
+			return // TODO: Return some clearer error
 		}
+
+		jobs := []string{}
+		ss := strings.Split(names, ",")
+		for _, s := range ss {
+			jobName := strings.TrimSpace(s)
+			if len(jobName) == 0 {
+				continue // TODO: Provide more clue to users
+			}
+			jobs = append(jobs, jobName)
+		}
+		s.ignoredJobs = jobs
 	}
 }

internal/validators/status/status.go

@@ -6,11 +6,12 @@ type status struct {
 	totalJobs    []string
 	completeJobs []string
 	errJobs      []string
+	ignoredJobs  []string
 	succeeded    bool
 }
 
 func (s *status) Detail() string {
-	return fmt.Sprintf(
+	result := fmt.Sprintf(
 		`%d out of %d
 
   Total job count:     %d
@@ -25,6 +26,16 @@ func (s *status) Detail() string {
 		len(s.completeJobs), s.completeJobs,
 		len(s.errJobs), s.errJobs,
 	)
+
+	if len(s.ignoredJobs) > 0 {
+		result = fmt.Sprintf(
+			`%s
+
+  --
+  Ignored jobs: %+q`, result, s.ignoredJobs)
+	}
+
+	return result
 }
 
 func (s *status) IsSuccess() bool {

internal/validators/status/status_test.go

@@ -32,6 +32,38 @@ func Test_status_Detail(t *testing.T) {
   Failed job count:    1
     jobs: ["job-3"]
 `,
+		},
+		"return detail with ignored jobs input": {
+			s: &status{
+				totalJobs: []string{
+					"job-1",
+					"job-2",
+					"job-3",
+					"job-4",
+				},
+				completeJobs: []string{
+					"job-2",
+					"job-4",
+				},
+				errJobs: []string{
+					"job-3",
+				},
+				ignoredJobs: []string{
+					"job-4",
+				},
+			},
+			want: `2 out of 4
+
+  Total job count:     4
+    jobs: ["job-1" "job-2" "job-3" "job-4"]
+  Completed job count: 2
+    jobs: ["job-2" "job-4"]
+  Failed job count:    1
+    jobs: ["job-3"]
+
+
+  --
+  Ignored jobs: ["job-4"]`,
 		},
 		"return detail when totalJobs and completeJobs is empty": {
 			s: &status{
@@ -54,7 +86,7 @@ func Test_status_Detail(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			got := tt.s.Detail()
 			if got != tt.want {
-				t.Errorf("status.Detail() str = %s, want: %s", got, tt.want)
+				t.Errorf("status.Detail() didn't match\n  got:\n%s\n\n  want:\n%s", got, tt.want)
 			}
 		})
 	}

internal/validators/status/validator.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 
 	"github.com/upsidr/merge-gatekeeper/internal/github"
 	"github.com/upsidr/merge-gatekeeper/internal/multierror"
@@ -79,11 +78,6 @@ func (sv *statusValidator) validateFields() error {
 	if len(sv.selfJobName) == 0 {
 		errs = append(errs, errors.New("self job name is empty"))
 	}
-	for _, job := range sv.ignoredJobs {
-		if len(job) == 0 {
-			errs = append(errs, errors.New("ignored job name is empty"))
-		}
-	}
 	if sv.client == nil {
 		errs = append(errs, errors.New("github client is empty"))
 	}
@@ -112,7 +106,7 @@ func (sv *statusValidator) Validate(ctx context.Context) (validators.Status, err
 	for _, ghaStatus := range ghaStatuses {
 		var toIgnore bool
 		for _, ignored := range sv.ignoredJobs {
-			if ghaStatus.Job == strings.TrimSpace(ignored) {
+			if ghaStatus.Job == ignored {
 				toIgnore = true
 				break
 			}

internal/validators/status/validator_test.go

@@ -58,32 +58,39 @@ func TestCreateValidator(t *testing.T) {
 			},
 			wantErr: false,
 		},
-		"returns error when option is empty": {
-			c:       &mock.Client{},
-			want:    nil,
-			wantErr: true,
-		},
-		"returns error when client is nil": {
-			c: nil,
+		"returns Validator when invalid string is provided for ignored jobs": {
+			c: &mock.Client{},
 			opts: []Option{
 				WithGitHubOwnerAndRepo("test", "test-repo"),
 				WithGitHubRef("sha"),
 				WithGitHubRef("sha-01"),
 				WithSelfJob("job"),
 				WithSelfJob("job-01"),
+				WithIgnoredJobs(","), // Malformed but handled
+			},
+			want: &statusValidator{
+				client:      &mock.Client{},
+				owner:       "test",
+				repo:        "test-repo",
+				ref:         "sha-01",
+				selfJobName: "job-01",
+				ignoredJobs: []string{}, // Not nil
 			},
+			wantErr: false,
+		},
+		"returns error when option is empty": {
+			c:       &mock.Client{},
 			want:    nil,
 			wantErr: true,
 		},
-		"returns error when ignored jobs is an empty string": {
-			c: &mock.Client{},
+		"returns error when client is nil": {
+			c: nil,
 			opts: []Option{
 				WithGitHubOwnerAndRepo("test", "test-repo"),
 				WithGitHubRef("sha"),
 				WithGitHubRef("sha-01"),
 				WithSelfJob("job"),
 				WithSelfJob("job-01"),
-				WithIgnoredJobs(","), // Malformed, and causes the error
 			},
 			want:    nil,
 			wantErr: true,
@@ -376,7 +383,7 @@ func Test_statusValidator_Validate(t *testing.T) {
 		},
 		"returns succeeded status and nil when only an ignored job is failing": {
 			selfJobName: "self-job",
-			ignoredJobs: []string{"job-02  ", "job-03"}, // Some extra space will be trimmed by strings.TrimSpace
+			ignoredJobs: []string{"job-02", "job-03"}, // String input here should be already TrimSpace'd
 			client: &mock.Client{
 				GetCombinedStatusFunc: func(ctx context.Context, owner, repo, ref string, opts *github.ListOptions) (*github.CombinedStatus, *github.Response, error) {
 					return &github.CombinedStatus{