From 129ffb7e1765ad0449875d6df17e95af8155f29f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 21 Aug 2018 00:29:35 +0100 Subject: [PATCH 1/2] fix(test): use httptest.newRequest instead of (incorrect) http.newRequest httptest.newRequest is suppose to be used for testing request handing, http.newRequest is for preparing outgoing requests, this allows to fix cache key selection and increase test coverage --- api_test.go | 2 +- autocomplete.go | 10 ----- autocomplete_test.go | 87 ++++++++++++++++++++++++-------------------- proxy_test.go | 4 +- views.go | 5 --- views_test.go | 20 +++++----- 6 files changed, 60 insertions(+), 68 deletions(-) diff --git a/api_test.go b/api_test.go index 1444e4232..6d1ce0e8c 100644 --- a/api_test.go +++ b/api_test.go @@ -836,7 +836,7 @@ func TestVerifyAllGroups(t *testing.T) { t.Logf("Testing API using mock files from Alertmanager %s", version) mockAlerts(version) r := ginTestEngine() - req, _ := http.NewRequest("GET", "/alerts.json", nil) + req := httptest.NewRequest("GET", "/alerts.json", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != http.StatusOK { diff --git a/autocomplete.go b/autocomplete.go index 969258d64..84d53356c 100644 --- a/autocomplete.go +++ b/autocomplete.go @@ -19,11 +19,6 @@ func knownLabelNames(c *gin.Context) { start := time.Now() cacheKey := c.Request.RequestURI - if cacheKey == "" { - // FIXME c.Request.RequestURI is empty when running tests for some reason - // needs checking, below acts as a workaround - cacheKey = c.Request.URL.RawQuery - } data, found := apiCache.Get(cacheKey) if found { @@ -67,11 +62,6 @@ func knownLabelValues(c *gin.Context) { start := time.Now() cacheKey := c.Request.RequestURI - if cacheKey == "" { - // FIXME c.Request.RequestURI is empty when running tests for some reason - // needs checking, below acts as a workaround - cacheKey = c.Request.URL.RawQuery - } data, found := apiCache.Get(cacheKey) if found { diff --git a/autocomplete_test.go b/autocomplete_test.go index a11f021c0..4c32fca93 100644 --- a/autocomplete_test.go +++ b/autocomplete_test.go @@ -41,32 +41,36 @@ func TestKnownLabelNames(t *testing.T) { mockAlerts(version) r := ginTestEngine() - req, _ := http.NewRequest("GET", "/labelNames.json", nil) - resp := httptest.NewRecorder() - r.ServeHTTP(resp, req) - if resp.Code != http.StatusOK { - t.Errorf("Invalid status code for request without any query: %d", resp.Code) - } - - for _, testCase := range labelTests { - url := fmt.Sprintf("/labelNames.json?term=%s", testCase.Term) - req, _ := http.NewRequest("GET", url, nil) + // repeat test a few times to test cached responses + for i := 1; i <= 3; i++ { + req := httptest.NewRequest("GET", "/labelNames.json", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != http.StatusOK { - t.Errorf("GET %s returned status %d", url, resp.Code) + t.Errorf("Invalid status code for request without any query: %d", resp.Code) } - ur := []string{} - err := json.Unmarshal(resp.Body.Bytes(), &ur) - if err != nil { - t.Errorf("Failed to unmarshal response: %s", err) - } + for _, testCase := range labelTests { + url := fmt.Sprintf("/labelNames.json?term=%s", testCase.Term) + req := httptest.NewRequest("GET", url, nil) + resp := httptest.NewRecorder() + r.ServeHTTP(resp, req) - if len(ur) != len(testCase.Results) { - t.Errorf("Invalid number of label names for %s, got %d, expected %d", url, len(ur), len(testCase.Results)) - t.Errorf("Results: %s", ur) + if resp.Code != http.StatusOK { + t.Errorf("GET %s returned status %d", url, resp.Code) + } + + ur := []string{} + err := json.Unmarshal(resp.Body.Bytes(), &ur) + if err != nil { + t.Errorf("Failed to unmarshal response: %s", err) + } + + if len(ur) != len(testCase.Results) { + t.Errorf("Invalid number of label names for %s, got %d, expected %d", url, len(ur), len(testCase.Results)) + t.Errorf("Results: %s", ur) + } } } } @@ -99,32 +103,35 @@ func TestKnownLabelValues(t *testing.T) { mockAlerts(version) r := ginTestEngine() - req, _ := http.NewRequest("GET", "/labelValues.json", nil) - resp := httptest.NewRecorder() - r.ServeHTTP(resp, req) - if resp.Code != http.StatusBadRequest { - t.Errorf("Invalid status code for request without any query: %d", resp.Code) - } - - for _, testCase := range valueTests { - url := fmt.Sprintf("/labelValues.json?name=%s", testCase.Name) - req, _ := http.NewRequest("GET", url, nil) + // repeat test a few times to test cached responses + for i := 1; i <= 3; i++ { + req := httptest.NewRequest("GET", "/labelValues.json", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) - - if resp.Code != http.StatusOK { - t.Errorf("GET %s returned status %d", url, resp.Code) + if resp.Code != http.StatusBadRequest { + t.Errorf("Invalid status code for request without any query: %d", resp.Code) } - ur := []string{} - err := json.Unmarshal(resp.Body.Bytes(), &ur) - if err != nil { - t.Errorf("Failed to unmarshal response: %s", err) - } + for _, testCase := range valueTests { + url := fmt.Sprintf("/labelValues.json?name=%s", testCase.Name) + req := httptest.NewRequest("GET", url, nil) + resp := httptest.NewRecorder() + r.ServeHTTP(resp, req) - if len(ur) != len(testCase.Results) { - t.Errorf("Invalid number of label values for %s, got %d, expected %d", url, len(ur), len(testCase.Results)) - t.Errorf("Results: %s", ur) + if resp.Code != http.StatusOK { + t.Errorf("GET %s returned status %d", url, resp.Code) + } + + ur := []string{} + err := json.Unmarshal(resp.Body.Bytes(), &ur) + if err != nil { + t.Errorf("Failed to unmarshal response: %s", err) + } + + if len(ur) != len(testCase.Results) { + t.Errorf("Invalid number of label values for %s, got %d, expected %d", url, len(ur), len(testCase.Results)) + t.Errorf("Results: %s", ur) + } } } } diff --git a/proxy_test.go b/proxy_test.go index 951cc5360..f983c9323 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -108,7 +108,7 @@ func TestProxy(t *testing.T) { if testCase.upstreamURI != "" { httpmock.RegisterResponder(testCase.method, testCase.upstreamURI, httpmock.NewStringResponder(testCase.code, testCase.response)) } - req, _ := http.NewRequest(testCase.method, testCase.localPath, nil) + req := httptest.NewRequest(testCase.method, testCase.localPath, nil) resp := newCloseNotifyingRecorder() r.ServeHTTP(resp, req) if resp.Code != testCase.code { @@ -215,7 +215,7 @@ func TestProxyHeaders(t *testing.T) { return httpmock.NewStringResponse(testCase.code, "ok"), nil }) - req, _ := http.NewRequest(testCase.method, testCase.localPath, nil) + req := httptest.NewRequest(testCase.method, testCase.localPath, nil) resp := newCloseNotifyingRecorder() r.ServeHTTP(resp, req) if resp.Code != testCase.code { diff --git a/views.go b/views.go index b625163a3..e01d32b70 100644 --- a/views.go +++ b/views.go @@ -222,11 +222,6 @@ func autocomplete(c *gin.Context) { start := time.Now() cacheKey := c.Request.RequestURI - if cacheKey == "" { - // FIXME c.Request.RequestURI is empty when running tests for some reason - // needs checking, below acts as a workaround - cacheKey = c.Request.URL.RawQuery - } data, found := apiCache.Get(cacheKey) if found { diff --git a/views_test.go b/views_test.go index 557afcd7c..c592ab12c 100644 --- a/views_test.go +++ b/views_test.go @@ -50,7 +50,7 @@ func ginTestEngine() *gin.Engine { func TestIndex(t *testing.T) { mockConfig() r := ginTestEngine() - req, _ := http.NewRequest("GET", "/", nil) + req := httptest.NewRequest("GET", "/", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != http.StatusOK { @@ -63,7 +63,7 @@ func TestIndexPrefix(t *testing.T) { defer os.Unsetenv("LISTEN_PREFIX") mockConfig() r := ginTestEngine() - req, _ := http.NewRequest("GET", "/prefix/", nil) + req := httptest.NewRequest("GET", "/prefix/", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != http.StatusOK { @@ -90,7 +90,7 @@ func TestAlerts(t *testing.T) { t.Logf("Testing alerts using mock files from Alertmanager %s", version) mockAlerts(version) r := ginTestEngine() - req, _ := http.NewRequest("GET", "/alerts.json?q=@receiver=by-cluster-service&q=alertname=HTTP_Probe_Failed&q=instance=web1", nil) + req := httptest.NewRequest("GET", "/alerts.json?q=@receiver=by-cluster-service&q=alertname=HTTP_Probe_Failed&q=instance=web1", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != http.StatusOK { @@ -160,7 +160,7 @@ func TestValidateAllAlerts(t *testing.T) { t.Logf("Validating alerts.json response using mock files from Alertmanager %s", version) mockAlerts(version) r := ginTestEngine() - req, _ := http.NewRequest("GET", "/alerts.json?q=alertname=HTTP_Probe_Failed&q=instance=web1", nil) + req := httptest.NewRequest("GET", "/alerts.json?q=alertname=HTTP_Probe_Failed&q=instance=web1", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != http.StatusOK { @@ -327,7 +327,7 @@ func TestAutocomplete(t *testing.T) { mockAlerts(version) r := ginTestEngine() - req, _ := http.NewRequest("GET", "/autocomplete.json", nil) + req := httptest.NewRequest("GET", "/autocomplete.json", nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != http.StatusBadRequest { @@ -336,7 +336,7 @@ func TestAutocomplete(t *testing.T) { for _, acTest := range acTests { url := fmt.Sprintf("/autocomplete.json?term=%s", acTest.Term) - req, _ := http.NewRequest("GET", url, nil) + req := httptest.NewRequest("GET", url, nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) @@ -395,7 +395,7 @@ func TestStaticFiles(t *testing.T) { mockConfig() r := ginTestEngine() for _, staticFileTest := range staticFileTests { - req, _ := http.NewRequest("GET", staticFileTest.path, nil) + req := httptest.NewRequest("GET", staticFileTest.path, nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != staticFileTest.code { @@ -433,7 +433,7 @@ func TestStaticFilesPrefix(t *testing.T) { mockConfig() r := ginTestEngine() for _, staticFileTest := range staticFilePrefixTests { - req, _ := http.NewRequest("GET", staticFileTest.path, nil) + req := httptest.NewRequest("GET", staticFileTest.path, nil) resp := httptest.NewRecorder() r.ServeHTTP(resp, req) if resp.Code != staticFileTest.code { @@ -447,7 +447,7 @@ func TestGzipMiddleware(t *testing.T) { r := ginTestEngine() paths := []string{"/", "/alerts.json", "/autocomplete.json", "/metrics"} for _, path := range paths { - req, _ := http.NewRequest("GET", path, nil) + req := httptest.NewRequest("GET", path, nil) req.Header.Set("Accept-Encoding", "gzip") resp := httptest.NewRecorder() r.ServeHTTP(resp, req) @@ -470,7 +470,7 @@ func TestGzipMiddlewareWithoutAcceptEncoding(t *testing.T) { r := ginTestEngine() paths := []string{"/", "/alerts.json", "/autocomplete.json", "/metrics"} for _, path := range paths { - req, _ := http.NewRequest("GET", path, nil) + req := httptest.NewRequest("GET", path, nil) req.Header.Set("Accept-Encoding", "") // ensure that we don't pass anything up resp := httptest.NewRecorder() r.ServeHTTP(resp, req) From 01477347690b34f8bf84bf3c653100a81f8fce85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 21 Aug 2018 09:26:03 +0100 Subject: [PATCH 2/2] fix(test): refactor autocomplete tests to be more DRY Too much duplication there, both loops can be merged --- autocomplete_test.go | 191 +++++++++++++++++++------------------------ 1 file changed, 85 insertions(+), 106 deletions(-) diff --git a/autocomplete_test.go b/autocomplete_test.go index 4c32fca93..237fcab82 100644 --- a/autocomplete_test.go +++ b/autocomplete_test.go @@ -3,134 +3,113 @@ package main import ( "encoding/json" "fmt" - "net/http" "net/http/httptest" "testing" "github.com/prymitive/unsee/internal/mock" ) -type labelTest struct { - Term string - Results []string +type requestTest struct { + PathSuffix string + StatusCode int + Results []string } -var labelTests = []labelTest{ +type autocompleteTest struct { + PathPrefix string + Tests []requestTest +} + +var autocompleteTests = []autocompleteTest{ { - Term: "a", - Results: []string{"alertname", "instance"}, + PathPrefix: "/labelNames.json", + Tests: []requestTest{ + { + PathSuffix: "", + StatusCode: 200, + Results: []string{"alertname", "cluster", "instance", "job"}, + }, + { + PathSuffix: "?term=", + StatusCode: 200, + Results: []string{"alertname", "cluster", "instance", "job"}, + }, + { + PathSuffix: "?term=a", + StatusCode: 200, + Results: []string{"alertname", "instance"}, + }, + { + PathSuffix: "?term=alertname", + StatusCode: 200, + Results: []string{"alertname"}, + }, + { + PathSuffix: "?term=1234567890", + StatusCode: 200, + Results: []string{}, + }, + }, }, { - Term: "alertname", - Results: []string{"alertname"}, - }, - { - Term: "1234567890", - Results: []string{}, - }, - { - Term: "", - Results: []string{"alertname", "cluster", "instance", "job"}, + PathPrefix: "/labelValues.json", + Tests: []requestTest{ + { + PathSuffix: "?name=", + StatusCode: 400, + Results: []string{}, + }, + { + PathSuffix: "?name=foobar", + StatusCode: 200, + Results: []string{}, + }, + { + PathSuffix: "?name=alertname", + StatusCode: 200, + Results: []string{"Free_Disk_Space_Too_Low", "HTTP_Probe_Failed", "Host_Down", "Memory_Usage_Too_High"}, + }, + { + PathSuffix: "?name=cluster", + StatusCode: 200, + Results: []string{"dev", "prod", "staging"}, + }, + }, }, } -func TestKnownLabelNames(t *testing.T) { +func TestLabelAutocomplete(t *testing.T) { mockConfig() for _, version := range mock.ListAllMocks() { - t.Logf("Testing known labels using mock files from Alertmanager %s", version) + t.Logf("Testing labels autocomplete using mock files from Alertmanager %s", version) mockAlerts(version) r := ginTestEngine() - // repeat test a few times to test cached responses - for i := 1; i <= 3; i++ { - req := httptest.NewRequest("GET", "/labelNames.json", nil) - resp := httptest.NewRecorder() - r.ServeHTTP(resp, req) + for _, testVariant := range autocompleteTests { + for _, testCase := range testVariant.Tests { + // repeat each test a few times to test cached responses + for i := 1; i <= 3; i++ { + url := fmt.Sprintf("%s%s", testVariant.PathPrefix, testCase.PathSuffix) + req := httptest.NewRequest("GET", url, nil) + resp := httptest.NewRecorder() + r.ServeHTTP(resp, req) - if resp.Code != http.StatusOK { - t.Errorf("Invalid status code for request without any query: %d", resp.Code) - } + if resp.Code != testCase.StatusCode { + t.Errorf("GET %s returned status %d, expected %d", url, resp.Code, testCase.StatusCode) + } - for _, testCase := range labelTests { - url := fmt.Sprintf("/labelNames.json?term=%s", testCase.Term) - req := httptest.NewRequest("GET", url, nil) - resp := httptest.NewRecorder() - r.ServeHTTP(resp, req) + if resp.Code < 300 { + ur := []string{} + err := json.Unmarshal(resp.Body.Bytes(), &ur) + if err != nil { + t.Errorf("Failed to unmarshal response: %s", err) + } - if resp.Code != http.StatusOK { - t.Errorf("GET %s returned status %d", url, resp.Code) - } - - ur := []string{} - err := json.Unmarshal(resp.Body.Bytes(), &ur) - if err != nil { - t.Errorf("Failed to unmarshal response: %s", err) - } - - if len(ur) != len(testCase.Results) { - t.Errorf("Invalid number of label names for %s, got %d, expected %d", url, len(ur), len(testCase.Results)) - t.Errorf("Results: %s", ur) - } - } - } - } -} - -type valueTest struct { - Name string - Results []string -} - -var valueTests = []valueTest{ - { - Name: "foobar", - Results: []string{}, - }, - { - Name: "alertname", - Results: []string{"Free_Disk_Space_Too_Low", "HTTP_Probe_Failed", "Host_Down", "Memory_Usage_Too_High"}, - }, - { - Name: "cluster", - Results: []string{"dev", "prod", "staging"}, - }, -} - -func TestKnownLabelValues(t *testing.T) { - mockConfig() - for _, version := range mock.ListAllMocks() { - t.Logf("Testing known label values using mock files from Alertmanager %s", version) - mockAlerts(version) - r := ginTestEngine() - - // repeat test a few times to test cached responses - for i := 1; i <= 3; i++ { - req := httptest.NewRequest("GET", "/labelValues.json", nil) - resp := httptest.NewRecorder() - r.ServeHTTP(resp, req) - if resp.Code != http.StatusBadRequest { - t.Errorf("Invalid status code for request without any query: %d", resp.Code) - } - - for _, testCase := range valueTests { - url := fmt.Sprintf("/labelValues.json?name=%s", testCase.Name) - req := httptest.NewRequest("GET", url, nil) - resp := httptest.NewRecorder() - r.ServeHTTP(resp, req) - - if resp.Code != http.StatusOK { - t.Errorf("GET %s returned status %d", url, resp.Code) - } - - ur := []string{} - err := json.Unmarshal(resp.Body.Bytes(), &ur) - if err != nil { - t.Errorf("Failed to unmarshal response: %s", err) - } - - if len(ur) != len(testCase.Results) { - t.Errorf("Invalid number of label values for %s, got %d, expected %d", url, len(ur), len(testCase.Results)) - t.Errorf("Results: %s", ur) + if len(ur) != len(testCase.Results) { + t.Errorf("Invalid number of responses for %s, got %d, expected %d", url, len(ur), len(testCase.Results)) + t.Errorf("Results: %s", ur) + } + } } } }