From 3e966234cfe9faf27ab6fb4238cdbe4194f3c406 Mon Sep 17 00:00:00 2001 From: shunsuke maeda Date: Sat, 1 Jun 2019 22:50:53 +0900 Subject: [PATCH] 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)) }