Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: gitlab client failing test #3653

Merged
merged 4 commits into from
Aug 9, 2023
Merged

fix: gitlab client failing test #3653

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b34055
fix: gitlab client failing test
ghaiszaher Aug 6, 2023
63dcd43
change HeadCommit to `67cb91d3f6198189f433c045154a885784ba6977`
ghaiszaher Aug 7, 2023
5f408df
remove test case for now
ghaiszaher Aug 7, 2023
d0a149d
Merge branch 'main' into fix/vcs-test
ghaiszaher Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions server/events/vcs/gitlab_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,8 @@ func TestGitlabClient_PullIsMergeable(t *testing.T) {
gitlabVersionUnder15_6 := "15.3.2-ce"
gitlabServerVersions := []string{gitlabVersionOver15_6, gitlabVersion15_6, gitlabVersionUnder15_6}
vcsStatusName := "atlantis-test"
defaultMR = 1
noHeadPipelineMR = 2
defaultMr := 1
noHeadPipelineMR := 2
Comment on lines -326 to +327
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marceloboeira apparently that wasn't enough, tests are still failing:

=== RUN   TestGitlabClient_PullIsMergeable/atlantis-test/plan#02
    gitlab_client_test.go:423: got unexpected request at "/api/v4/projects/4580910/repository/commits/sha/statuses"
    gitlab_client_test.go:451: unexpected error: GET http://127.0.0.1:52736/api/v4/projects/4580910/repository/commits/sha/statuses: 404 failed to parse unknown error format: not found
        
    --- FAIL: TestGitlabClient_PullIsMergeable/atlantis-test/plan#02 (1.09s)

Is a case missing in

atlantis/server/events/vcs/gitlab_client_test.go

Lines 397 to 425 in 4b34055

switch r.RequestURI {
case "/api/v4/":
// Rate limiter requests.
w.WriteHeader(http.StatusOK)
case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/1":
w.WriteHeader(http.StatusOK)
w.Write([]byte(pipelineSuccess)) // nolint: errcheck
case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/2":
w.WriteHeader(http.StatusOK)
w.Write([]byte(headPipelineNotAvailable)) // nolint: errcheck
case "/api/v4/projects/4580910":
w.WriteHeader(http.StatusOK)
w.Write([]byte(projectSuccess)) // nolint: errcheck
case "/api/v4/projects/4580910/repository/commits/67cb91d3f6198189f433c045154a885784ba6977/statuses":
w.WriteHeader(http.StatusOK)
response := fmt.Sprintf(`[{"id":133702594,"sha":"67cb91d3f6198189f433c045154a885784ba6977","ref":"patch-1","status":"%s","name":"%s","target_url":null,"description":"ApplySuccess","created_at":"2018-12-12T18:31:57.957Z","started_at":null,"finished_at":"2018-12-12T18:31:58.480Z","allow_failure":false,"coverage":null,"author":{"id":1755902,"username":"lkysow","name":"LukeKysow","state":"active","avatar_url":"https://secure.gravatar.com/avatar/25fd57e71590fe28736624ff24d41c5f?s=80&d=identicon","web_url":"https://gitlab.com/lkysow"}}]`, c.status, c.statusName)
w.Write([]byte(response)) // nolint: errcheck
case "/api/v4/version":
w.WriteHeader(http.StatusOK)
w.Header().Set("Content-Type", "application/json")
type version struct {
Version string
}
v := version{Version: serverVersion}
json.NewEncoder(w).Encode(v)
default:
t.Errorf("got unexpected request at %q", r.RequestURI)
http.Error(w, "not found", http.StatusNotFound)
}
?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh 😮‍💨

thanks for bringing this up.

I'm curious how the tests passed din the MR build... let me take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were skipped 😕 #3653 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try changing like 448 from "sha" to "67cb91d3f6198189f433c045154a885784ba6977"?

https://github.com/runatlantis/atlantis/pull/3653/files#diff-0cf0d1deb8569e72ff771612d80c7f49b7c947e8d2bf62b50562cf6cae54b493R448

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's what's causing the problem since the mock is expecting a commit hash instead of "sha" in the URL. Alternatively we could create a mock for "sha" for isolation but I don't see an additional value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marceloboeira that solves most of the failures, but still some test cases fail with:

    gitlab_client_test.go:452: [true != false]

This test case:

atlantis/server/events/vcs/gitlab_client_test.go

Lines 384 to 390 in 63dcd43

{
fmt.Sprintf("%s/plan", vcsStatusName),
models.SuccessCommitStatus,
gitlabServerVersions,
noHeadPipelineMR,
true,
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we have to revert the MR so we fix the build, I won't have time to look into it, it might be the default to isPipelineSkipped := true is the problem but I will have to open another MR with the fix (and figure out if we can have the CI running the tests or get them working properly locally)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marceloboeira that is indeed the case. I don't have much experience with Gitlab API so I can't tell what should be the correct behavior here.

Unless you think that the broken test might mean an incorrect behavior, how about removing this test case for now and then later you could look into it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code definitely fixes the nil pointer (which caused panic) but by fixing it it might have exposed a logic issue. In any case, it is very niche. If you want to remove/comment the test I will open another MR soon to fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, if you manage to merge your pull request first, I will close this one.

cases := []struct {
statusName string
status models.CommitStatus
Expand Down