From 5d1511a5627399442ead58f6f06597e23d256587 Mon Sep 17 00:00:00 2001 From: Latiif Date: Sat, 11 Jan 2020 10:43:13 +0100 Subject: [PATCH 1/4] Add tests for GetRelease functionality - Add test for GetRelease in /pkg/agent - Add 'action=get' tests for /kubeops/handler - Add tests for 'dashboardcompat.go' in /kubeops/handler - Replace newConfigFixture with newActionConfigFixture - Skip tests blocked by memory driver bug TODO: REMOVE CHANGES FROM THIS COMMIT WHEN MEMORY DRIVER IS FIXED Signed-off-by: Latiif --- .../internal/handler/dashboardcompat_test.go | 200 ++++++++++++++++++ cmd/kubeops/internal/handler/handler_test.go | 44 ++++ pkg/agent/agent_test.go | 58 +++++ 3 files changed, 302 insertions(+) create mode 100644 cmd/kubeops/internal/handler/dashboardcompat_test.go diff --git a/cmd/kubeops/internal/handler/dashboardcompat_test.go b/cmd/kubeops/internal/handler/dashboardcompat_test.go new file mode 100644 index 00000000000..6a3acc235b7 --- /dev/null +++ b/cmd/kubeops/internal/handler/dashboardcompat_test.go @@ -0,0 +1,200 @@ +package handler + +import ( + "net/http/httptest" + "runtime/debug" + + "github.com/google/go-cmp/cmp" + "github.com/kubeapps/common/response" + h3chart "helm.sh/helm/v3/pkg/chart" + h3 "helm.sh/helm/v3/pkg/release" + h2chart "k8s.io/helm/pkg/proto/hapi/chart" + h2 "k8s.io/helm/pkg/proto/hapi/release" + + "testing" +) + +func TestNewDashboardCompatibleRelease(t *testing.T) { + type testScenario struct { + // Scenario params + Description string + Helm3Release h3.Release + Helm2Release h2.Release + MarshallingFunction func(h2.Release) string + ShouldFail bool + } + tests := []testScenario{ + { + Description: "Two equivalent releases", + MarshallingFunction: asResponse, + Helm3Release: h3.Release{ + Name: "Foo", + Namespace: "default", + Chart: &h3chart.Chart{ + Metadata: &h3chart.Metadata{}, + Values: map[string]interface{}{ + "port": 8080, + }, + }, + Info: &h3.Info{ + Status: h3.StatusDeployed, + }, + Version: 1, + Config: map[string]interface{}{ + "port": 3000, + "user": map[string]interface{}{ + "name": "user1", + "password": "123456", + }, + }, + }, + Helm2Release: h2.Release{ + Name: "Foo", + Namespace: "default", + Info: &h2.Info{ + Status: &h2.Status{ + Code: h2.Status_DEPLOYED, + }, + }, + Chart: &h2chart.Chart{ + Metadata: &h2chart.Metadata{}, + Values: &h2chart.Config{ + Raw: "port: 8080\n", + }, + }, + Version: 1, + Config: &h2chart.Config{ + Raw: "port: 3000\nuser:\n name: user1\n password: \"123456\"\n", + }, + }, + }, + { + Description: "Two equivalent releases with switched order of values", + MarshallingFunction: asResponse, + Helm3Release: h3.Release{ + Name: "Foo", + Namespace: "default", + Chart: &h3chart.Chart{ + Metadata: &h3chart.Metadata{}, + Values: map[string]interface{}{}, + }, + Info: &h3.Info{ + Status: h3.StatusDeployed, + }, + Version: 1, + Config: map[string]interface{}{ + "user": map[string]interface{}{ + "password": "123456", + "name": "user1", + }, + "port": 3000, + }, + }, + Helm2Release: h2.Release{ + Name: "Foo", + Namespace: "default", + Info: &h2.Info{ + Status: &h2.Status{ + Code: h2.Status_DEPLOYED, + }, + }, + Chart: &h2chart.Chart{ + Metadata: &h2chart.Metadata{}, + Values: &h2chart.Config{ + Raw: "{}\n", + }, + }, + Version: 1, + Config: &h2chart.Config{ + Raw: "port: 3000\nuser:\n name: user1\n password: \"123456\"\n", + }, + }, + }, + { + Description: "Two equivalent releases with different versions", + ShouldFail: true, + MarshallingFunction: asResponse, + Helm3Release: h3.Release{ + Name: "Foo", + Namespace: "default", + Chart: &h3chart.Chart{ + Metadata: &h3chart.Metadata{}, + Values: map[string]interface{}{ + "port": 8080, + }, + }, + Info: &h3.Info{ + Status: h3.StatusDeployed, + }, + Version: 1, + Config: map[string]interface{}{ + "port": 3000, + "user": map[string]interface{}{ + "name": "user1", + "password": "123456", + }, + }, + }, + Helm2Release: h2.Release{ + Name: "Foo", + Namespace: "default", + Info: &h2.Info{ + Status: &h2.Status{ + Code: h2.Status_DEPLOYED, + }, + }, + Chart: &h2chart.Chart{ + Metadata: &h2chart.Metadata{}, + Values: &h2chart.Config{ + Raw: "port: 8080\n", + }, + }, + Version: 5, + Config: &h2chart.Config{ + Raw: "port: 3000\nuser:\n name: user1\n password: \"123456\"\n", + }, + }, + }, + { + Description: "Incomplete Helm 3 Release", + ShouldFail: true, + MarshallingFunction: asResponse, + Helm3Release: h3.Release{Name: "Incomplete"}, + Helm2Release: h2.Release{Name: "Incomplete"}, + }, + } + + for _, test := range tests { + t.Run(test.Description, func(t *testing.T) { + // A panic is a failure for the test + defer func() { + if r := recover(); r != nil { + if !test.ShouldFail { + t.Errorf("Not expected to fail, yet got a panic: %v. \nStacktrace: \n%s", r, string(debug.Stack())) + } + } + }() + // Perform conversion + compatibleH3rls := newDashboardCompatibleRelease(test.Helm3Release) + // Marshall both: Compatible H3Release and H2Release + h3Marshalled := test.MarshallingFunction(compatibleH3rls) + t.Logf("Marshalled Helm 3 Release %s", h3Marshalled) + h2Marshalled := test.MarshallingFunction(test.Helm2Release) + t.Logf("Marshalled Helm 2 Release %s", h2Marshalled) + // Check result + areEqual := h2Marshalled == h3Marshalled + if test.ShouldFail && areEqual { + t.Errorf("Expected to fail, but are equal %v", cmp.Diff(h3Marshalled, h2Marshalled)) + } + if !test.ShouldFail && !areEqual { + t.Errorf("Not equal: %s", cmp.Diff(h3Marshalled, h2Marshalled)) + } + }) + } +} + +func asResponse(data h2.Release) string { + w := httptest.NewRecorder() + response.NewDataResponse(data).Write(w) + return w.Body.String() +} diff --git a/cmd/kubeops/internal/handler/handler_test.go b/cmd/kubeops/internal/handler/handler_test.go index baf95bee722..095215d4401 100644 --- a/cmd/kubeops/internal/handler/handler_test.go +++ b/cmd/kubeops/internal/handler/handler_test.go @@ -52,6 +52,7 @@ func TestActions(t *testing.T) { Description string ExistingReleases []release.Release DisableAuth bool + Skip bool //TODO: Remove this when the memory bug is fixed // Request params RequestBody string RequestQuery string @@ -117,10 +118,51 @@ func TestActions(t *testing.T) { }, ResponseBody: "", }, + { + // Scenario params + Description: "Get a non-existing release", + ExistingReleases: []release.Release{}, + DisableAuth: true, + Skip: true, + // Request params + RequestBody: "", + RequestQuery: "", + Action: "get", + Params: map[string]string{"namespace": "default", "releaseName": "foobar"}, + // Expected result + StatusCode: 404, + RemainingReleases: []release.Release{}, + ResponseBody: "", + }, + { + // Scenario params + Description: "Get a simple release", + ExistingReleases: []release.Release{ + createRelease("foo", "foobar", "default", 1, release.StatusDeployed), + createRelease("oof", "oofbar", "dev", 1, release.StatusDeployed), + }, + DisableAuth: true, + // Request params + RequestBody: "", + RequestQuery: "", + Action: "get", + Params: map[string]string{"namespace": "default", "releaseName": "foobar"}, + // Expected result + StatusCode: 200, + RemainingReleases: []release.Release{ + createRelease("foo", "foobar", "default", 1, release.StatusDeployed), + createRelease("oof", "oofbar", "dev", 1, release.StatusDeployed), + }, + ResponseBody: `{"data":{"name":"foobar","info":{"status":{"code":1}},"chart":{"metadata":{"name":"foo"},"values":{"raw":"{}\n"}},"config":{"raw":"{}\n"},"version":1,"namespace":"default"}}`, + }, } for _, test := range tests { t.Run(test.Description, func(t *testing.T) { + // TODO Remove this `if` statement after the memory driver bug is fixed + if test.Skip { + t.SkipNow() + } // Initialize environment for test req := httptest.NewRequest("GET", fmt.Sprintf("http://foo.bar%s", test.RequestQuery), strings.NewReader(test.RequestBody)) if !test.DisableAuth { @@ -138,6 +180,8 @@ func TestActions(t *testing.T) { } // Perform request switch test.Action { + case "get": + GetRelease(*cfg, response, req, test.Params) case "create": CreateRelease(*cfg, response, req, test.Params) default: diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index b2ef55b8905..d707343c4b1 100644 --- a/pkg/agent/agent_test.go +++ b/pkg/agent/agent_test.go @@ -70,6 +70,64 @@ func makeReleases(t *testing.T, actionConfig *action.Configuration, rels []relea } } +func TestGetRelease(t *testing.T) { + fooApp := releaseStub{"foo", "my_ns", 1, release.StatusDeployed} + barApp := releaseStub{"bar", "other_ns", 1, release.StatusDeployed} + testCases := []struct { + description string + existingReleases []releaseStub + targetApp string + targetNamespace string + expectedResult string + shouldFail bool + }{ + { + description: "Get an existing release", + existingReleases: []releaseStub{fooApp, barApp}, + targetApp: "foo", + targetNamespace: "my_ns", + expectedResult: "foo", + }, + { + description: "Get an existing release with default namespace", + existingReleases: []releaseStub{fooApp, barApp}, + targetApp: "foo", + targetNamespace: "", + expectedResult: "foo", + }, + { + description: "Get an non-existing release", + existingReleases: []releaseStub{barApp}, + targetApp: "foo", + targetNamespace: "my_ns", + expectedResult: "", + shouldFail: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + cfg := newActionConfigFixture(t) + makeReleases(t, cfg, tc.existingReleases) + rls, err := GetRelease(cfg, tc.targetApp) + if tc.shouldFail && err == nil { + t.Errorf("Get %s/%s should fail", tc.targetNamespace, tc.targetApp) + } + if !tc.shouldFail { + if err != nil { + t.Errorf("Unexpected error %v", err) + } + if rls == nil { + t.Fatalf("Release is nil: %v", rls) + } + if rls.Name != tc.expectedResult { + t.Errorf("Expecting app %s, received %s", tc.expectedResult, rls.Name) + } + } + }) + } +} + func TestCreateReleases(t *testing.T) { testCases := []struct { desc string From a74e1b1cfd1bbfb2e8c90f1cb30c2630ef4eac67 Mon Sep 17 00:00:00 2001 From: Latiif Date: Mon, 13 Jan 2020 14:11:47 +0100 Subject: [PATCH 2/4] Improve tests for 'dashboardcompat.go' - Remove non-functionality-testing tests. - Remove 'shouldFail' flag. - Capture panics and report them within the test suite. Signed-off-by: Latiif --- .../internal/handler/dashboardcompat_test.go | 65 +------------------ 1 file changed, 3 insertions(+), 62 deletions(-) diff --git a/cmd/kubeops/internal/handler/dashboardcompat_test.go b/cmd/kubeops/internal/handler/dashboardcompat_test.go index 6a3acc235b7..86549fd6ed5 100644 --- a/cmd/kubeops/internal/handler/dashboardcompat_test.go +++ b/cmd/kubeops/internal/handler/dashboardcompat_test.go @@ -21,7 +21,6 @@ func TestNewDashboardCompatibleRelease(t *testing.T) { Helm3Release h3.Release Helm2Release h2.Release MarshallingFunction func(h2.Release) string - ShouldFail bool } tests := []testScenario{ { @@ -110,68 +109,14 @@ func TestNewDashboardCompatibleRelease(t *testing.T) { }, }, }, - { - Description: "Two equivalent releases with different versions", - ShouldFail: true, - MarshallingFunction: asResponse, - Helm3Release: h3.Release{ - Name: "Foo", - Namespace: "default", - Chart: &h3chart.Chart{ - Metadata: &h3chart.Metadata{}, - Values: map[string]interface{}{ - "port": 8080, - }, - }, - Info: &h3.Info{ - Status: h3.StatusDeployed, - }, - Version: 1, - Config: map[string]interface{}{ - "port": 3000, - "user": map[string]interface{}{ - "name": "user1", - "password": "123456", - }, - }, - }, - Helm2Release: h2.Release{ - Name: "Foo", - Namespace: "default", - Info: &h2.Info{ - Status: &h2.Status{ - Code: h2.Status_DEPLOYED, - }, - }, - Chart: &h2chart.Chart{ - Metadata: &h2chart.Metadata{}, - Values: &h2chart.Config{ - Raw: "port: 8080\n", - }, - }, - Version: 5, - Config: &h2chart.Config{ - Raw: "port: 3000\nuser:\n name: user1\n password: \"123456\"\n", - }, - }, - }, - { - Description: "Incomplete Helm 3 Release", - ShouldFail: true, - MarshallingFunction: asResponse, - Helm3Release: h3.Release{Name: "Incomplete"}, - Helm2Release: h2.Release{Name: "Incomplete"}, - }, } for _, test := range tests { t.Run(test.Description, func(t *testing.T) { - // A panic is a failure for the test + // Capture the panic and report it in an orderly fashion defer func() { if r := recover(); r != nil { - if !test.ShouldFail { - t.Errorf("Not expected to fail, yet got a panic: %v. \nStacktrace: \n%s", r, string(debug.Stack())) - } + t.Errorf("Got a panic: %v. \nStacktrace: \n%s", r, string(debug.Stack())) } }() // Perform conversion @@ -182,11 +127,7 @@ func TestNewDashboardCompatibleRelease(t *testing.T) { h2Marshalled := test.MarshallingFunction(test.Helm2Release) t.Logf("Marshalled Helm 2 Release %s", h2Marshalled) // Check result - areEqual := h2Marshalled == h3Marshalled - if test.ShouldFail && areEqual { - t.Errorf("Expected to fail, but are equal %v", cmp.Diff(h3Marshalled, h2Marshalled)) - } - if !test.ShouldFail && !areEqual { + if h3Marshalled != h2Marshalled { t.Errorf("Not equal: %s", cmp.Diff(h3Marshalled, h2Marshalled)) } }) From 2fabd142f09cfbd2aede2fd33bd68bc8b98a8f03 Mon Sep 17 00:00:00 2001 From: Latiif Date: Mon, 13 Jan 2020 14:25:32 +0100 Subject: [PATCH 3/4] Ensure order of releases from Memory Driver Signed-off-by: Latiif --- cmd/kubeops/internal/handler/handler_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/kubeops/internal/handler/handler_test.go b/cmd/kubeops/internal/handler/handler_test.go index fe8b3e30a14..d2f3af11e29 100644 --- a/cmd/kubeops/internal/handler/handler_test.go +++ b/cmd/kubeops/internal/handler/handler_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io/ioutil" "net/http/httptest" + "sort" "strings" "testing" @@ -263,6 +264,14 @@ func TestActions(t *testing.T) { t.Errorf("Expecting a StatusCode %d, received %d", test.StatusCode, response.Code) } releases := derefReleases(cfg.ActionConfig.Releases) + // The Helm memory driver does not appear to have consistent ordering. + // See https://github.com/helm/helm/issues/7263 + // Just sort by "name.version.namespace" which is good enough here. + sort.Slice(releases, func(i, j int) bool { + iKey := fmt.Sprintf("%s.%d.%s", releases[i].Name, releases[i].Version, releases[i].Namespace) + jKey := fmt.Sprintf("%s.%d.%s", releases[j].Name, releases[j].Version, releases[j].Namespace) + return iKey < jKey + }) rlsComparer := cmp.Comparer(func(x release.Release, y release.Release) bool { return x.Name == y.Name && x.Version == y.Version && From eb54b98663ae43d99a41b174f827a4140e0fc6df Mon Sep 17 00:00:00 2001 From: Latiif Date: Mon, 13 Jan 2020 15:05:26 +0100 Subject: [PATCH 4/4] Add comments to field "MarshallingFunction" and to function "asResponse" Signed-off-by: Latiif --- cmd/kubeops/internal/handler/dashboardcompat_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cmd/kubeops/internal/handler/dashboardcompat_test.go b/cmd/kubeops/internal/handler/dashboardcompat_test.go index 86549fd6ed5..d12be5dc439 100644 --- a/cmd/kubeops/internal/handler/dashboardcompat_test.go +++ b/cmd/kubeops/internal/handler/dashboardcompat_test.go @@ -17,9 +17,13 @@ import ( func TestNewDashboardCompatibleRelease(t *testing.T) { type testScenario struct { // Scenario params - Description string - Helm3Release h3.Release - Helm2Release h2.Release + Description string + Helm3Release h3.Release + Helm2Release h2.Release + // MarshallingFunction defines what it means to + // parse a Helm 2 Release to a string i.e. which fields are relevant/redundant, + // and how to format the fields of a Helm 2 release. + // E.g. spew.Dumps is a valid marhsalling function. MarshallingFunction func(h2.Release) string } tests := []testScenario{ @@ -134,6 +138,8 @@ func TestNewDashboardCompatibleRelease(t *testing.T) { } } +// asResponse is one of many possible Marshalling functions +// However, it's the one most relevant for the current usage of 'dashboardcompat.go' func asResponse(data h2.Release) string { w := httptest.NewRecorder() response.NewDataResponse(data).Write(w)