From 197215f85df3b7240466040984205417819162d4 Mon Sep 17 00:00:00 2001 From: Nanik T Date: Tue, 8 Oct 2019 09:02:43 +1100 Subject: [PATCH 1/4] Make error message more human readable Changes made: * service.go - changes to the error string returned * service_test.go - modify TestWaitAndMaybeOpenService test case to accomodate for the new changes --- pkg/minikube/service/service.go | 2 +- pkg/minikube/service/service_test.go | 51 ++++++++++++++++++++++++++-- test.sh | 2 +- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/pkg/minikube/service/service.go b/pkg/minikube/service/service.go index 710ca0ebfeb1..faabe06b2b63 100644 --- a/pkg/minikube/service/service.go +++ b/pkg/minikube/service/service.go @@ -275,7 +275,7 @@ func WaitAndMaybeOpenService(api libmachine.API, namespace string, service strin chkSVC := func() error { return CheckService(namespace, service) } if err := retry.Expo(chkSVC, time.Duration(interval)*time.Second, time.Duration(wait)*time.Second); err != nil { - return errors.Wrapf(err, "Could not find finalized endpoint being pointed to by %s", service) + return errors.Wrapf(err, "Service %s was not found in default namespace , please try with 'minikube service %s -n Y'", service, service) } serviceURL, err := GetServiceURLsForService(api, namespace, service, urlTemplate) diff --git a/pkg/minikube/service/service_test.go b/pkg/minikube/service/service_test.go index 20b9f30fc036..be71633587b3 100644 --- a/pkg/minikube/service/service_test.go +++ b/pkg/minikube/service/service_test.go @@ -97,6 +97,29 @@ var serviceNamespaces = map[string]typed_core.ServiceInterface{ "default": defaultNamespaceServiceInterface, } +var nondefaultserviceNamespaces = map[string]typed_core.ServiceInterface{ + "default": nondefaultNamespaceServiceInterface, +} + + +var nondefaultNamespaceServiceInterface = &MockServiceInterface{ + ServiceList: &core.ServiceList{ + Items: []core.Service{ + { + ObjectMeta: meta.ObjectMeta{ + Name: "non-namespace-dashboard-no-ports", + Namespace: "non-default", + Labels: map[string]string{"mock": "mock"}, + }, + Spec: core.ServiceSpec{ + Ports: []core.ServicePort{}, + }, + }, + }, + }, +} + + var defaultNamespaceServiceInterface = &MockServiceInterface{ ServiceList: &core.ServiceList{ Items: []core.Service{ @@ -824,6 +847,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { urlMode bool https bool err bool + nondefault bool }{ /* { description: "no host", @@ -841,6 +865,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, https: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, + nondefault: false, }, { description: "correctly return serviceURLs, no https, no url mode", @@ -848,6 +873,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { service: "mock-dashboard", api: defaultAPI, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, + nondefault: false, }, { description: "correctly return serviceURLs, no https, url mode", @@ -856,6 +882,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, urlMode: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, + nondefault: false, }, { description: "correctly return serviceURLs, https, url mode", @@ -865,6 +892,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { urlMode: true, https: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, + nondefault: false, }, { description: "correctly return empty serviceURLs", @@ -873,14 +901,31 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, expected: []string{}, err: true, + nondefault: false, + }, + { + description: "correctly return empty serviceURLs", + namespace: "default", + service: "non-namespace-dashboard-no-ports", + api: defaultAPI, + expected: []string{}, + err: true, + nondefault: true, }, } defer revertK8sClient(K8s) for _, test := range tests { t.Run(test.description, func(t *testing.T) { - K8s = &MockClientGetter{ - servicesMap: serviceNamespaces, - endpointsMap: endpointNamespaces, + if (test.nondefault) { + K8s = &MockClientGetter{ + servicesMap: nondefaultserviceNamespaces, + endpointsMap: endpointNamespaces, + } + } else { + K8s = &MockClientGetter{ + servicesMap: serviceNamespaces, + endpointsMap: endpointNamespaces, + } } err := WaitAndMaybeOpenService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0) if test.err && err == nil { diff --git a/test.sh b/test.sh index c6173fb5b89d..c48b4a1ef055 100755 --- a/test.sh +++ b/test.sh @@ -20,7 +20,7 @@ TESTSUITE="${TESTSUITE:-all}" # if env variable not set run all the tests exitcode=0 if [[ "$TESTSUITE" = "lint" ]] || [[ "$TESTSUITE" = "all" ]] -then +then echo "= make lint =============================================================" make -s lint-ci && echo ok || ((exitcode += 4)) echo "= go mod ================================================================" From 75ce59d500346058f4ea010f1b4d4bbae98f873e Mon Sep 17 00:00:00 2001 From: Nanik T Date: Tue, 8 Oct 2019 10:12:35 +1100 Subject: [PATCH 2/4] Fix linting --- pkg/minikube/service/service_test.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/minikube/service/service_test.go b/pkg/minikube/service/service_test.go index be71633587b3..30eb6be7c056 100644 --- a/pkg/minikube/service/service_test.go +++ b/pkg/minikube/service/service_test.go @@ -101,7 +101,6 @@ var nondefaultserviceNamespaces = map[string]typed_core.ServiceInterface{ "default": nondefaultNamespaceServiceInterface, } - var nondefaultNamespaceServiceInterface = &MockServiceInterface{ ServiceList: &core.ServiceList{ Items: []core.Service{ @@ -119,7 +118,6 @@ var nondefaultNamespaceServiceInterface = &MockServiceInterface{ }, } - var defaultNamespaceServiceInterface = &MockServiceInterface{ ServiceList: &core.ServiceList{ Items: []core.Service{ @@ -865,7 +863,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, https: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, + nondefault: false, }, { description: "correctly return serviceURLs, no https, no url mode", @@ -873,7 +871,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { service: "mock-dashboard", api: defaultAPI, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, + nondefault: false, }, { description: "correctly return serviceURLs, no https, url mode", @@ -882,7 +880,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, urlMode: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, + nondefault: false, }, { description: "correctly return serviceURLs, https, url mode", @@ -892,7 +890,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { urlMode: true, https: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, + nondefault: false, }, { description: "correctly return empty serviceURLs", @@ -901,7 +899,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, expected: []string{}, err: true, - nondefault: false, + nondefault: false, }, { description: "correctly return empty serviceURLs", @@ -910,13 +908,13 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, expected: []string{}, err: true, - nondefault: true, + nondefault: true, }, } defer revertK8sClient(K8s) for _, test := range tests { t.Run(test.description, func(t *testing.T) { - if (test.nondefault) { + if test.nondefault { K8s = &MockClientGetter{ servicesMap: nondefaultserviceNamespaces, endpointsMap: endpointNamespaces, From 733f7bc3f3d58c228c2bb1f6c0bcf19417ace61b Mon Sep 17 00:00:00 2001 From: Nanik T Date: Thu, 10 Oct 2019 07:13:47 +1100 Subject: [PATCH 3/4] Move the non-found-service test into separate function and modify the error text to include service name --- pkg/minikube/service/service.go | 2 +- pkg/minikube/service/service_test.go | 67 ++++++++++++++++++++-------- 2 files changed, 49 insertions(+), 20 deletions(-) diff --git a/pkg/minikube/service/service.go b/pkg/minikube/service/service.go index faabe06b2b63..fcc5bb7a7c47 100644 --- a/pkg/minikube/service/service.go +++ b/pkg/minikube/service/service.go @@ -275,7 +275,7 @@ func WaitAndMaybeOpenService(api libmachine.API, namespace string, service strin chkSVC := func() error { return CheckService(namespace, service) } if err := retry.Expo(chkSVC, time.Duration(interval)*time.Second, time.Duration(wait)*time.Second); err != nil { - return errors.Wrapf(err, "Service %s was not found in default namespace , please try with 'minikube service %s -n Y'", service, service) + return errors.Wrapf(err, "Service %s was not found in %s namespace , please try with 'minikube service %s -n Y'", service, namespace, service) } serviceURL, err := GetServiceURLsForService(api, namespace, service, urlTemplate) diff --git a/pkg/minikube/service/service_test.go b/pkg/minikube/service/service_test.go index 30eb6be7c056..0d4d13fe571e 100644 --- a/pkg/minikube/service/service_test.go +++ b/pkg/minikube/service/service_test.go @@ -97,7 +97,7 @@ var serviceNamespaces = map[string]typed_core.ServiceInterface{ "default": defaultNamespaceServiceInterface, } -var nondefaultserviceNamespaces = map[string]typed_core.ServiceInterface{ +var serviceNamespaceOther = map[string]typed_core.ServiceInterface{ "default": nondefaultNamespaceServiceInterface, } @@ -107,7 +107,7 @@ var nondefaultNamespaceServiceInterface = &MockServiceInterface{ { ObjectMeta: meta.ObjectMeta{ Name: "non-namespace-dashboard-no-ports", - Namespace: "non-default", + Namespace: "cannot_be_found_namespace", Labels: map[string]string{"mock": "mock"}, }, Spec: core.ServiceSpec{ @@ -845,7 +845,6 @@ func TestWaitAndMaybeOpenService(t *testing.T) { urlMode bool https bool err bool - nondefault bool }{ /* { description: "no host", @@ -863,7 +862,6 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, https: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, }, { description: "correctly return serviceURLs, no https, no url mode", @@ -871,7 +869,6 @@ func TestWaitAndMaybeOpenService(t *testing.T) { service: "mock-dashboard", api: defaultAPI, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, }, { description: "correctly return serviceURLs, no https, url mode", @@ -880,7 +877,6 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, urlMode: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, }, { description: "correctly return serviceURLs, https, url mode", @@ -890,7 +886,6 @@ func TestWaitAndMaybeOpenService(t *testing.T) { urlMode: true, https: true, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, - nondefault: false, }, { description: "correctly return empty serviceURLs", @@ -899,8 +894,50 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, expected: []string{}, err: true, - nondefault: false, }, + } + defer revertK8sClient(K8s) + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + K8s = &MockClientGetter{ + servicesMap: serviceNamespaces, + endpointsMap: endpointNamespaces, + } + err := WaitAndMaybeOpenService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0) + if test.err && err == nil { + t.Fatalf("WaitAndMaybeOpenService expected to fail for test: %v", test) + } + if !test.err && err != nil { + t.Fatalf("WaitAndMaybeOpenService not expected to fail but got err: %v", err) + } + + }) + } +} + +func TestWaitAndMaybeOpenServiceForNotDefaultNamspace(t *testing.T) { + defaultAPI := &tests.MockAPI{ + FakeStore: tests.FakeStore{ + Hosts: map[string]*host.Host{ + config.GetMachineName(): { + Name: config.GetMachineName(), + Driver: &tests.MockDriver{}, + }, + }, + }, + } + defaultTemplate := template.Must(template.New("svc-template").Parse("http://{{.IP}}:{{.Port}}")) + + var tests = []struct { + description string + api libmachine.API + namespace string + service string + expected []string + urlMode bool + https bool + err bool + }{ { description: "correctly return empty serviceURLs", namespace: "default", @@ -908,22 +945,14 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, expected: []string{}, err: true, - nondefault: true, }, } defer revertK8sClient(K8s) for _, test := range tests { t.Run(test.description, func(t *testing.T) { - if test.nondefault { - K8s = &MockClientGetter{ - servicesMap: nondefaultserviceNamespaces, - endpointsMap: endpointNamespaces, - } - } else { - K8s = &MockClientGetter{ - servicesMap: serviceNamespaces, - endpointsMap: endpointNamespaces, - } + K8s = &MockClientGetter{ + servicesMap: serviceNamespaceOther, + endpointsMap: endpointNamespaces, } err := WaitAndMaybeOpenService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0) if test.err && err == nil { From 10ff44e5826535bcb636fe31879226fa1cfd5109 Mon Sep 17 00:00:00 2001 From: Nanik T Date: Fri, 18 Oct 2019 06:28:24 +1100 Subject: [PATCH 4/4] Modify output text to make it more readable as per PR comment --- pkg/minikube/service/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/minikube/service/service.go b/pkg/minikube/service/service.go index fcc5bb7a7c47..58a7264731b3 100644 --- a/pkg/minikube/service/service.go +++ b/pkg/minikube/service/service.go @@ -275,7 +275,7 @@ func WaitAndMaybeOpenService(api libmachine.API, namespace string, service strin chkSVC := func() error { return CheckService(namespace, service) } if err := retry.Expo(chkSVC, time.Duration(interval)*time.Second, time.Duration(wait)*time.Second); err != nil { - return errors.Wrapf(err, "Service %s was not found in %s namespace , please try with 'minikube service %s -n Y'", service, namespace, service) + return errors.Wrapf(err, "Service %s was not found in %q namespace. You may select another namespace by using 'minikube service %s -n ", service, namespace, service) } serviceURL, err := GetServiceURLsForService(api, namespace, service, urlTemplate)