Fix flaky test #2395
Fix flaky test #2395
Conversation
k8s-ci-robot
added
cncf-cla: yes
| @@ -52,8 +52,6 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| Expect(ing).NotTo(BeNil()) | |||
|
|
|||
| time.Sleep(10 * time.Second) | |||
|
|
|||
There was a problem hiding this comment.
This is not necessary because WaitForNginxServer below will make sure we wait enough after ensureIngress for the logs to appear.
| @@ -88,6 +86,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { | |||
| replicas := 2 | |||
| err := framework.UpdateDeployment(f.KubeClientSet, f.IngressController.Namespace, "http-svc", replicas, nil) | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| time.Sleep(5 * time.Second) | |||
There was a problem hiding this comment.
we need to give some time for the logs to appear, 5 seconds might be a bit too conservative but we can revisit this later
| log, err := f.NginxLogs() | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(log).ToNot(BeEmpty()) | ||
| index := strings.Index(log, fmt.Sprintf("reason: 'UPDATE' Ingress %s/foo.com", f.IngressController.Namespace)) | ||
| index := strings.Index(log, "id=should_handle_annotation_changes") |
There was a problem hiding this comment.
This is the actual fix. I noticed that marking the start of logs using reason: 'UPDATE' Ingress %s/foo.com is not enough because some logs appear before that even though those logs are result of that update.
So now we mark the start of the logs we are interested in using a request with unique query string to this test.
| @@ -153,7 +159,14 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { | |||
| _, err = f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.IngressController.Namespace).Update(ingress) | |||
| Expect(err).ToNot(HaveOccurred()) | |||
|
|
|||
| time.Sleep(5 * time.Second) | |||
| By("generating the respective ssl listen directive") | |||
There was a problem hiding this comment.
by moving WaitForNginxServer we can get rid of sleep and let WaitForNginxServer to take care of it
| @@ -193,12 +198,10 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { | |||
| } | |||
| _, err = f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.IngressController.Namespace).Update(ingress) | |||
| Expect(err).ToNot(HaveOccurred()) | |||
| time.Sleep(5 * time.Second) | |||
There was a problem hiding this comment.
we already sleep below, this is not necessary
| @@ -290,8 +293,6 @@ func enableDynamicConfiguration(namespace string, kubeClientSet kubernetes.Inter | |||
| return err | |||
| } | |||
|
|
|||
| time.Sleep(5 * time.Second) | |||
|
|
|||
There was a problem hiding this comment.
this was probably some leftover from previous implementation - I don't see any reason for it to stay now.
k8s-ci-robot
added
the
do-not-merge/work-in-progress
Codecov Report
@@ Coverage Diff @@
## master #2395 +/- ##
==========================================
+ Coverage 39.3% 39.32% +0.01%
==========================================
Files 73 73
Lines 5216 5216
==========================================
+ Hits 2050 2051 +1
+ Misses 2878 2876 -2
- Partials 288 289 +1
Continue to review full report at Codecov.
|
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign aledbf |
|
/approve |
|
/lgtm |
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi 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 |
What this PR does / why we need it:
The PR fixes flakiness with dynamic configuration tests and shaves off some seconds from the test run.
Which issue this PR fixes: n/a
Special notes for your reviewer: n/a