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
NE-1187: Use t.Run for table-driven tests #884
Conversation
@gcs278: This pull request references NE-1187 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest-required |
1 similar comment
/retest-required |
54d2b8b
to
cd113e6
Compare
tlsProfileSpec := tlsProfileSpecForSecurityProfile(ic.Spec.TLSSecurityProfile) | ||
err := validateTLSSecurityProfile(ic) | ||
if tc.valid && err != nil { | ||
t.Fatalf("unexpected error: %v\nprofile:\n%s", err, util.ToYaml(tlsProfileSpec)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this update after reviewing ,original commit was missing removal of tc.description.
mutated := original.DeepCopy() | ||
tc.mutate(mutated) | ||
if changed, updated := deploymentConfigChanged(&original, mutated); changed != tc.expect { | ||
t.Errorf("expect deploymentConfigChanged to be %t, got %t", tc.expect, changed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated this after code review
@@ -2293,11 +2315,11 @@ func TestComputeIngressUpgradeableCondition(t *testing.T) { | |||
} | |||
wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus) | |||
if err != nil { | |||
t.Errorf("%q: unexpected error from desiredLoadBalancerService: %v", tc.description, err) | |||
t.Errorf("unexpected error from desiredLoadBalancerService: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated this also after reviewing
@@ -2315,7 +2337,7 @@ func TestComputeIngressUpgradeableCondition(t *testing.T) { | |||
|
|||
actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret) | |||
if actual.Status != expectedStatus { | |||
t.Errorf("%q: expected Upgradeable to be %q, got %q", tc.description, expectedStatus, actual.Status) | |||
t.Errorf("expected Upgradeable to be %q, got %q", expectedStatus, actual.Status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also updated this after reviewing
/retest-required |
Merge late in release cycle to avoid backport issues, revisit 7/5 |
must gather failure TestUnmanagedDNSToManagedDNSInternalIngressController failure, but it is the same failure as https://issues.redhat.com/browse/OCPBUGS-13106 |
/hold |
/assign @rfredette |
wantPDB, pdb, err := desiredRouterPodDisruptionBudget(ic, deploymentRef) | ||
if err != nil { | ||
t.Fatalf("unexpected error: %v", err) | ||
} | ||
if !wantPDB { | ||
if tc.expectPDB { | ||
t.Error("expected true, got false") | ||
} | ||
} else if pdb == nil { | ||
t.Error("expected pointer, got nil") | ||
} else if pdb.Spec.MaxUnavailable == nil { | ||
t.Errorf("expected PDB with non-nil MaxUnavailable, got %#v", pdb) | ||
} else if *pdb.Spec.MaxUnavailable != tc.expectMaxUnavailable { | ||
t.Errorf("expected %#v, got %#v", tc.expectMaxUnavailable, pdb.Spec.MaxUnavailable) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe convert this to switch/case like you've done elsewhere in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
One nit, otherwise this looks good. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rfredette The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Use t.Run to run test cases for table-driven unit tests, for consistency. * pkg/operator/controller/canary/controller_test.go (Test_cycleServicePort): * pkg/operator/controller/canary/daemonset_test.go (Test_canaryDaemonsetChanged): * pkg/operator/controller/canary/namespace_test.go (Test_canaryNamespaceChanged): * pkg/operator/controller/canary/route_test.go (Test_canaryRouteChanged): * pkg/operator/controller/certificate/default_cert_test.go (Test_desiredRouterDefaultCertificateSecret): * pkg/operator/controller/dns/controller_test.go (Test_publishRecordToZones, TestPublishRecordToZonesMergesStatus) (Test_migrateRecordStatusConditions, Test_dnsZoneStatusSlicesEqual) (Test_recordIsAlreadyPublishedToZone): * pkg/operator/controller/ingress/controller_test.go (Test_tlsProfileSpecForSecurityProfile) (Test_tlsProfileSpecForIngressController) (Test_validateHTTPHeaderBufferValues, Test_validateClientTLS) (Test_IsProxyProtocolNeeded): * pkg/operator/controller/ingress/deployment_test.go (Test_inferTLSProfileSpecFromDeployment, TestDeploymentHash) (Test_deploymentConfigChanged): * pkg/operator/controller/ingress/dns_test.go (Test_desiredWildcardDNSRecord, Test_manageDNSForDomain): * pkg/operator/controller/ingress/load_balancer_service_test.go (Test_desiredLoadBalancerService, Test_shouldUseLocalWithFallback) (Test_isServiceOwnedByIngressController): * pkg/operator/controller/ingress/namespace_test.go (Test_routerNamespaceChanged): * pkg/operator/controller/ingress/nodeport_service_test.go (Test_nodePortServiceChanged): * pkg/operator/controller/ingress/poddisruptionbudget_test.go (Test_desiredRouterPodDisruptionBudget): * pkg/operator/controller/ingress/status_test.go (Test_computeDeploymentPodsScheduledCondition) (Test_computeIngressDegradedCondition) (Test_computeDeploymentRollingOutCondition) (Test_computeLoadBalancerProgressingStatus) (Test_computeDeploymentAvailableCondition) (Test_computeDeploymentReplicasMinAvailableCondition) (Test_computeDeploymentReplicasAllAvailableCondition) (Test_computeLoadBalancerStatus, Test_computeIngressProgressingCondition) (Test_computeIngressAvailableCondition, Test_ingressStatusesEqual) (Test_computeDNSStatus, Test_MergeConditions) (Test_computeAllowedSourceRanges): * pkg/operator/controller/ingressclass/ingressclass_test.go (Test_ingressClassChanged): * pkg/operator/controller/status/controller_test.go (Test_computeOperatorProgressingCondition, Test_operatorStatusesEqual) (Test_computeOperatorStatusVersions) (Test_computeOperatorUpgradeableCondition) (Test_computeOperatorEvaluationConditionsDetectedCondition): * pkg/operator/controller/sync-http-error-code-configmap/controller_test.go (Test_desiredHttpErrorCodeConfigMap): * pkg/util/network_test.go (Test_URI): * pkg/util/retryableerror/retryableerror_test.go (TestRetryableError): Use t.Run.
/lgtm |
/retest |
/unhold |
/hold Revision f2327ab was retested 3 times: holding |
must gather test failures Flaky CI...this is a unit test fix. |
@gcs278: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Use t.Run to run test cases for table-driven unit tests, for consistency.
(Test_tlsProfileSpecForIngressController)
(Test_validateHTTPHeaderBufferValues, Test_validateClientTLS) (Test_IsProxyProtocolNeeded):
(Test_computeIngressDegradedCondition)
(Test_computeDeploymentRollingOutCondition)
(Test_computeLoadBalancerProgressingStatus)
(Test_computeDeploymentAvailableCondition)
(Test_computeDeploymentReplicasMinAvailableCondition) (Test_computeDeploymentReplicasAllAvailableCondition) (Test_computeLoadBalancerStatus, Test_computeIngressProgressingCondition) (Test_computeIngressAvailableCondition, Test_ingressStatusesEqual) (Test_computeDNSStatus, Test_MergeConditions)
(Test_computeAllowedSourceRanges):
(Test_computeOperatorUpgradeableCondition)
(Test_computeOperatorEvaluationConditionsDetectedCondition):