From a14be777765d3d7929d99562e5dc6d5eb3232499 Mon Sep 17 00:00:00 2001 From: Miguel Paolino Date: Fri, 18 Mar 2022 10:29:55 -0300 Subject: [PATCH 1/6] Adds error handling in all missing http.NewRequest calls under BaseClient --- api/client/base_client.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/api/client/base_client.go b/api/client/base_client.go index 2be8c34..47666a3 100644 --- a/api/client/base_client.go +++ b/api/client/base_client.go @@ -57,6 +57,9 @@ func (c *BaseClient) Get(kind string, resource string) ([]byte, int, error) { log.Printf("GET %s\n", url) req, err := http.NewRequest("GET", url, nil) + if err != nil { + return []byte(""), 0, err + } req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Token %s", c.authToken)) @@ -87,6 +90,9 @@ func (c *BaseClient) List(kind string) ([]byte, int, error) { log.Printf("GET %s\n", url) req, err := http.NewRequest("GET", url, nil) + if err != nil { + return []byte(""), 0, err + } req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Token %s", c.authToken)) @@ -117,6 +123,9 @@ func (c *BaseClient) ListWithParams(kind string, query url.Values) ([]byte, int, log.Printf("GET %s\n", url) req, err := http.NewRequest("GET", url, nil) + if err != nil { + return []byte(""), 0, err + } req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Token %s", c.authToken)) @@ -147,6 +156,9 @@ func (c *BaseClient) Delete(kind string, name string) ([]byte, int, error) { log.Printf("DELETE %s\n", url) req, err := http.NewRequest("DELETE", url, nil) + if err != nil { + return []byte(""), 0, err + } req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Token %s", c.authToken)) @@ -187,6 +199,9 @@ func (c *BaseClient) PostHeaders(kind string, resource []byte, headers map[strin log.Println("Resource", string(resource)) req, err := http.NewRequest("POST", url, bytes.NewBuffer(resource)) + if err != nil { + return []byte(""), 0, err + } req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Token %s", c.authToken)) @@ -221,6 +236,9 @@ func (c *BaseClient) Patch(kind string, name string, resource []byte) ([]byte, i log.Printf("PATCH %s\n", url) req, err := http.NewRequest("PATCH", url, bytes.NewBuffer(resource)) + if err != nil { + return []byte(""), 0, err + } req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Token %s", c.authToken)) From 446a24ca023b3c50eb9d8e0025879f0b0e23f28a Mon Sep 17 00:00:00 2001 From: Miguel Paolino Date: Fri, 18 Mar 2022 10:57:10 -0300 Subject: [PATCH 2/6] Handle error on waitingUntilJobIsRunning in ssh subpackage --- cmd/ssh/session.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/ssh/session.go b/cmd/ssh/session.go index 4e9df31..cf9316a 100644 --- a/cmd/ssh/session.go +++ b/cmd/ssh/session.go @@ -44,6 +44,11 @@ func StartDebugSession(job *models.JobV1Alpha, message string) error { fmt.Printf("* Waiting for debug session to boot up .") err := waitUntilJobIsRunning(job, func() { fmt.Printf(".") }) + if err != nil { + fmt.Printf("\n[ERROR] %s\n", err) + return err + } + fmt.Println() // Reload Job From 94809aa8bf6081278c5f103ca5578705c9031087 Mon Sep 17 00:00:00 2001 From: Miguel Paolino Date: Fri, 18 Mar 2022 13:17:58 -0300 Subject: [PATCH 3/6] Adds BaseClient Get test and error handling for file copy in newfileUploadRequest --- api/client/base_client.go | 3 ++ api/client/base_client_test.go | 52 ++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 api/client/base_client_test.go diff --git a/api/client/base_client.go b/api/client/base_client.go index 47666a3..58e8c08 100644 --- a/api/client/base_client.go +++ b/api/client/base_client.go @@ -277,6 +277,9 @@ func newfileUploadRequest(uri string, args map[string]string, fileArgName, path return nil, err } _, err = io.Copy(part, file) + if err != nil { + return nil, err + } for key, val := range args { _ = writer.WriteField(key, val) diff --git a/api/client/base_client_test.go b/api/client/base_client_test.go new file mode 100644 index 0000000..a86fabe --- /dev/null +++ b/api/client/base_client_test.go @@ -0,0 +1,52 @@ +package client + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + httpmock "gopkg.in/jarcoal/httpmock.v1" +) + +func Test__Get__Response200(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + received := false + + userAgent := "Test-User-Agent" + UserAgent = userAgent + + httpmock.RegisterResponder("GET", "https://org.semaphoretext.xyz/api/testapi/kind/resource", + func(req *http.Request) (*http.Response, error) { + received = true + assert.Equal(t, "Token MYTOKEN", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, userAgent, req.Header.Get("User-Agent")) + + return httpmock.NewStringResponse(200, ""), nil + }, + ) + + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.Get("kind", "resource") + + assert.NoError(t, err) + assert.Equal(t, 200, statusCode) + assert.Equal(t, []byte(""), response) + + if received == false { + t.Error("Expected the API to receive GET request") + } +} + +func Test__Get__InvalidURL(t *testing.T) { + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.Get("kind", "^-^") + + assert.Error(t, err) + assert.Equal(t, 0, statusCode) + assert.Equal(t, []byte(""), response) +} From 43a49190f737e1280bbebffe8984a60fb360721a Mon Sep 17 00:00:00 2001 From: Miguel Paolino Date: Fri, 18 Mar 2022 14:10:11 -0300 Subject: [PATCH 4/6] Adds BaseClient's ListWithParams test --- api/client/base_client_test.go | 90 ++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/api/client/base_client_test.go b/api/client/base_client_test.go index a86fabe..c3e6241 100644 --- a/api/client/base_client_test.go +++ b/api/client/base_client_test.go @@ -2,6 +2,7 @@ package client import ( "net/http" + "net/url" "testing" "github.com/stretchr/testify/assert" @@ -50,3 +51,92 @@ func Test__Get__InvalidURL(t *testing.T) { assert.Equal(t, 0, statusCode) assert.Equal(t, []byte(""), response) } + +func Test__List__Response200(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + received := false + + userAgent := "Test-User-Agent" + UserAgent = userAgent + + httpmock.RegisterResponder("GET", "https://org.semaphoretext.xyz/api/testapi/kind", + func(req *http.Request) (*http.Response, error) { + received = true + assert.Equal(t, "Token MYTOKEN", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, userAgent, req.Header.Get("User-Agent")) + + return httpmock.NewStringResponse(200, ""), nil + }, + ) + + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.List("kind") + + assert.NoError(t, err) + assert.Equal(t, 200, statusCode) + assert.Equal(t, []byte(""), response) + + if received == false { + t.Error("Expected the API to receive GET request") + } +} + +func Test__List__InvalidURL(t *testing.T) { + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.List("^-^") + + assert.Error(t, err) + assert.Equal(t, 0, statusCode) + assert.Equal(t, []byte(""), response) +} + +func Test__ListWithParams__Response200(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + received := false + + userAgent := "Test-User-Agent" + UserAgent = userAgent + + values := url.Values{} + values.Add("some", "value") + + httpmock.RegisterResponder("GET", "https://org.semaphoretext.xyz/api/testapi/kind", + func(req *http.Request) (*http.Response, error) { + received = true + assert.Equal(t, "Token MYTOKEN", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, userAgent, req.Header.Get("User-Agent")) + assert.Equal(t, "value", req.URL.Query().Get("some")) + return httpmock.NewStringResponse(200, ""), nil + }, + ) + + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.ListWithParams("kind", values) + + assert.NoError(t, err) + assert.Equal(t, 200, statusCode) + assert.Equal(t, []byte(""), response) + + if received == false { + t.Error("Expected the API to receive GET request") + } +} + +func Test__ListWithParams__InvalidURL(t *testing.T) { + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.ListWithParams("^-^", url.Values{}) + + assert.Error(t, err) + assert.Equal(t, 0, statusCode) + assert.Equal(t, []byte(""), response) +} From 1a1277271154b4c51ee811b8044de2a94292cfae Mon Sep 17 00:00:00 2001 From: Miguel Paolino Date: Fri, 18 Mar 2022 15:31:39 -0300 Subject: [PATCH 5/6] Added Delete and PostHeaders tests for BaseClient --- api/client/base_client_test.go | 100 ++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 3 deletions(-) diff --git a/api/client/base_client_test.go b/api/client/base_client_test.go index c3e6241..1a73d59 100644 --- a/api/client/base_client_test.go +++ b/api/client/base_client_test.go @@ -1,6 +1,7 @@ package client import ( + "io" "net/http" "net/url" "testing" @@ -38,7 +39,7 @@ func Test__Get__Response200(t *testing.T) { assert.Equal(t, []byte(""), response) if received == false { - t.Error("Expected the API to receive GET request") + t.Error("Expected the API to receive a GET request") } } @@ -81,7 +82,7 @@ func Test__List__Response200(t *testing.T) { assert.Equal(t, []byte(""), response) if received == false { - t.Error("Expected the API to receive GET request") + t.Error("Expected the API to receive a GET request") } } @@ -127,7 +128,7 @@ func Test__ListWithParams__Response200(t *testing.T) { assert.Equal(t, []byte(""), response) if received == false { - t.Error("Expected the API to receive GET request") + t.Error("Expected the API to receive a GET request") } } @@ -140,3 +141,96 @@ func Test__ListWithParams__InvalidURL(t *testing.T) { assert.Equal(t, 0, statusCode) assert.Equal(t, []byte(""), response) } + +func Test__Delete__Response200(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + received := false + + userAgent := "Test-User-Agent" + UserAgent = userAgent + + values := url.Values{} + values.Add("some", "value") + + httpmock.RegisterResponder("DELETE", "https://org.semaphoretext.xyz/api/testapi/kind/resource", + func(req *http.Request) (*http.Response, error) { + received = true + assert.Equal(t, "Token MYTOKEN", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, userAgent, req.Header.Get("User-Agent")) + return httpmock.NewStringResponse(200, ""), nil + }, + ) + + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.Delete("kind", "resource") + + assert.NoError(t, err) + assert.Equal(t, 200, statusCode) + assert.Equal(t, []byte(""), response) + + if received == false { + t.Error("Expected the API to receive a DELETE request") + } +} + +func Test__Delete__InvalidURL(t *testing.T) { + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.Delete("kind", "^-^") + + assert.Error(t, err) + assert.Equal(t, 0, statusCode) + assert.Equal(t, []byte(""), response) +} + +func Test__PostHeaders__Response201(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + received := false + + userAgent := "Test-User-Agent" + UserAgent = userAgent + headers := map[string]string{"key": "value"} + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + resource := []byte("resource") + + httpmock.RegisterResponder("POST", "https://org.semaphoretext.xyz/api/testapi/kind", + func(req *http.Request) (*http.Response, error) { + received = true + assert.Equal(t, "Token MYTOKEN", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, userAgent, req.Header.Get("User-Agent")) + assert.Equal(t, "value", req.Header.Get("key")) + + body, err := io.ReadAll(req.Body) + assert.NoError(t, err) + assert.Equal(t, "resource", string(body)) + return httpmock.NewStringResponse(201, ""), nil + }, + ) + + response, statusCode, err := client.PostHeaders("kind", resource, headers) + + assert.NoError(t, err) + assert.Equal(t, 201, statusCode) + assert.Equal(t, []byte(""), response) + + if received == false { + t.Error("Expected the API to receive a POST request") + } +} + +func Test__PostHeaders__InvalidURL(t *testing.T) { + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.PostHeaders("^-^", []byte(""), map[string]string{}) + + assert.Error(t, err) + assert.Equal(t, 0, statusCode) + assert.Equal(t, []byte(""), response) +} From 1a332e1813a6c599ad4be48a031d75bb4cb3f73b Mon Sep 17 00:00:00 2001 From: Miguel Paolino Date: Wed, 23 Mar 2022 14:24:54 -0300 Subject: [PATCH 6/6] Added some base_client tests --- api/client/base_client.go | 5 +- api/client/base_client_test.go | 126 ++++++++++++++++++++++++++++----- 2 files changed, 113 insertions(+), 18 deletions(-) diff --git a/api/client/base_client.go b/api/client/base_client.go index 58e8c08..b70e5d2 100644 --- a/api/client/base_client.go +++ b/api/client/base_client.go @@ -282,7 +282,10 @@ func newfileUploadRequest(uri string, args map[string]string, fileArgName, path } for key, val := range args { - _ = writer.WriteField(key, val) + err = writer.WriteField(key, val) + if err != nil { + return nil, err + } } err = writer.Close() if err != nil { diff --git a/api/client/base_client_test.go b/api/client/base_client_test.go index 1a73d59..ea3d862 100644 --- a/api/client/base_client_test.go +++ b/api/client/base_client_test.go @@ -1,9 +1,12 @@ package client import ( + "fmt" "io" "net/http" "net/url" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -37,10 +40,7 @@ func Test__Get__Response200(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 200, statusCode) assert.Equal(t, []byte(""), response) - - if received == false { - t.Error("Expected the API to receive a GET request") - } + assert.True(t, received, "Expected the API to receive a GET request") } func Test__Get__InvalidURL(t *testing.T) { @@ -80,10 +80,7 @@ func Test__List__Response200(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 200, statusCode) assert.Equal(t, []byte(""), response) - - if received == false { - t.Error("Expected the API to receive a GET request") - } + assert.True(t, received, "Expected the API to receive a GET request") } func Test__List__InvalidURL(t *testing.T) { @@ -127,9 +124,7 @@ func Test__ListWithParams__Response200(t *testing.T) { assert.Equal(t, 200, statusCode) assert.Equal(t, []byte(""), response) - if received == false { - t.Error("Expected the API to receive a GET request") - } + assert.True(t, received, "Expected the API to receive a GET request") } func Test__ListWithParams__InvalidURL(t *testing.T) { @@ -172,9 +167,7 @@ func Test__Delete__Response200(t *testing.T) { assert.Equal(t, 200, statusCode) assert.Equal(t, []byte(""), response) - if received == false { - t.Error("Expected the API to receive a DELETE request") - } + assert.True(t, received, "Expected the API to receive a DELETE request") } func Test__Delete__InvalidURL(t *testing.T) { @@ -220,9 +213,7 @@ func Test__PostHeaders__Response201(t *testing.T) { assert.Equal(t, 201, statusCode) assert.Equal(t, []byte(""), response) - if received == false { - t.Error("Expected the API to receive a POST request") - } + assert.True(t, received, "Expected the API to receive a POST request") } func Test__PostHeaders__InvalidURL(t *testing.T) { @@ -234,3 +225,104 @@ func Test__PostHeaders__InvalidURL(t *testing.T) { assert.Equal(t, 0, statusCode) assert.Equal(t, []byte(""), response) } + +func Test__Patch__Response200(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + received := false + + userAgent := "Test-User-Agent" + UserAgent = userAgent + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + resourceBody := []byte("{}") + + httpmock.RegisterResponder("PATCH", "https://org.semaphoretext.xyz/api/testapi/kind/resource", + func(req *http.Request) (*http.Response, error) { + received = true + assert.Equal(t, "Token MYTOKEN", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, userAgent, req.Header.Get("User-Agent")) + body, err := io.ReadAll(req.Body) + assert.NoErrorf(t, err, "") + assert.Equal(t, resourceBody, body) + return httpmock.NewStringResponse(200, ""), nil + }, + ) + + response, statusCode, err := client.Patch("kind", "resource", resourceBody) + + assert.NoError(t, err) + assert.Equal(t, 200, statusCode) + assert.Equal(t, []byte(""), response) + assert.True(t, received, "Expected the API to receive a PATCH request") +} + +func Test__Patch__InvalidURL(t *testing.T) { + client := NewBaseClient("MYTOKEN", "org.semaphoretext.xyz", "testapi") + + response, statusCode, err := client.Patch("^-^", "resource", []byte("")) + + assert.Error(t, err) + assert.Equal(t, 0, statusCode) + assert.Equal(t, []byte(""), response) +} + +func Test__newfileUploadRequest__Success(t *testing.T) { + // Lets create a temp file with the text "SOME CONTENT" + f, err := os.CreateTemp("", "semtest") + assert.NoError(t, err, "Could not create temporary file") + defer os.Remove(f.Name()) + content := "SOME CONTENT" + _, err = f.WriteString(content) + assert.NoError(t, err, "Could not write to temporary file") + + // The parameters for newfileUploadRequest + uri := "https://org.semaphoretext.xyz/api/upload" + args := map[string]string{"key": "value"} + fileArgName := "uploaded" + path := f.Name() + fileName := filepath.Base(path) + + // We explicitily define the type to make sure the return value matches + var req *http.Request + req, err = newfileUploadRequest(uri, args, fileArgName, path) + + // Lets check if request was correctly created + assert.NoError(t, err) + assert.Equal(t, "POST", req.Method) + assert.Equal(t, "value", req.FormValue("key")) + multiPartFile, multiPartHeader, err := req.FormFile("uploaded") + assert.NoError(t, err, "Could not get the FormFile from the upload request: %s", err) + assert.Equal(t, fileName, multiPartHeader.Filename) + buffer := make([]byte, len(content)) // "SOME CONTENT" + nbytes, err := multiPartFile.Read(buffer) + assert.NoErrorf(t, err, "Could not read the multipart content: %s", err) + assert.Equal(t, len(content), nbytes) + assert.Equal(t, content, string(buffer)) +} + +func Test__newfileUploadRequest__FailToOpenFile(t *testing.T) { + // Lets create a temp file, keep the path then delete it + // to make sure we have and invalid path + f, err := os.CreateTemp("", "semtest") + assert.NoError(t, err, "Could not create temporary file") + // Just in case test crashes prematurely + defer os.Remove(f.Name()) + + // The parameters for newfileUploadRequest + uri := "https://org.semaphoretext.xyz/api/upload" + args := map[string]string{"key": "value"} + fileArgName := "uploaded" + path := f.Name() // lets keep the path to the temp file + + // Delete the file + assert.NoError(t, os.Remove(f.Name())) + + req, err := newfileUploadRequest(uri, args, fileArgName, path) + + fmt.Printf("Error: %s", err) + assert.Error(t, err) + var result *http.Request = nil + assert.Equal(t, result, req) +}