Application image test rework#225
Conversation
|
@skoshchi Is it worth creating a separate function to wait for the "ACTIVE" state of all pods? Yes Otherwise the code looks OK |
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
2809157 to
38087e8
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
3 times, most recently
from
a52d443 to
567c24e
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
2 times, most recently
from
25a8660 to
f0ff8db
Compare
skoshchi
changed the title
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
ae61587 to
e79247a
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
e79247a to
8af29c9
Compare
|
|
||
| foundImage := foundDeployment.Spec.Template.Spec.Containers[0].Image | ||
| if foundImage != newImage { | ||
| if foundImage != testImgOld { |
There was a problem hiding this comment.
This looks like a logic mistake. Previous version compared the found image with the image which was set on line 83. You are comparing it with different image.
| return true | ||
| }, time.Second*30, time.Millisecond*250).Should(BeTrue()) | ||
|
|
||
| waitForPodsActiveState(name) |
There was a problem hiding this comment.
What is the difference between this check and check on the line 111?
| Testing can be configured via environment variables: | ||
| - NAMESPACE_FOR_TESTING - Namespace where webservers will be deployed. Default value: ```jws-operator-tests``` | ||
| - TEST_IMG - Default image for tests which do not require specific image. Default value: ```quay.io/web-servers/tomcat-demo``` | ||
| - TEST_IMG_OLD - The image required for the Update test. Default value: ```quay.io/web-servers/tomcat10update:latest``` |
There was a problem hiding this comment.
I don't like the name TEST_IMG_OLD. What about TEST_IMAGE_FOR_UPDATE or TEST_IMAGE_II with description: Additional/The second test image which is used in update test.
Or remove this settings and keep the tomcat10update image hard coded. The test just switches one image to the other and it doesn't matter what images it uses. It's only about whether the change happens.
| }, time.Minute*5, time.Second*5).Should(BeTrue(), "Building pods took too long time.") | ||
| } | ||
|
|
||
| func waitForPodsActiveState(name string) { |
There was a problem hiding this comment.
I have a problem with this function.
It succeeds when number of pods from operator status is the same as number of replicas set in the operator and all the pods are in active state. What if there are pods in terminating state?
What if the status of the operator does not work correctly, wouldn't be better to check the number of pods from deployment of directly from the cluster?
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
8af29c9 to
e0091a2
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
e0091a2 to
6e1c92f
Compare
| }, time.Second*420, time.Second*30).Should(BeTrue(), "Image Update Test: Required amount of replicas with updated image were not achieved") | ||
| } | ||
|
|
||
| func isApplicationImageWasUpdated(createdWebserver *webserversv1alpha1.WebServer, oldImage string) bool { |
There was a problem hiding this comment.
-
Update the name of the function.
-
Do you expect that this function will be used by multiple tests?
|
|
||
| // Checks pods state in the cluster | ||
| return createdWebserver.Spec.Replicas == foundDeployment.Status.AvailableReplicas | ||
| }, time.Second*420, time.Second*30).Should(BeTrue(), "Image Update Test: Required amount of replicas with updated image were not achieved") |
There was a problem hiding this comment.
The function is in testHelper hence it is expected that multiple tests will use ti. Hence in the log message there should not be a name of one particular tests.
| if int(createdWebserver.Spec.Replicas) != len(createdWebserver.Status.Pods) { | ||
| return false | ||
| } | ||
| for _, pod := range createdWebserver.Status.Pods { | ||
| if pod.State != webserversv1alpha1.PodStateActive { | ||
| fmt.Printf("Pod %s is in state %s\n", pod.Name, pod.State) | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Checks pods state in the cluster | ||
| return createdWebserver.Spec.Replicas == foundDeployment.Status.AvailableReplicas |
There was a problem hiding this comment.
Isn't the check against Deployment.Status.AvailableReplicas sufficient?
Why is it important to check pods from operator status and their state?
2 participants
Is it worth creating a separate function to wait for the "ACTIVE" state of all pods? This error appears only once. Can we have something similar in the future?