fix: gitlab client failing test

[ghaiszaher]

what

• Fix failing test caused by fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty #3428

[ghaiszaher]

I think the github workflows need to be revisited, gotest was skipped on the last commit in the mentioned PR:
https://github.com/runatlantis/atlantis/actions/runs/5762924587/job/15623661330
https://github.com/runatlantis/atlantis/actions/runs/5762924585

[ghaiszaher]

@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)
	}

?

[marceloboeira]

Ohh 😮‍💨

thanks for bringing this up.

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

[ghaiszaher]

They were skipped 😕 #3653 (comment)

[marceloboeira]

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

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

[marceloboeira]

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.

[ghaiszaher]

@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,
	},

[marceloboeira]

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)

[ghaiszaher]

@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?

[marceloboeira]

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

[ghaiszaher]

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

[ghaiszaher]

@GenPage @jamengual could you take a look at this pull request to resolve failing gotest checks in other PRs?

[chenrui333]

Looks good, thanks @ghaiszaher!