Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
cla commented on PR #669: URL: https://github.com/apache/commons-collections/pull/669#issuecomment-3694138681 Hi @garydgregory I made the changes as you suggested. Could you please review the latest changes? -- 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]
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
cla commented on code in PR #669:
URL:
https://github.com/apache/commons-collections/pull/669#discussion_r2649260248
##
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##
@@ -118,6 +123,47 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+@Test
+void testGetValuesAsBagIsSafeCopy() {
Review Comment:
I added the test case using EasyMock, consistent with the project's existing
dependencies.
--
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]
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
cla commented on code in PR #669:
URL:
https://github.com/apache/commons-collections/pull/669#discussion_r2649258987
##
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##
@@ -105,7 +110,7 @@ void testGetValuesAsList() {
@Test
void testGetValuesAsSet() {
-assertNull(MultiMapUtils.getValuesAsList(null, "key1"));
+assertNull(MultiMapUtils.getValuesAsSet(null, "key1"));
Review Comment:
I corrected a typo.
--
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]
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
garydgregory commented on PR #669: URL: https://github.com/apache/commons-collections/pull/669#issuecomment-3694094817 Hi @cla WRT bag you have 2 options: - add a concrete implementation in the test sources - mocking as you mentioned -- 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]
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
cla commented on PR #669: URL: https://github.com/apache/commons-collections/pull/669#issuecomment-3694087301 @garydgregory Thank you for the feedback! I reviewed my test cases and found the issues. I updated the test for `getValuesAsSet` accordingly. However, I couldn't find any `MultiValuedMap`implementation whose `get(key)`returns a `Bag`(Only Collections/List/Set). So I'm not sure how to write a failing test case. If it's okay to make test case with Mockito I'll make it! -- 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]
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
garydgregory commented on code in PR #669:
URL:
https://github.com/apache/commons-collections/pull/669#discussion_r2649128105
##
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##
@@ -118,6 +118,50 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+@Test
+void testGetValuesAsBagIsSafeCopy() {
+final String[] values = { "v1", "v2", "v3" };
+final MultiValuedMap map = new
ArrayListValuedHashMap<>();
+for (final String val : values) {
+map.put("key1", val);
+map.put("key1", val);
+}
+
+final Bag bag = MultiMapUtils.getValuesAsBag(map, "key1");
+bag.add("v4");
+assertFalse(map.containsMapping("key1", "v4"));
+assertEquals(6, MultiMapUtils.getValuesAsBag(map, "key1").size());
+}
+
+@Test
+void testGetValuesAsListIsSafeCopy() {
+final String[] values = { "v1", "v2", "v3" };
+final MultiValuedMap map = new
ArrayListValuedHashMap<>();
+for (final String val : values) {
+map.put("key1", val);
+}
+
+final List list = MultiMapUtils.getValuesAsList(map, "key1");
+list.add("v4");
+assertFalse(map.containsMapping("key1", "v4"));
+assertEquals(Arrays.asList(values), MultiMapUtils.getValuesAsList(map,
"key1"));
+}
+
+@Test
+void testGetValuesAsSetIsSafeCopy() {
Review Comment:
This test doesn't prove anything about the changes in `main` because it
passes without changes to `main`.
##
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##
@@ -118,6 +118,50 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+@Test
+void testGetValuesAsBagIsSafeCopy() {
+final String[] values = { "v1", "v2", "v3" };
+final MultiValuedMap map = new
ArrayListValuedHashMap<>();
+for (final String val : values) {
+map.put("key1", val);
+map.put("key1", val);
+}
+
+final Bag bag = MultiMapUtils.getValuesAsBag(map, "key1");
+bag.add("v4");
+assertFalse(map.containsMapping("key1", "v4"));
+assertEquals(6, MultiMapUtils.getValuesAsBag(map, "key1").size());
+}
+
+@Test
+void testGetValuesAsListIsSafeCopy() {
Review Comment:
OTOH, this test _does_ prove the changes in `main` matter because it fails
without changes to `main`.
##
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##
@@ -118,6 +118,50 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+@Test
+void testGetValuesAsBagIsSafeCopy() {
Review Comment:
This test doesn't prove anything about the changes in `main` because it
passes without changes to `main`.
--
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]
