You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/10/08 18:19:41 UTC

[GitHub] [commons-collections] tongxin97 opened a new pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

tongxin97 opened a new pull request #189:
URL: https://github.com/apache/commons-collections/pull/189


   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. 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] kinow commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705865306


   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.
   
   ```java
       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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] tongxin97 commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
tongxin97 commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-739309631


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] tongxin97 commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
tongxin97 commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705861916


   @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?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] tongxin97 edited a comment on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
tongxin97 edited a comment on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705869080


   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](https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/map/Flat3Map.html), 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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] tongxin97 commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
tongxin97 commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705869080


   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](https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/map/Flat3Map.html), 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 :)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] tongxin97 commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
tongxin97 commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705861916






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] kinow commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705865306


   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.
   
   ```java
       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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] tongxin97 edited a comment on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
tongxin97 edited a comment on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705869080


   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](https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/map/Flat3Map.html), 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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] garydgregory commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-739306828


   FYI `Flat3Map` make no order guarantee just like `Map`.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] coveralls commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705772002


   
   [![Coverage Status](https://coveralls.io/builds/34037267/badge)](https://coveralls.io/builds/34037267)
   
   Coverage remained the same at 90.127% when pulling **6f9704ab7d7923b61a241ec1a691a50dd3add97c on tongxin97:flaky-testEntrySet** into **6c35a010ecebeec30275df1e0727dd4b29778370 on apache:master**.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] coveralls commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-705772002


   
   [![Coverage Status](https://coveralls.io/builds/34037267/badge)](https://coveralls.io/builds/34037267)
   
   Coverage remained the same at 90.127% when pulling **6f9704ab7d7923b61a241ec1a691a50dd3add97c on tongxin97:flaky-testEntrySet** into **6c35a010ecebeec30275df1e0727dd4b29778370 on apache:master**.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-collections] garydgregory commented on pull request #189: [COLLECTIONS-768] Fix flaky Flat3MapTest.testEntrySet()

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #189:
URL: https://github.com/apache/commons-collections/pull/189#issuecomment-739312957


   @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.
   
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org