From 1e77e0a12494ed29be26fe72f131bc698e247043 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Dachary?= Date: Sat, 29 Jan 2022 13:42:34 +0100 Subject: [PATCH 1/3] GitLab reviews may not have the updated_at field set Fallback to created_at if that the case and to time.Now() if it is also missing. Fixes: 18434 --- services/migrations/gitlab.go | 11 ++- services/migrations/gitlab_test.go | 140 +++++++++++++++++++++++++++++ services/migrations/main_test.go | 25 ++++-- 3 files changed, 166 insertions(+), 10 deletions(-) diff --git a/services/migrations/gitlab.go b/services/migrations/gitlab.go index a9856739c242b..f587dad7a6576 100644 --- a/services/migrations/gitlab.go +++ b/services/migrations/gitlab.go @@ -640,13 +640,22 @@ func (g *GitlabDownloader) GetReviews(context base.IssueContext) ([]*base.Review return nil, err } + var createdAt time.Time + if approvals.UpdatedAt != nil { + createdAt = *approvals.UpdatedAt + } else if approvals.CreatedAt != nil { + createdAt = *approvals.CreatedAt + } else { + createdAt = time.Now() + } + reviews := make([]*base.Review, 0, len(approvals.ApprovedBy)) for _, user := range approvals.ApprovedBy { reviews = append(reviews, &base.Review{ IssueIndex: context.LocalID(), ReviewerID: int64(user.User.ID), ReviewerName: user.User.Username, - CreatedAt: *approvals.UpdatedAt, + CreatedAt: createdAt, // All we get are approvals State: base.ReviewStateApproved, }) diff --git a/services/migrations/gitlab_test.go b/services/migrations/gitlab_test.go index 6b77aa62efbfb..ad61577653c17 100644 --- a/services/migrations/gitlab_test.go +++ b/services/migrations/gitlab_test.go @@ -8,13 +8,16 @@ import ( "context" "fmt" "net/http" + "net/http/httptest" "os" + "strconv" "testing" "time" base "code.gitea.io/gitea/modules/migration" "github.com/stretchr/testify/assert" + "github.com/xanzy/go-gitlab" ) func TestGitlabDownloadRepo(t *testing.T) { @@ -310,12 +313,14 @@ func TestGitlabDownloadRepo(t *testing.T) { assert.NoError(t, err) assertReviewsEqual(t, []*base.Review{ { + IssueIndex: 1, ReviewerID: 4102996, ReviewerName: "zeripath", CreatedAt: time.Date(2019, 11, 28, 16, 2, 8, 377000000, time.UTC), State: "APPROVED", }, { + IssueIndex: 1, ReviewerID: 527793, ReviewerName: "axifive", CreatedAt: time.Date(2019, 11, 28, 16, 2, 8, 377000000, time.UTC), @@ -327,6 +332,7 @@ func TestGitlabDownloadRepo(t *testing.T) { assert.NoError(t, err) assertReviewsEqual(t, []*base.Review{ { + IssueIndex: 2, ReviewerID: 4575606, ReviewerName: "real6543", CreatedAt: time.Date(2020, 4, 19, 19, 24, 21, 108000000, time.UTC), @@ -334,3 +340,137 @@ func TestGitlabDownloadRepo(t *testing.T) { }, }, rvs) } + +func gitlabClientMockSetup(t *testing.T) (*http.ServeMux, *httptest.Server, *gitlab.Client) { + // mux is the HTTP request multiplexer used with the test server. + mux := http.NewServeMux() + + // server is a test HTTP server used to provide mock API responses. + server := httptest.NewServer(mux) + + // client is the Gitlab client being tested. + client, err := gitlab.NewClient("", gitlab.WithBaseURL(server.URL)) + if err != nil { + server.Close() + t.Fatalf("Failed to create client: %v", err) + } + + return mux, server, client +} + +func gitlabClientMockTeardown(server *httptest.Server) { + server.Close() +} + +type reviewTestCase struct { + repoID, prID, reviewerID int + reviewerName string + createdAt, updatedAt *time.Time + expectedCreatedAt time.Time +} + +func convertTestCase(t reviewTestCase) (func(w http.ResponseWriter, r *http.Request), base.Review) { + var updatedAtField string + if t.updatedAt == nil { + updatedAtField = "" + } else { + updatedAtField = `"updated_at": "` + t.updatedAt.Format(time.RFC3339) + `",` + } + + var createdAtField string + if t.createdAt == nil { + createdAtField = "" + } else { + createdAtField = `"created_at": "` + t.createdAt.Format(time.RFC3339) + `",` + } + + handler := func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, ` +{ + "id": 5, + "iid": `+strconv.Itoa(t.prID)+`, + "project_id": `+strconv.Itoa(t.repoID)+`, + "title": "Approvals API", + "description": "Test", + "state": "opened", + `+createdAtField+` + `+updatedAtField+` + "merge_status": "cannot_be_merged", + "approvals_required": 2, + "approvals_left": 1, + "approved_by": [ + { + "user": { + "name": "Administrator", + "username": "`+t.reviewerName+`", + "id": `+strconv.Itoa(t.reviewerID)+`, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon", + "web_url": "http://localhost:3000/root" + } + } + ] +}`) + } + review := base.Review{ + IssueIndex: int64(t.prID), + ReviewerID: int64(t.reviewerID), + ReviewerName: t.reviewerName, + CreatedAt: t.expectedCreatedAt, + State: "APPROVED", + } + + return handler, review +} + +func TestGitlabGetReviews(t *testing.T) { + mux, server, client := gitlabClientMockSetup(t) + defer gitlabClientMockTeardown(server) + + repoID := 1324 + + downloader := &GitlabDownloader{ + ctx: context.Background(), + client: client, + repoID: repoID, + } + + createdAt := time.Date(2020, 4, 19, 19, 24, 21, 0, time.UTC) + + for _, testCase := range []reviewTestCase{ + { + repoID: repoID, + prID: 1, + reviewerID: 801, + reviewerName: "someone1", + createdAt: nil, + updatedAt: &createdAt, + expectedCreatedAt: createdAt, + }, + { + repoID: repoID, + prID: 2, + reviewerID: 802, + reviewerName: "someone2", + createdAt: &createdAt, + updatedAt: nil, + expectedCreatedAt: createdAt, + }, + { + repoID: repoID, + prID: 3, + reviewerID: 803, + reviewerName: "someone3", + createdAt: nil, + updatedAt: nil, + expectedCreatedAt: time.Now(), + }, + } { + mock, review := convertTestCase(testCase) + mux.HandleFunc(fmt.Sprintf("/api/v4/projects/%d/merge_requests/%d/approvals", testCase.repoID, testCase.prID), mock) + + rvs, err := downloader.GetReviews(base.BasicIssueContext(testCase.prID)) + assert.NoError(t, err) + assertReviewsEqual(t, []*base.Review{&review}, rvs) + } +} diff --git a/services/migrations/main_test.go b/services/migrations/main_test.go index ddf73df98e47c..d73a1098a7543 100644 --- a/services/migrations/main_test.go +++ b/services/migrations/main_test.go @@ -6,6 +6,8 @@ package migrations import ( + "fmt" + "math" "path/filepath" "testing" "time" @@ -28,6 +30,11 @@ func assertTimeEqual(t *testing.T, expected, actual time.Time) { assert.Equal(t, expected.UTC(), actual.UTC()) } +func assertTimeEqualEpsilon(t *testing.T, expected, actual time.Time) { + d := expected.UTC().Sub(actual.UTC()).Seconds() + assert.True(t, math.Abs(d) < 1, fmt.Sprintf("%v - %v >= 1", expected, actual)) +} + func assertTimePtrEqual(t *testing.T, expected, actual *time.Time) { if expected == nil { assert.Nil(t, actual) @@ -223,15 +230,15 @@ func assertRepositoryEqual(t *testing.T, expected, actual *base.Repository) { } func assertReviewEqual(t *testing.T, expected, actual *base.Review) { - assert.Equal(t, expected.ID, actual.ID) - assert.Equal(t, expected.IssueIndex, actual.IssueIndex) - assert.Equal(t, expected.ReviewerID, actual.ReviewerID) - assert.Equal(t, expected.ReviewerName, actual.ReviewerName) - assert.Equal(t, expected.Official, actual.Official) - assert.Equal(t, expected.CommitID, actual.CommitID) - assert.Equal(t, expected.Content, actual.Content) - assertTimeEqual(t, expected.CreatedAt, actual.CreatedAt) - assert.Equal(t, expected.State, actual.State) + assert.Equal(t, expected.ID, actual.ID, "ID") + assert.Equal(t, expected.IssueIndex, actual.IssueIndex, "IsssueIndex") + assert.Equal(t, expected.ReviewerID, actual.ReviewerID, "ReviewerID") + assert.Equal(t, expected.ReviewerName, actual.ReviewerName, "ReviewerName") + assert.Equal(t, expected.Official, actual.Official, "Official") + assert.Equal(t, expected.CommitID, actual.CommitID, "CommitID") + assert.Equal(t, expected.Content, actual.Content, "Content") + assertTimeEqualEpsilon(t, expected.CreatedAt, actual.CreatedAt) + assert.Equal(t, expected.State, actual.State, "State") assertReviewCommentsEqual(t, expected.Comments, actual.Comments) } From 0a4d285ca3d81f6ef22ec36cad00e5ca0702ad8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Dachary?= Date: Sat, 29 Jan 2022 15:51:18 +0100 Subject: [PATCH 2/3] use assert.WithinDuration --- services/migrations/main_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/services/migrations/main_test.go b/services/migrations/main_test.go index d73a1098a7543..b040df83d14bd 100644 --- a/services/migrations/main_test.go +++ b/services/migrations/main_test.go @@ -6,8 +6,6 @@ package migrations import ( - "fmt" - "math" "path/filepath" "testing" "time" @@ -30,11 +28,6 @@ func assertTimeEqual(t *testing.T, expected, actual time.Time) { assert.Equal(t, expected.UTC(), actual.UTC()) } -func assertTimeEqualEpsilon(t *testing.T, expected, actual time.Time) { - d := expected.UTC().Sub(actual.UTC()).Seconds() - assert.True(t, math.Abs(d) < 1, fmt.Sprintf("%v - %v >= 1", expected, actual)) -} - func assertTimePtrEqual(t *testing.T, expected, actual *time.Time) { if expected == nil { assert.Nil(t, actual) @@ -237,7 +230,7 @@ func assertReviewEqual(t *testing.T, expected, actual *base.Review) { assert.Equal(t, expected.Official, actual.Official, "Official") assert.Equal(t, expected.CommitID, actual.CommitID, "CommitID") assert.Equal(t, expected.Content, actual.Content, "Content") - assertTimeEqualEpsilon(t, expected.CreatedAt, actual.CreatedAt) + assert.WithinDuration(t, expected.CreatedAt, actual.CreatedAt, 10*time.Second) assert.Equal(t, expected.State, actual.State, "State") assertReviewCommentsEqual(t, expected.Comments, actual.Comments) } From 77a06f0d2237b8c1218b83d3dac18c4d4044ee72 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 29 Jan 2022 17:38:53 +0100 Subject: [PATCH 3/3] Update services/migrations/gitlab.go --- services/migrations/gitlab.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/migrations/gitlab.go b/services/migrations/gitlab.go index f587dad7a6576..97ebc4dd8b488 100644 --- a/services/migrations/gitlab.go +++ b/services/migrations/gitlab.go @@ -641,10 +641,10 @@ func (g *GitlabDownloader) GetReviews(context base.IssueContext) ([]*base.Review } var createdAt time.Time - if approvals.UpdatedAt != nil { - createdAt = *approvals.UpdatedAt - } else if approvals.CreatedAt != nil { + if approvals.CreatedAt != nil { createdAt = *approvals.CreatedAt + } else if approvals.UpdatedAt != nil { + createdAt = *approvals.UpdatedAt } else { createdAt = time.Now() }