Merge pull request #249 from duck8823/fix/wrong_duration

Fix wrong duration in GitHub commit status with parallel job

application/context.go

@@ -6,6 +6,7 @@ import (
 	"github.com/duck8823/duci/domain/model/job"
 	"github.com/duck8823/duci/domain/model/job/target/github"
 	"net/url"
+	"time"
 )
 
 var ctxKey = "duci_job"
@@ -16,6 +17,27 @@ type BuildJob struct {
 	TargetSource *github.TargetSource
 	TaskName     string
 	TargetURL    *url.URL
+	beginTime    time.Time
+	endTime      time.Time
+}
+
+// BeginAt set a time that begin job
+func (j *BuildJob) BeginAt(time time.Time) {
+	j.beginTime = time
+}
+
+// EndAt set a time that end job
+func (j *BuildJob) EndAt(time time.Time) {
+	j.endTime = time
+}
+
+// Duration returns job duration
+func (j *BuildJob) Duration() string {
+	dur := j.endTime.Sub(j.beginTime)
+	if int(dur.Minutes()) > 0 {
+		return fmt.Sprintf("%dmin", int(dur.Minutes()))
+	}
+	return fmt.Sprintf("%dsec", int(dur.Seconds()))
 }
 
 // ContextWithJob set parent context BuildJob and returns it.

application/context_test.go

@@ -7,10 +7,14 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/uuid"
 	"testing"
