Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make error message more human readable #5563

Merged
merged 4 commits into from
Oct 17, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/minikube/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this text, but it should detect if namespace is default, otherwise the error message doesn't make much sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made changes to the string to include the service name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tstromberg any feedback ? Thanks

}

serviceURL, err := GetServiceURLsForService(api, namespace, service, urlTemplate)
Expand Down
49 changes: 46 additions & 3 deletions pkg/minikube/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,27 @@ 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{
Expand Down Expand Up @@ -824,6 +845,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) {
urlMode bool
https bool
err bool
nondefault bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bools should never be negative or have a name that starts with with no: it gets too confusing, particularly for non-english readers, when nondefault=false. I also don't understand what this bool is the default of.

Instead of default or nondefault, I think this would be more readable if each test directly defined which servicesMap to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look into the test again and refactor to make it more readable for code structure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the code to split out the test on it's own to make it more easier to understand for other. Let me know what you think ?. Thanks

}{
/* {
description: "no host",
Expand All @@ -841,13 +863,15 @@ 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",
namespace: "default",
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",
Expand All @@ -856,6 +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,
},
{
description: "correctly return serviceURLs, https, url mode",
Expand All @@ -865,6 +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,
},
{
description: "correctly return empty serviceURLs",
Expand All @@ -873,14 +899,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 {
Expand Down
2 changes: 1 addition & 1 deletion test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 ================================================================"
Expand Down