fixed testCreateStatefulPodWithEmptyTemplate

[rjuare8]

• Bugfix

Description

This PR fixes a flaky test in WorkloadUtilsTest.java, specifically testCreateStatefulPodWithEmptyTemplate, which was failing intermittently when run with NonDex.

Originally, the test asserted equality of the entire annotations map, including a hard-coded revision stub:

assertThat(pod.getMetadata().getAnnotations(),
    is(Map.of("extra", "annotations",
              PodRevision.STRIMZI_REVISION_ANNOTATION, "65a2d237")));

Under NonDex, the strimzi.io/revision value changed between runs even though the logical pod contents were the same, causing order-sensitive hash differences and exposing the flakiness. A representative failure looked like:

java.lang.AssertionError: 

Expected: is <{extra=annotations, strimzi.io/revision=65a2d237}>
     but: was <{extra=annotations, strimzi.io/revision=474c2be1}>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at 
io.strimzi.operator.cluster.model.WorkloadUtilsTest.testCreateStatefulPodWithEmptyTemplate(WorkloadUtilsTest.java:548)

To make the test robust while still verifying the expected behavior, this PR relaxes the assertion to check:
that the "extra" -> "annotations" pair is present, and
that the strimzi.io/revision annotation key is present (without asserting a specific hash stub).

The new assertion is:

assertThat(pod.getMetadata().getAnnotations(), allOf(
        hasEntry("extra", "annotations"),
        hasKey(PodRevision.STRIMZI_REVISION_ANNOTATION)));

This keeps the test focused on the semantic contract (that the pod carries the extra annotation and a revision annotation), while removing dependence on a specific, order-sensitive hash value. As a result, the test now passes consistently across NonDex seeds and CI runs, without any changes to production code.

Alternative deterministic approach (for discussion)

While working on this, I also investigated a more structural way to make the revision hash itself deterministic by canonicalizing the JSON representation used in PodSetUtils.podToString(Pod).

Concretely, this would involve configuring the shared ObjectMapper in PodSetUtils like:

public class PodSetUtils {
    private static final ObjectMapper MAPPER = new ObjectMapper();
    private static final TypeReference<Map<String, Object>> POD_TYPE = new TypeReference<>() { };

    static {
        // Make JSON serialization of Maps deterministic
        MAPPER.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);
    }

    // ...
}

With ORDER_MAP_ENTRIES_BY_KEYS enabled, map keys are sorted before serialization, so the JSON used to compute the revision hash becomes stable regardless of the underlying HashMap iteration order. This would, in turn, make the strimzi.io/revision annotation value itself deterministic.

However, adopting this change would alter the canonical JSON (and therefore the hash stub), which means any tests that currently assert a specific hard-coded value for PodRevision.STRIMZI_REVISION_ANNOTATION would need to be updated to the new stable hash (or relaxed, similar to the change in this PR).

For now, this PR takes the minimal, test-only approach to fix the flakiness. I’m happy to follow up with a separate PR that introduces the deterministic ObjectMapper configuration in PodSetUtils (and adjusts the relevant tests) if you think that would be valuable for the project.

• [ x] Write tests
• [ x] Make sure all tests pass

[scholzj]

It is pretty useful to keep the revision included here. So maybe the:

• The second entry should be hasEntry as well and not hasKey

Also, we should probably check the map size to get to the full extent of the original assert?

[scholzj]

Also, it might make sense to add some explanatory comment as to why it is done this way so that nobody comes later and simplifies it again.

[rjuare8]

so the reason I use hasKey rather than hasEntry is since PodRevision.STRIMZI_REVISION_ANNOTATION is not deterministic under nonDex ~ please read the specifications of this tool.

I could use hasEntry if I include the following small change to PodSetUtils as I mentioned in my PR. However, I would probably need to change PodRevision.STRIMZI_REVISION_ANNOTATION in a couple other tests. Please let me know if you are interested in this change.

public class PodSetUtils {
    private static final ObjectMapper MAPPER = new ObjectMapper();
    private static final TypeReference<Map<String, Object>> POD_TYPE = new TypeReference<>() { };

    static {
        // Make JSON serialization of Maps deterministic
        MAPPER.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);
    }

    // ...
}

[scholzj]

Testing that the value is constant is really important. If it would not be stable between runs, it would mean that it would cause production issues that are really hard to detect by other test levels (e.g. by system test) because of their randomness etc. So in my opinion, it is crucial that it remains to be part of the test.

To be honest, I'm not sure I entirely understand the point of what you are doing, the value it adds, and how it works. So I'm not sure to what extent it creates some artificial problems and to what extend this is a real bug that might happen in real environments. I assume you have muhc better understanding of that. But if it requires the improvement you suggest to the PodSetUtils to stabilize the test, then it probably should be done.

[rjuare8]

Ok I understand. Let me try to explain as best as I can.

What NonDex does here is deliberately perturb sources of nondeterminism (like HashMap iteration order) to uncover tests that accidentally depend on a specific order. Under NonDex, this test fails with:

java.lang.AssertionError: 

Expected: is <{extra=annotations, strimzi.io/revision=65a2d237}>
     but: was <{extra=annotations, strimzi.io/revision=474c2be1}>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at io.strimzi.operator.cluster.model.WorkloadUtilsTest.testCreateStatefulPodWithEmptyTemplate(WorkloadUtilsTest.java:548)

To reproduce, I ran:

mvn -pl cluster-operator \
  edu.illinois:nondex-maven-plugin:2.1.7:nondex \
  -DnondexRuns=5 \
  -Dtest=io.strimzi.operator.cluster.model.WorkloadUtilsTest#testCreateStatefulPodWithEmptyTemplate

My proposed change:

public class PodSetUtils {
    private static final ObjectMapper MAPPER = new ObjectMapper();
    private static final TypeReference<Map<String, Object>> POD_TYPE = new TypeReference<>() { };

    static {
        // Make JSON serialization of Maps deterministic
        MAPPER.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);
    }

    // ...
}

Here I use Jackson’s SerializationConfig option ORDER_MAP_ENTRIES_BY_KEYS so that map entries are always serialized in a consistent key order. That keeps the revision hash deterministic for the same logical pod, while preserving the test’s guarantee that the value is constant when the pod actually changes.

[scholzj]

That sounds reasonable. At least in this particular case, we definitely want it to be in deterministic order as well. So I think this rather than just fixing the test actually fixes potential bug in the regular code.

[rjuare8]

Just added the serialization for the PodSetUtils and updated additional tests that check the value of PodRevision.STRIMZI_REVISION_ANNOTATION. Let me know if there's additional changes neeeded.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.78%. Comparing base (bada1ec) to head (ee92005).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12204      +/-   ##
============================================
- Coverage     74.81%   74.78%   -0.03%     
+ Complexity     6630     6629       -1     
============================================
  Files           377      377              
  Lines         25360    25360              
  Branches       3402     3402              
============================================
- Hits          18972    18965       -7     
- Misses         5000     5008       +8     
+ Partials       1388     1387       -1     

see 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.