From 8413a21f6b6261a67afbff952057718cd65bd992 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Wed, 16 Apr 2025 12:06:46 +0100 Subject: [PATCH 1/7] Fix `StatusJSONResponse` usage Signed-off-by: Babak K. Shandiz --- pkg/cmd/gist/delete/delete_test.go | 34 +++++++++++++++--------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/pkg/cmd/gist/delete/delete_test.go b/pkg/cmd/gist/delete/delete_test.go index 24ca2bb33c2..bb49b974a1d 100644 --- a/pkg/cmd/gist/delete/delete_test.go +++ b/pkg/cmd/gist/delete/delete_test.go @@ -327,11 +327,12 @@ func Test_deleteRun(t *testing.T) { func Test_gistDelete(t *testing.T) { tests := []struct { - name string - httpStubs func(*httpmock.Registry) - hostname string - gistID string - wantErr error + name string + httpStubs func(*httpmock.Registry) + hostname string + gistID string + wantErr error + wantErrString string }{ { name: "successful delete", @@ -343,7 +344,6 @@ func Test_gistDelete(t *testing.T) { }, hostname: "github.com", gistID: "1234", - wantErr: nil, }, { name: "when an gist is not found, it returns a NotFoundError", @@ -362,17 +362,15 @@ func Test_gistDelete(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("DELETE", "gists/1234"), - httpmock.StatusJSONResponse(500, `{"message": "arbitrary error"}`), + httpmock.StatusJSONResponse(500, ghAPI.HTTPError{ + StatusCode: 500, + Message: "arbitrary error", + }), ) }, - hostname: "github.com", - gistID: "1234", - wantErr: api.HTTPError{ - HTTPError: &ghAPI.HTTPError{ - StatusCode: 500, - Message: "arbitrary error", - }, - }, + hostname: "github.com", + gistID: "1234", + wantErrString: "HTTP 500: arbitrary error (https://api.github.com/gists/1234)", }, } @@ -383,8 +381,10 @@ func Test_gistDelete(t *testing.T) { client := api.NewClientFromHTTP(&http.Client{Transport: reg}) err := deleteGist(client, tt.hostname, tt.gistID) - if tt.wantErr != nil { - assert.ErrorAs(t, err, &tt.wantErr) + if tt.wantErrString != "" { + assert.EqualError(t, err, tt.wantErrString) + } else if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr) } else { assert.NoError(t, err) } From ce5391d9df5c3189ee7a89519e75147b17fecf06 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 25 Apr 2025 10:59:21 +0100 Subject: [PATCH 2/7] Replace `assert` with `require` Signed-off-by: Babak K. Shandiz --- pkg/cmd/gist/delete/delete_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/gist/delete/delete_test.go b/pkg/cmd/gist/delete/delete_test.go index bb49b974a1d..bc88cdffcd2 100644 --- a/pkg/cmd/gist/delete/delete_test.go +++ b/pkg/cmd/gist/delete/delete_test.go @@ -18,6 +18,7 @@ import ( ghAPI "github.com/cli/go-gh/v2/pkg/api" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdDelete(t *testing.T) { @@ -382,11 +383,11 @@ func Test_gistDelete(t *testing.T) { err := deleteGist(client, tt.hostname, tt.gistID) if tt.wantErrString != "" { - assert.EqualError(t, err, tt.wantErrString) + require.EqualError(t, err, tt.wantErrString) } else if tt.wantErr != nil { - assert.ErrorIs(t, err, tt.wantErr) + require.ErrorIs(t, err, tt.wantErr) } else { - assert.NoError(t, err) + require.NoError(t, err) } }) From c4b2fd8396e55b8386b69ff496ece9a861982c4d Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 25 Apr 2025 11:02:53 +0100 Subject: [PATCH 3/7] Improve assertion against errors Signed-off-by: Babak K. Shandiz --- pkg/cmd/gist/delete/delete_test.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/gist/delete/delete_test.go b/pkg/cmd/gist/delete/delete_test.go index bc88cdffcd2..e9e707682d7 100644 --- a/pkg/cmd/gist/delete/delete_test.go +++ b/pkg/cmd/gist/delete/delete_test.go @@ -347,16 +347,17 @@ func Test_gistDelete(t *testing.T) { gistID: "1234", }, { - name: "when an gist is not found, it returns a NotFoundError", + name: "when a gist is not found, it returns a NotFoundError", httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("DELETE", "gists/1234"), httpmock.StatusStringResponse(404, "{}"), ) }, - hostname: "github.com", - gistID: "1234", - wantErr: shared.NotFoundErr, + hostname: "github.com", + gistID: "1234", + wantErr: shared.NotFoundErr, // To make sure we return the pre-defined error instance. + wantErrString: "not found", }, { name: "when there is a non-404 error deleting the gist, that error is returned", @@ -382,14 +383,16 @@ func Test_gistDelete(t *testing.T) { client := api.NewClientFromHTTP(&http.Client{Transport: reg}) err := deleteGist(client, tt.hostname, tt.gistID) - if tt.wantErrString != "" { - require.EqualError(t, err, tt.wantErrString) - } else if tt.wantErr != nil { - require.ErrorIs(t, err, tt.wantErr) - } else { + if tt.wantErrString == "" && tt.wantErr == nil { require.NoError(t, err) + } else { + if tt.wantErrString != "" { + require.EqualError(t, err, tt.wantErrString) + } + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + } } - }) } } From 61ee86ad236e6a6ac3a8d9bac24f45f086123d6d Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 25 Apr 2025 11:10:51 +0100 Subject: [PATCH 4/7] Add `JSONErrorResponse` helper func Signed-off-by: Babak K. Shandiz --- pkg/httpmock/stub.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 4e61d12f44f..63210679f87 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -9,6 +9,8 @@ import ( "os" "regexp" "strings" + + "github.com/cli/go-gh/v2/pkg/api" ) type Matcher func(req *http.Request) bool @@ -160,6 +162,9 @@ func JSONResponse(body interface{}) Responder { } } +// StatusJSONResponse turns the given argument into a JSON response. +// +// The argument is not meant to be a JSON string, unless it's intentional. func StatusJSONResponse(status int, body interface{}) Responder { return func(req *http.Request) (*http.Response, error) { b, _ := json.Marshal(body) @@ -170,6 +175,12 @@ func StatusJSONResponse(status int, body interface{}) Responder { } } +// JSONErrorResponse is a type-safe helper to avoid confusion around the +// provided argument. +func JSONErrorResponse(status int, err api.HTTPError) Responder { + return StatusJSONResponse(status, err) +} + func FileResponse(filename string) Responder { return func(req *http.Request) (*http.Response, error) { f, err := os.Open(filename) From c862ec21ba2bf6ba247066cc679266ef02d1f276 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 25 Apr 2025 11:11:59 +0100 Subject: [PATCH 5/7] Use `httpmock.JSONErrorResponse` Signed-off-by: Babak K. Shandiz --- pkg/cmd/gist/delete/delete_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/delete/delete_test.go b/pkg/cmd/gist/delete/delete_test.go index e9e707682d7..2c4df8d8d6f 100644 --- a/pkg/cmd/gist/delete/delete_test.go +++ b/pkg/cmd/gist/delete/delete_test.go @@ -364,7 +364,7 @@ func Test_gistDelete(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("DELETE", "gists/1234"), - httpmock.StatusJSONResponse(500, ghAPI.HTTPError{ + httpmock.JSONErrorResponse(500, ghAPI.HTTPError{ StatusCode: 500, Message: "arbitrary error", }), From 2e636eb5832569be706ec2e856665b746cd5732f Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 25 Apr 2025 11:22:24 +0100 Subject: [PATCH 6/7] Replace `StatusJSONResponse` to `JSONErrorResponse` for better readibility Signed-off-by: Babak K. Shandiz --- pkg/cmd/gpg-key/delete/delete_test.go | 2 +- pkg/cmd/run/watch/watch_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/gpg-key/delete/delete_test.go b/pkg/cmd/gpg-key/delete/delete_test.go index 115e72db239..dc730b100ed 100644 --- a/pkg/cmd/gpg-key/delete/delete_test.go +++ b/pkg/cmd/gpg-key/delete/delete_test.go @@ -177,7 +177,7 @@ func Test_deleteRun(t *testing.T) { opts: DeleteOptions{KeyID: "ABC123", Confirmed: true}, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "user/gpg_keys"), httpmock.StatusStringResponse(200, keysResp)) - reg.Register(httpmock.REST("DELETE", "user/gpg_keys/123"), httpmock.StatusJSONResponse(404, api.HTTPError{ + reg.Register(httpmock.REST("DELETE", "user/gpg_keys/123"), httpmock.JSONErrorResponse(404, api.HTTPError{ StatusCode: 404, Message: "GPG key 123 not found", })) diff --git a/pkg/cmd/run/watch/watch_test.go b/pkg/cmd/run/watch/watch_test.go index d42e8d3d80c..49e56217b41 100644 --- a/pkg/cmd/run/watch/watch_test.go +++ b/pkg/cmd/run/watch/watch_test.go @@ -316,7 +316,7 @@ func TestWatchRun(t *testing.T) { ) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/1234"), - httpmock.StatusJSONResponse(404, api.HTTPError{ + httpmock.JSONErrorResponse(404, api.HTTPError{ StatusCode: 404, Message: "run 1234 not found", }), From ddd22d679df8c79f6408dabdda4beea8d6fc0edc Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 25 Apr 2025 11:23:16 +0100 Subject: [PATCH 7/7] Fix improper use of `StatsJSONResponse` Signed-off-by: Babak K. Shandiz --- pkg/cmd/repo/autolink/delete/http_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/repo/autolink/delete/http_test.go b/pkg/cmd/repo/autolink/delete/http_test.go index a2676178db4..a0aec5e131a 100644 --- a/pkg/cmd/repo/autolink/delete/http_test.go +++ b/pkg/cmd/repo/autolink/delete/http_test.go @@ -7,6 +7,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/go-gh/v2/pkg/api" "github.com/stretchr/testify/require" ) @@ -14,10 +15,10 @@ func TestAutolinkDeleter_Delete(t *testing.T) { repo := ghrepo.New("OWNER", "REPO") tests := []struct { - name string - id string - stubStatus int - stubRespJSON string + name string + id string + stubStatus int + stubResp any expectErr bool expectedErrMsg string @@ -31,17 +32,18 @@ func TestAutolinkDeleter_Delete(t *testing.T) { name: "404 repo or autolink not found", id: "123", stubStatus: http.StatusNotFound, - stubRespJSON: `{}`, // API response not used in output expectErr: true, expectedErrMsg: "error deleting autolink: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/repos/OWNER/REPO/autolinks/123)", }, { - name: "500 unexpected error", - id: "123", - stubRespJSON: `{"messsage": "arbitrary error"}`, + name: "500 unexpected error", + id: "123", + stubResp: api.HTTPError{ + Message: "arbitrary error", + }, stubStatus: http.StatusInternalServerError, expectErr: true, - expectedErrMsg: "HTTP 500 (https://api.github.com/repos/OWNER/REPO/autolinks/123)", + expectedErrMsg: "HTTP 500: arbitrary error (https://api.github.com/repos/OWNER/REPO/autolinks/123)", }, } @@ -53,7 +55,7 @@ func TestAutolinkDeleter_Delete(t *testing.T) { http.MethodDelete, fmt.Sprintf("repos/%s/%s/autolinks/%s", repo.RepoOwner(), repo.RepoName(), tt.id), ), - httpmock.StatusJSONResponse(tt.stubStatus, tt.stubRespJSON), + httpmock.StatusJSONResponse(tt.stubStatus, tt.stubResp), ) defer reg.Verify(t)