[COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

[tongxin97]

This patch checks on the removed map entry's value in three assertions, and does not wrongly assume the removed entry is C=three like the current test does.

Detailed description: https://issues.apache.org/jira/browse/COLLECTIONS-768

[coveralls]

Coverage remained the same at 90.127% when pulling 6f9704ab7d7923b61a241ec1a691a50dd3add97c on tongxin97:flaky-testEntrySet into 6c35a010ecebeec30275df1e0727dd4b29778370 on apache:master.

[tongxin97]

@kinow Thanks for taking a look. I'm under the impression that a regular LinkedHashMap or a TreeMap with custom comparator could preserve insertion order. Btw I'm new to contributing to Apache projects, so for logistics, are you (or someone else from your team) going to accept and merge this PR? Is there anything more you need from my side?

[kinow]

Hi @tongxin97

@kinow Thanks for taking a look. I'm under the impression that a regular LinkedHashMap or a TreeMap with custom comparator could preserve insertion order.

If so maybe this test could be fixed by changing the data structure given to the method you changed? e.g.

    public void testEntrySet() {
        // Sanity check
        putAndRemove(new TreeMap<>());
        // Actual test
        putAndRemove(new Flat3Map<>(new TreeMap<>()));
    }

(I tried to reproduce the failure locally, but it didn't happen after I tried ~10 times, so not 100% if that fixes it, just a quick code to show what I meant)

Btw I'm new to contributing to Apache projects, so for logistics, are you (or someone else from your team) going to accept and merge this PR? Is there anything more you need from my side?

You are doing really great! If I were confident in the change, I could merge it. Developers may leave pull requests approved without merging too. It depends on how confident a developer is to merge the change. Here, I am hoping another developer (maybe someone more familiar with Commons Collections) will take a look and chime-in with his/her point of view.

If another developer thinks this is a good fix, I'd be OK with this being merged. So you don't need to change anything for now, unless you have some other ideas on how to fix the issue :+1:

Thanks! Bruno

[tongxin97]

Hi again Bruno @kinow Thanks for the very thorough reply.

I see what you are trying to do in the code segment. Based on the Java Doc, I think like HashMap, Flat3Map itself is not supposed to preserve any order in its entry set. So under this test, Flat3Map should behave like HashMap and the current fix should be good. I hope another developer can take a look and confirm this :)

~Xin

[garydgregory]

FYI Flat3Map make no order guarantee just like Map.

[tongxin97]

@garydgregory thanks for taking a look. Do you think this is a valid fix?

[garydgregory]

@tongxin97 I do not think so.

While it is not documented, the iteration order for a Flat3Map is in fact predictable when the size <= 3, and this is in fact checked, intentionally or not by the test.

So this PR fixes the sanity check portion of test and looses the ordering test for the map we actually want to test. Therefore, the simple fix should be for the sanity check to use a LinkedHashMap instead of a HashMap, like this:

    public void testEntrySet() {
        // Sanity check
        putAndRemove(new LinkedHashMap<>());
        // Actual test
        putAndRemove(new Flat3Map<>());
    }

This is presumably the intent of the test since the putAndRemove test method uses exactly three entries which matches exactly the intended size of useful Flat3Map instances.