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.

Sign up for GitHub

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

Jump to bottom

Use framework.ExpectNoError() instead Expect() #74113

Merged
merged 1 commit into from
Feb 19, 2019
Merged

Use framework.ExpectNoError() instead Expect() #74113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions test/e2e/framework/exec_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (f *Framework) ExecWithOptions(options ExecOptions) (string, string, error)
Logf("ExecWithOptions %+v", options)

config, err := LoadConfig()
Expect(err).NotTo(HaveOccurred(), "failed to load restclient config")
ExpectNoError(err, "failed to load restclient config")

const tty = false

Expand Down Expand Up @@ -101,7 +101,7 @@ func (f *Framework) ExecCommandInContainerWithFullOutput(podName, containerName
func (f *Framework) ExecCommandInContainer(podName, containerName string, cmd ...string) string {
stdout, stderr, err := f.ExecCommandInContainerWithFullOutput(podName, containerName, cmd...)
Logf("Exec stderr: %q", stderr)
Expect(err).NotTo(HaveOccurred(),
ExpectNoError(err,
"failed to execute command in pod %v, container %v: %v",
podName, containerName, err)
return stdout
Expand All @@ -113,14 +113,14 @@ func (f *Framework) ExecShellInContainer(podName, containerName string, cmd stri

func (f *Framework) ExecCommandInPod(podName string, cmd ...string) string {
pod, err := f.PodClient().Get(podName, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred(), "failed to get pod")
ExpectNoError(err, "failed to get pod")
Expect(pod.Spec.Containers).NotTo(BeEmpty())
return f.ExecCommandInContainer(podName, pod.Spec.Containers[0].Name, cmd...)
}

func (f *Framework) ExecCommandInPodWithFullOutput(podName string, cmd ...string) (string, string, error) {
pod, err := f.PodClient().Get(podName, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred(), "failed to get pod")
ExpectNoError(err, "failed to get pod")
Expect(pod.Spec.Containers).NotTo(BeEmpty())
return f.ExecCommandInContainerWithFullOutput(podName, pod.Spec.Containers[0].Name, cmd...)
}
Expand Down
24 changes: 12 additions & 12 deletions test/e2e/framework/framework.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func (f *Framework) BeforeEach() {
componentTexts)
}

Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
config.QPS = f.Options.ClientQPS
config.Burst = f.Options.ClientBurst
if f.Options.GroupVersion != nil {
Expand All @@ -185,23 +185,23 @@ func (f *Framework) BeforeEach() {
config.ContentType = TestContext.KubeAPIContentType
}
f.ClientSet, err = clientset.NewForConfig(config)
Expect(err).NotTo(HaveOccurred())
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't ExpectNoError(err) without an additional message just as bad as Expect(err).NotTo(HaveOccurred()) regarding the information contained in the output when the check fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

@pohly Sorry for my late response.
ExpectNoError() can output err to log as

2162 func ExpectNoError(err error, explain ...interface{}) {
2163         ExpectNoErrorWithOffset(1, err, explain...)
2164 }
2165
2166 // ExpectNoErrorWithOffset checks if "err" is set, and if so, fails assertion while logging the error at "offset" levels above its caller
2167 // (for example, for call chain f -> g -> ExpectNoErrorWithOffset(1, ...) error would be logged for "f").
2168 func ExpectNoErrorWithOffset(offset int, err error, explain ...interface{}) {
2169         if err != nil {
2170                 Logf("Unexpected error occurred: %v", err)
2171         }
2172         ExpectWithOffset(1+offset, err).NotTo(HaveOccurred(), explain...)
2173 }

So the method seems better than Expect(err).NotTo(HaveOccurred()) which doesn't output it, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this is correct. Expect(err).NotTo(HaveOccurred()) does print the error as can be seen in the description of #34059, but often the error doesn't have enough information ("timed out waiting for the condition" - which condition?!).

The additional information is what has to be provided via the optional messages parameter, regardless whether NotTo or ExpectNoError` are used.

ExpectNoError(err)
f.APIExtensionsClientSet, err = apiextensionsclient.NewForConfig(config)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
f.InternalClientset, err = internalclientset.NewForConfig(config)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
f.AggregatorClient, err = aggregatorclient.NewForConfig(config)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
f.DynamicClient, err = dynamic.NewForConfig(config)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
// csi.storage.k8s.io is based on CRD, which is served only as JSON
jsonConfig := config
jsonConfig.ContentType = "application/json"
f.CSIClientSet, err = csi.NewForConfig(jsonConfig)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
// node.k8s.io is also based on CRD
f.NodeAPIClientSet, err = nodeapiclient.NewForConfig(jsonConfig)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)

// create scales getter, set GroupVersion and NegotiatedSerializer to default values
// as they are required when creating a REST client.
Expand All @@ -212,9 +212,9 @@ func (f *Framework) BeforeEach() {
config.NegotiatedSerializer = legacyscheme.Codecs
}
restClient, err := rest.RESTClientFor(config)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
discoClient, err := discovery.NewDiscoveryClientForConfig(config)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
cachedDiscoClient := cacheddiscovery.NewMemCacheClient(discoClient)
restMapper := restmapper.NewDeferredDiscoveryRESTMapper(cachedDiscoClient)
restMapper.Reset()
Expand All @@ -229,14 +229,14 @@ func (f *Framework) BeforeEach() {
namespace, err := f.CreateNamespace(f.BaseName, map[string]string{
"e2e-framework": f.BaseName,
})
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)

f.Namespace = namespace

if TestContext.VerifyServiceAccount {
By("Waiting for a default service account to be provisioned in namespace")
err = WaitForDefaultServiceAccountInNamespace(f.ClientSet, namespace.Name)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
} else {
Logf("Skipping waiting for service account")
}
Expand Down
4 changes: 1 addition & 3 deletions test/e2e/framework/gpu_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import (
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/uuid"

. "github.com/onsi/gomega"
"k8s.io/klog"
)

Expand Down Expand Up @@ -53,7 +51,7 @@ func NumberOfNVIDIAGPUs(node *v1.Node) int64 {
// NVIDIADevicePlugin returns the official Google Device Plugin pod for NVIDIA GPU in GKE
func NVIDIADevicePlugin() *v1.Pod {
ds, err := DsFromManifest(GPUDevicePluginDSYAML)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
p := &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "device-plugin-nvidia-gpu-" + string(uuid.NewUUID()),
Expand Down
1 change: 0 additions & 1 deletion test/e2e/framework/ingress/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ go_library(
"//test/e2e/manifest:go_default_library",
"//test/utils:go_default_library",
"//vendor/github.com/onsi/ginkgo:go_default_library",
"//vendor/github.com/onsi/gomega:go_default_library",
"//vendor/google.golang.org/api/compute/v1:go_default_library",
"//vendor/k8s.io/klog:go_default_library",
],
Expand Down
5 changes: 2 additions & 3 deletions test/e2e/framework/ingress/ingress_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import (
testutils "k8s.io/kubernetes/test/utils"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
)

const (
Expand Down Expand Up @@ -760,7 +759,7 @@ func (j *IngressTestJig) GetServicePorts(includeDefaultBackend bool) map[string]
svcPorts := make(map[string]v1.ServicePort)
if includeDefaultBackend {
defaultSvc, err := j.Client.CoreV1().Services(metav1.NamespaceSystem).Get(defaultBackendName, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
svcPorts[defaultBackendName] = defaultSvc.Spec.Ports[0]
}

Expand All @@ -775,7 +774,7 @@ func (j *IngressTestJig) GetServicePorts(includeDefaultBackend bool) map[string]
}
for _, svcName := range backendSvcs {
svc, err := j.Client.CoreV1().Services(j.Ingress.Namespace).Get(svcName, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
svcPorts[svcName] = svc.Spec.Ports[0]
}
return svcPorts
Expand Down
11 changes: 5 additions & 6 deletions test/e2e/framework/networking_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
Expand Down Expand Up @@ -507,7 +506,7 @@ func (config *NetworkingTestConfig) createSessionAffinityService(selector map[st

func (config *NetworkingTestConfig) DeleteNodePortService() {
err := config.getServiceClient().Delete(config.NodePortService.Name, nil)
Expect(err).NotTo(HaveOccurred(), "error while deleting NodePortService. err:%v)", err)
ExpectNoError(err, "error while deleting NodePortService. err:%v)", err)
time.Sleep(15 * time.Second) // wait for kube-proxy to catch up with the service being deleted.
}

Expand Down Expand Up @@ -535,13 +534,13 @@ func (config *NetworkingTestConfig) createTestPods() {

func (config *NetworkingTestConfig) createService(serviceSpec *v1.Service) *v1.Service {
_, err := config.getServiceClient().Create(serviceSpec)
Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Failed to create %s service: %v", serviceSpec.Name, err))
ExpectNoError(err, fmt.Sprintf("Failed to create %s service: %v", serviceSpec.Name, err))

err = WaitForService(config.f.ClientSet, config.Namespace, serviceSpec.Name, true, 5*time.Second, 45*time.Second)
Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("error while waiting for service:%s err: %v", serviceSpec.Name, err))
ExpectNoError(err, fmt.Sprintf("error while waiting for service:%s err: %v", serviceSpec.Name, err))

createdService, err := config.getServiceClient().Get(serviceSpec.Name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Failed to create %s service: %v", serviceSpec.Name, err))
ExpectNoError(err, fmt.Sprintf("Failed to create %s service: %v", serviceSpec.Name, err))

return createdService
}
Expand Down Expand Up @@ -705,7 +704,7 @@ func CheckReachabilityFromPod(expectToBeReachable bool, timeout time.Duration, n
}
return true, nil
})
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
}

// Does an HTTP GET, but does not reuse TCP connections
Expand Down
1 change: 0 additions & 1 deletion test/e2e/framework/providers/gce/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ go_library(
"//staging/src/k8s.io/cloud-provider:go_default_library",
"//test/e2e/framework:go_default_library",
"//vendor/github.com/onsi/ginkgo:go_default_library",
"//vendor/github.com/onsi/gomega:go_default_library",
"//vendor/google.golang.org/api/compute/v1:go_default_library",
"//vendor/google.golang.org/api/googleapi:go_default_library",
"//vendor/k8s.io/utils/exec:go_default_library",
Expand Down
4 changes: 1 addition & 3 deletions test/e2e/framework/providers/gce/firewall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ import (
"k8s.io/apimachinery/pkg/util/wait"
cloudprovider "k8s.io/cloud-provider"
"k8s.io/kubernetes/test/e2e/framework"

. "github.com/onsi/gomega"
)

const (
Expand Down Expand Up @@ -118,7 +116,7 @@ func GetClusterName(instancePrefix string) string {
// From cluster/gce/util.sh, all firewall rules should be consistent with the ones created by startup scripts.
func GetE2eFirewalls(masterName, masterTag, nodeTag, network, clusterIpRange string) []*compute.Firewall {
instancePrefix, err := GetInstancePrefix(masterName)
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
clusterName := GetClusterName(instancePrefix)

fws := []*compute.Firewall{}
Expand Down
19 changes: 9 additions & 10 deletions test/e2e/framework/providers/gce/ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
compute "google.golang.org/api/compute/v1"
"google.golang.org/api/googleapi"
"k8s.io/api/core/v1"
Expand Down Expand Up @@ -137,7 +136,7 @@ func (cont *GCEIngressController) ListGlobalForwardingRules() []*compute.Forward
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
fwdList := []*compute.ForwardingRule{}
l, err := gceCloud.ListGlobalForwardingRules()
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
for _, fwd := range l {
if cont.isOwned(fwd.Name) {
fwdList = append(fwdList, fwd)
Expand Down Expand Up @@ -171,7 +170,7 @@ func (cont *GCEIngressController) deleteForwardingRule(del bool) string {
func (cont *GCEIngressController) GetGlobalAddress(ipName string) *compute.Address {
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
ip, err := gceCloud.GetGlobalAddress(ipName)
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
return ip
}

Expand Down Expand Up @@ -199,7 +198,7 @@ func (cont *GCEIngressController) ListTargetHttpProxies() []*compute.TargetHttpP
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
tpList := []*compute.TargetHttpProxy{}
l, err := gceCloud.ListTargetHTTPProxies()
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
for _, tp := range l {
if cont.isOwned(tp.Name) {
tpList = append(tpList, tp)
Expand All @@ -212,7 +211,7 @@ func (cont *GCEIngressController) ListTargetHttpsProxies() []*compute.TargetHttp
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
tpsList := []*compute.TargetHttpsProxy{}
l, err := gceCloud.ListTargetHTTPSProxies()
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
for _, tps := range l {
if cont.isOwned(tps.Name) {
tpsList = append(tpsList, tps)
Expand Down Expand Up @@ -260,7 +259,7 @@ func (cont *GCEIngressController) ListUrlMaps() []*compute.UrlMap {
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
umList := []*compute.UrlMap{}
l, err := gceCloud.ListURLMaps()
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
for _, um := range l {
if cont.isOwned(um.Name) {
umList = append(umList, um)
Expand Down Expand Up @@ -302,7 +301,7 @@ func (cont *GCEIngressController) ListGlobalBackendServices() []*compute.Backend
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
beList := []*compute.BackendService{}
l, err := gceCloud.ListGlobalBackendServices()
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
for _, be := range l {
if cont.isOwned(be.Name) {
beList = append(beList, be)
Expand Down Expand Up @@ -374,7 +373,7 @@ func (cont *GCEIngressController) ListSslCertificates() []*compute.SslCertificat
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
sslList := []*compute.SslCertificate{}
l, err := gceCloud.ListSslCertificates()
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
for _, ssl := range l {
if cont.isOwned(ssl.Name) {
sslList = append(sslList, ssl)
Expand Down Expand Up @@ -415,7 +414,7 @@ func (cont *GCEIngressController) ListInstanceGroups() []*compute.InstanceGroup
gceCloud := cont.Cloud.Provider.(*Provider).gceCloud
igList := []*compute.InstanceGroup{}
l, err := gceCloud.ListInstanceGroups(cont.Cloud.Zone)
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
for _, ig := range l {
if cont.isOwned(ig.Name) {
igList = append(igList, ig)
Expand Down Expand Up @@ -565,7 +564,7 @@ func (cont *GCEIngressController) GetFirewallRuleName() string {
// methods here to be consistent with rest of the code in this repo.
func (cont *GCEIngressController) GetFirewallRule() *compute.Firewall {
fw, err := cont.GetFirewallRuleOrError()
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
return fw
}

Expand Down
6 changes: 3 additions & 3 deletions test/e2e/framework/providers/kubemark/kubemark.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ func (p *Provider) FrameworkBeforeEach(f *framework.Framework) {
externalConfig, err := clientcmd.BuildConfigFromFlags("", *kubemarkExternalKubeConfig)
externalConfig.QPS = f.Options.ClientQPS
externalConfig.Burst = f.Options.ClientBurst
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
externalClient, err := clientset.NewForConfig(externalConfig)
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
f.KubemarkExternalClusterClientSet = externalClient
p.closeChannel = make(chan struct{})
externalInformerFactory := informers.NewSharedInformerFactory(externalClient, 0)
kubemarkInformerFactory := informers.NewSharedInformerFactory(f.ClientSet, 0)
kubemarkNodeInformer := kubemarkInformerFactory.Core().V1().Nodes()
go kubemarkNodeInformer.Informer().Run(p.closeChannel)
p.controller, err = kubemark.NewKubemarkController(externalClient, externalInformerFactory, f.ClientSet, kubemarkNodeInformer)
Expect(err).NotTo(HaveOccurred())
framework.ExpectNoError(err)
externalInformerFactory.Start(p.closeChannel)
Expect(p.controller.WaitForCacheSync(p.closeChannel)).To(BeTrue())
go p.controller.Run(p.closeChannel)
Expand Down
3 changes: 1 addition & 2 deletions test/e2e/framework/service_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import (
imageutils "k8s.io/kubernetes/test/utils/image"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
)

const (
Expand Down Expand Up @@ -349,7 +348,7 @@ func GetNodePublicIps(c clientset.Interface) ([]string, error) {

func PickNodeIP(c clientset.Interface) string {
publicIps, err := GetNodePublicIps(c)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
if len(publicIps) == 0 {
Failf("got unexpected number (%d) of public IPs", len(publicIps))
}
Expand Down
12 changes: 5 additions & 7 deletions test/e2e/framework/statefulset_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ import (
"strings"
"time"

. "github.com/onsi/gomega"

apps "k8s.io/api/apps/v1"
appsV1beta2 "k8s.io/api/apps/v1beta2"
"k8s.io/api/core/v1"
Expand Down Expand Up @@ -98,18 +96,18 @@ func (s *StatefulSetTester) CreateStatefulSet(manifestPath, ns string) *apps.Sta

Logf("Parsing statefulset from %v", mkpath("statefulset.yaml"))
ss, err := manifest.StatefulSetFromManifest(mkpath("statefulset.yaml"), ns)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
Logf("Parsing service from %v", mkpath("service.yaml"))
svc, err := manifest.SvcFromManifest(mkpath("service.yaml"))
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)

Logf(fmt.Sprintf("creating " + ss.Name + " service"))
_, err = s.c.CoreV1().Services(ns).Create(svc)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)

Logf(fmt.Sprintf("creating statefulset %v/%v with %d replicas and selector %+v", ss.Namespace, ss.Name, *(ss.Spec.Replicas), ss.Spec.Selector))
_, err = s.c.AppsV1().StatefulSets(ns).Create(ss)
Expect(err).NotTo(HaveOccurred())
ExpectNoError(err)
s.WaitForRunningAndReady(*ss.Spec.Replicas, ss)
return ss
}
Expand Down Expand Up @@ -187,7 +185,7 @@ type VerifyStatefulPodFunc func(*v1.Pod)
func (s *StatefulSetTester) VerifyPodAtIndex(index int, ss *apps.StatefulSet, verify VerifyStatefulPodFunc) {
name := getStatefulSetPodNameAtIndex(index, ss)
pod, err := s.c.CoreV1().Pods(ss.Namespace).Get(name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Failed to get stateful pod %s for StatefulSet %s/%s", name, ss.Namespace, ss.Name))
ExpectNoError(err, fmt.Sprintf("Failed to get stateful pod %s for StatefulSet %s/%s", name, ss.Namespace, ss.Name))
verify(pod)
}

Expand Down