+	"time"
 )
 
 func TestContextWithJob(t *testing.T) {
 	// given
+	opts := cmp.AllowUnexported(application.BuildJob{})
+
+	// and
 	want := &application.BuildJob{
 		ID: job.ID(uuid.New()),
 	}
@@ -22,14 +26,17 @@ func TestContextWithJob(t *testing.T) {
 	got := ctx.Value(application.GetCtxKey())
 
 	// then
-	if !cmp.Equal(got, want) {
-		t.Errorf("must be equal, but %+v", cmp.Diff(got, want))
+	if !cmp.Equal(got, want, opts) {
+		t.Errorf("must be equal, but %+v", cmp.Diff(got, want, opts))
 	}
 }
 
 func TestBuildJobFromContext(t *testing.T) {
 	t.Run("with value", func(t *testing.T) {
 		// given
+		opts := cmp.AllowUnexported(application.BuildJob{})
+
+		// and
 		want := &application.BuildJob{
 			ID: job.ID(uuid.New()),
 		}
@@ -45,8 +52,8 @@ func TestBuildJobFromContext(t *testing.T) {
 		}
 
 		// and
-		if !cmp.Equal(got, want) {
-			t.Errorf("must be equal, but %+v", cmp.Diff(got, want))
+		if !cmp.Equal(got, want, opts) {
+			t.Errorf("must be equal, but %+v", cmp.Diff(got, want, opts))
 		}
 	})
 
@@ -68,3 +75,81 @@ func TestBuildJobFromContext(t *testing.T) {
 	})
 
 }
+
+func TestBuildJob_BeginAt(t *testing.T) {
+	// given
+	want := time.Unix(10, 10)
+
+	// and
+	sut := &application.BuildJob{}
+
+	// when
+	sut.BeginAt(want)
+	got := sut.GetBeginTime()
+
+	// then
+	if !cmp.Equal(got, want) {
+		t.Errorf("must be equal, but %+v", cmp.Diff(got, want))
+	}
+}
+
+func TestBuildJob_EndAt(t *testing.T) {
+	// given
+	want := time.Unix(10, 10)
+
+	// and
+	sut := &application.BuildJob{}
+
+	// when
+	sut.EndAt(want)
+	got := sut.GetEndTime()
+
+	// then
+	if !cmp.Equal(got, want) {
+		t.Errorf("must be equal, but %+v", cmp.Diff(got, want))
+	}
+}
+
+func TestBuildJob_Duration(t *testing.T) {
+	tests := []struct {
+		name      string
+		beginTime time.Time
+		endTime   time.Time
+		want      string
+	}{
+		{
+			"when duration is less than 60 seconds",
+			time.Unix(10, 10),
+			time.Unix(70, 0),
+			"59sec",
+		},
+		{
+			"when duration is greater than 60 seconds",
+			time.Unix(10, 10),
+			time.Unix(70, 11),
+			"1min",
+		},
+		{
+			"when duration is equal to 60 seconds",
+			time.Unix(10, 10),
+			time.Unix(70, 10),
+			"1min",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			// given
+			sut := &application.BuildJob{}
+			sut.BeginAt(test.beginTime)
+			sut.EndAt(test.endTime)
+
+			// when
+			got := sut.Duration()
+
+			// then
+			if got != test.want {
+				t.Errorf("want %s, but got %s", test.want, got)
+			}
+		})
+	}
+}

application/duci/duci.go

@@ -20,7 +20,6 @@ type duci struct {
 	executor.Executor
 	jobService jobService.Service
 	github     github.GitHub
-	begin      time.Time
 }
 
 // New returns duci instance
@@ -78,12 +77,12 @@ func (d *duci) Init(ctx context.Context) {
 
 // Start represents a function of start job
 func (d *duci) Start(ctx context.Context) {
-	d.begin = now()
 	buildJob, err := application.BuildJobFromContext(ctx)
 	if err != nil {
 		logrus.Errorf("%+v", err)
 		return
 	}
+	buildJob.BeginAt(now())
 	if err := d.github.CreateCommitStatus(ctx, github.CommitStatus{
 		TargetSource: buildJob.TargetSource,
 		State:        github.PENDING,
@@ -117,6 +116,7 @@ func (d *duci) End(ctx context.Context, e error) {
 		logrus.Errorf("%+v", err)
 		return
 	}
+	buildJob.EndAt(now())
 	if err := d.jobService.Finish(buildJob.ID); err != nil {
 		if err := d.jobService.Append(buildJob.ID, job.LogLine{Timestamp: now(), Message: err.Error()}); err != nil {
 			logrus.Errorf("%+v", err)
@@ -129,7 +129,7 @@ func (d *duci) End(ctx context.Context, e error) {
 		if err := d.github.CreateCommitStatus(ctx, github.CommitStatus{
 			TargetSource: buildJob.TargetSource,
 			State:        github.SUCCESS,
-			Description:  github.Description(fmt.Sprintf("success in %s", d.duration())),
+			Description:  github.Description(fmt.Sprintf("success in %s", buildJob.Duration())),
 			Context:      buildJob.TaskName,
 			TargetURL:    buildJob.TargetURL,
 		}); err != nil {
@@ -139,7 +139,7 @@ func (d *duci) End(ctx context.Context, e error) {
 		if err := d.github.CreateCommitStatus(ctx, github.CommitStatus{
 			TargetSource: buildJob.TargetSource,
 			State:        github.FAILURE,
-			Description:  github.Description(fmt.Sprintf("failure in %s", d.duration())),
+			Description:  github.Description(fmt.Sprintf("failure in %s", buildJob.Duration())),
 			Context:      buildJob.TaskName,
 			TargetURL:    buildJob.TargetURL,
 		}); err != nil {
@@ -157,11 +157,3 @@ func (d *duci) End(ctx context.Context, e error) {
 		}
 	}
 }
-
-func (d *duci) duration() string {
-	dur := now().Sub(d.begin)
-	if int(dur.Minutes()) > 0 {
-		return fmt.Sprintf("%dmin", int(dur.Minutes()))
-	}
-	return fmt.Sprintf("%dsec", int(dur.Seconds()))
-}

application/duci/duci_test.go

@@ -339,6 +339,7 @@ func TestDuci_End(t *testing.T) {
 			TaskName:     "task/name",
 			TargetURL:    duci.URLMust(url.Parse("http://example.com")),
 		}
+		buildJob.BeginAt(time.Unix(0, 0))
 		ctx := application.ContextWithJob(context.Background(), buildJob)
 		var err error = nil
 
@@ -374,7 +375,6 @@ func TestDuci_End(t *testing.T) {
 		sut := &duci.Duci{}
 		defer sut.SetJobService(service)()
 		defer sut.SetGitHub(hub)()
-		defer sut.SetBegin(time.Unix(0, 0))()
 
 		// when
 		sut.End(ctx, err)
@@ -391,6 +391,7 @@ func TestDuci_End(t *testing.T) {
 			TaskName:     "task/name",
 			TargetURL:    duci.URLMust(url.Parse("http://example.com")),
 		}
+		buildJob.BeginAt(time.Unix(0, 0))
 		ctx := application.ContextWithJob(context.Background(), buildJob)
 		err := runner.ErrFailure
 
@@ -426,7 +427,6 @@ func TestDuci_End(t *testing.T) {
 		sut := &duci.Duci{}
 		defer sut.SetJobService(service)()
 		defer sut.SetGitHub(hub)()
-		defer sut.SetBegin(time.Unix(0, 0))()
 
 		// when
 		sut.End(ctx, err)

application/duci/export_test.go

@@ -27,14 +27,6 @@ func (d *Duci) SetGitHub(hub github.GitHub) (reset func()) {
 	}
 }
 
-func (d *Duci) SetBegin(t time.Time) (reset func()) {
-	tmp := d.begin
-	d.begin = t
-	return func() {
-		d.begin = tmp
-	}
-}
-
 func URLMust(url *url.URL, err error) *url.URL {
 	if err != nil {
 		panic(err)

application/export_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/tcnksm/go-latest"
 	"os"
 	"testing"
+	"time"
 )
 
 type MaskString = maskString
@@ -65,3 +66,11 @@ func GenerateSSHKey(t *testing.T, path string) {
 		t.Fatalf("error occur: %+v", err)
 	}
 }
+
+func (j *BuildJob) GetBeginTime() time.Time {
+	return j.beginTime
+}
+
+func (j *BuildJob) GetEndTime() time.Time {
+	return j.endTime
+}

presentation/controller/webhook/handler_test.go

@@ -263,7 +263,11 @@ func TestHandler_PushEvent(t *testing.T) {
 					TargetURL: webhook.URLMust(url.Parse("http://example.com/logs/72d3162e-cc78-11e3-81ab-4c9367dc0958")),
 				}
 
-				opt := webhook.CmpOptsAllowFields(go_github.PushEventRepository{}, "ID", "FullName", "SSHURL", "CloneURL")
+				opt := cmp.Options{
+					webhook.CmpOptsAllowFields(go_github.PushEventRepository{}, "ID", "FullName", "SSHURL", "CloneURL"),
+					cmp.AllowUnexported(application.BuildJob{}),
+				}
+
 				if !cmp.Equal(got, want, opt) {
 					t.Errorf("must be equal but: %+v", cmp.Diff(got, want, opt))
 				}
@@ -430,7 +434,10 @@ func TestHandler_IssueCommentEvent_Normal(t *testing.T) {
 					TargetURL: webhook.URLMust(url.Parse("http://example.com/logs/72d3162e-cc78-11e3-81ab-4c9367dc0958")),
 				}
 
-				opt := webhook.CmpOptsAllowFields(go_github.Repository{}, "ID", "FullName", "SSHURL", "CloneURL")
+				opt := cmp.Options{
+					webhook.CmpOptsAllowFields(go_github.Repository{}, "ID", "FullName", "SSHURL", "CloneURL"),
+					cmp.AllowUnexported(application.BuildJob{}),
+				}
 				if !cmp.Equal(got, want, opt) {
 					t.Errorf("must be equal but: %+v", cmp.Diff(got, want, opt))
 				}
@@ -799,7 +806,10 @@ func TestHandler_PullRequestEvent(t *testing.T) {
 						TargetURL: webhook.URLMust(url.Parse("http://example.com/logs/72d3162e-cc78-11e3-81ab-4c9367dc0958")),
 					}
 
-					opt := webhook.CmpOptsAllowFields(go_github.Repository{}, "ID", "FullName", "SSHURL", "CloneURL")
+					opt := cmp.Options{
+						webhook.CmpOptsAllowFields(go_github.Repository{}, "ID", "FullName", "SSHURL", "CloneURL"),
+						cmp.AllowUnexported(application.BuildJob{}),
+					}
 					if !cmp.Equal(got, want, opt) {
 						t.Errorf("must be equal but: %+v", cmp.Diff(got, want, opt))
 					}