tomncooper commented on code in PR #896:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/896#discussion_r1796692951


##########
flink-autoscaler/pom.xml:
##########
@@ -107,6 +107,12 @@ under the License.
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>org.awaitility</groupId>
+            <artifactId>awaitility</artifactId>
+            <version>${awaitility.version}</version>

Review Comment:
   Should this be test scoped?



##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/metrics/KubernetesClientMetricsTest.java:
##########
@@ -456,33 +339,101 @@ public void testAPIServerIsDown() {
 
         var deployment = TestUtils.buildApplicationCluster();
         mockServer.shutdown();
-        assertEquals(
-                0,
-                
listener.getCounter(listener.getMetricId(REQUEST_FAILED_COUNTER_ID))
-                        .get()
-                        .getCount());
-        assertEquals(
-                0.0,
-                
listener.getMeter(listener.getMetricId(REQUEST_FAILED_METER_ID)).get().getRate());
-        Awaitility.await()
-                .atMost(1, TimeUnit.MINUTES)
+
+        assertCounterIsZero(listener, REQUEST_FAILED_COUNTER_ID);
+        assertRateIsZero(listener, REQUEST_FAILED_METER_ID);
+
+        await().atMost(20, TimeUnit.SECONDS)
                 .until(
                         () -> {
                             assertThrows(
                                     KubernetesClientException.class,
                                     () -> 
kubernetesClient.resource(deployment).createOrReplace());
-                            return listener.getCounter(
-                                                            
listener.getMetricId(
-                                                                    
REQUEST_FAILED_COUNTER_ID))
-                                                    .get()
-                                                    .getCount()
-                                            > 0
-                                    && listener.getMeter(
-                                                            
listener.getMetricId(
-                                                                    
REQUEST_FAILED_METER_ID))
-                                                    .get()
-                                                    .getRate()
-                                            > 0.01;
+                            assertCounterIsPositive(listener);
+                            assertPositiveRate(listener, 
REQUEST_FAILED_METER_ID);
+                            return true;
                         });
     }
+
+    private static void assertRateIsZero(TestingMetricListener listener, 
String meterId) {
+        assertPositiveRate(listener, meterId, meter -> 
assertThat(meter.getRate()).isEqualTo(0.0));

Review Comment:
   Testing for equality to a float might be problematic (unless `isEqualTo` 
does something clever)?
   
   Would it be safer to test for > and < some small range either side of zero?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to