1996fanrui commented on code in PR #721:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/721#discussion_r1416590912
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/autoscaler/KubernetesScalingRealizerTest.java:
##########
@@ -30,12 +31,23 @@
public class KubernetesScalingRealizerTest {
@Test
- public void testAutoscalerOverridesVertexIdsAreSorted() {
-
+ public void
testAutoscalerOverridesStringDoesNotChangeUnlessOverridesChange() {
KubernetesJobAutoScalerContext ctx =
TestingKubernetesAutoscalerUtils.createContext("test", null);
+ FlinkDeployment resource = (FlinkDeployment) ctx.getResource();
+
+ // Create resource with existing parallelism overrides
+ resource.getSpec()
+ .getFlinkConfiguration()
+ .put(PipelineOptions.PARALLELISM_OVERRIDES.key(), "a:1,b:2");
Review Comment:
Thanks for your clarification!
I prefer to test both of `"a:1,b:2"` and `"b:2,a:1"` for 2 reasons:
- We always use the black box testing, it means we don't care about some
detailed logic or `Data Structure` inside of test method. We just test its
feature.
- In the most of cases, the traversal order is determined when we using the
`LinkedHashMap`, and the serialization result is often `b:2,a:1`, but I don't
think the serialization result is fixed. That's means when 2 `LinkedHashMap`
are equal, but the serialization results may not be equal. And that's why we
need this PR, right?
- So I don't think our test should make an assumption: the serialization
result is fixed.
- Also, the test is better to cover more cases. (For example, the
`LinkedHashMap` is changed to `TreeMap` in the future.)
BTW, current change is fine, and this comment isn't necessary for me, so I
approved it. If you think it's not needed, feel free to merge it. Thanks~
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
