From 3e966234cfe9faf27ab6fb4238cdbe4194f3c406 Mon Sep 17 00:00:00 2001 From: shunsuke maeda Date: Sat, 1 Jun 2019 22:50:53 +0900 Subject: [PATCH 1/2] Fix wrong duration in GitHub commit status with parallel job --- application/context.go | 22 +++++++++++++++++++ application/context_test.go | 14 ++++++++---- application/duci/duci.go | 16 ++++---------- application/duci/duci_test.go | 4 ++-- application/duci/export_test.go | 8 ------- .../controller/webhook/handler_test.go | 16 +++++++++++--- 6 files changed, 51 insertions(+), 29 deletions(-) diff --git a/application/context.go b/application/context.go index ea2b7165..b1373691 100644 --- a/application/context.go +++ b/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. diff --git a/application/context_test.go b/application/context_test.go index 2ed3fd9d..9e0b3161 100644 --- a/application/context_test.go +++ b/application/context_test.go @@ -11,6 +11,9 @@ import ( func TestContextWithJob(t *testing.T) { // given + opts := cmp.AllowUnexported(application.BuildJob{}) + + // and want := &application.BuildJob{ ID: job.ID(uuid.New()), } @@ -22,14 +25,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 +51,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)) } }) diff --git a/application/duci/duci.go b/application/duci/duci.go index d02b9dd5..01974bec 100644 --- a/application/duci/duci.go +++ b/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())) -} diff --git a/application/duci/duci_test.go b/application/duci/duci_test.go index c6a3dec1..43b2e8ee 100644 --- a/application/duci/duci_test.go +++ b/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) diff --git a/application/duci/export_test.go b/application/duci/export_test.go index aca22d8c..af538c4c 100644 --- a/application/duci/export_test.go +++ b/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) diff --git a/presentation/controller/webhook/handler_test.go b/presentation/controller/webhook/handler_test.go index f4a9d65d..0e5a608b 100644 --- a/presentation/controller/webhook/handler_test.go +++ b/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)) } From 5e045e0519aacb9816015461f6c1e4fd3a25987f Mon Sep 17 00:00:00 2001 From: shunsuke maeda Date: Sun, 2 Jun 2019 13:27:45 +0900 Subject: [PATCH 2/2] Add test cases --- application/context_test.go | 79 +++++++++++++++++++++++++++++++++++++ application/export_test.go | 9 +++++ 2 files changed, 88 insertions(+) diff --git a/application/context_test.go b/application/context_test.go index 9e0b3161..bdb65f44 100644 --- a/application/context_test.go +++ b/application/context_test.go @@ -7,6 +7,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/uuid" "testing" + "time" ) func TestContextWithJob(t *testing.T) { @@ -74,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) + } + }) + } +} diff --git a/application/export_test.go b/application/export_test.go index eb5ca9f6..74ef4503 100644 --- a/application/export_test.go +++ b/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 +}