This is an automated email from the ASF dual-hosted git repository.
shoothzj pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new c4fa8c2013 Fix improper negative key generation and update assertions
in tests (#4332)
c4fa8c2013 is described below
commit c4fa8c20131fb5e52b7448ee6ee1491bf9e870fa
Author: ZhangJian He <[email protected]>
AuthorDate: Tue May 7 19:33:26 2024 +0800
Fix improper negative key generation and update assertions in tests (#4332)
Fix #2077
By the way, I have been tested, negative number will fail.
### Motivation
In the existing tests for `ConcurrentLongHashSet` and
`ConcurrentLongLongHashMap`, the method `Math.abs(random.nextLong())` is used
to generate keys. However, `Math.abs` can return a negative number if
`Long.MIN_VALUE` is generated, which is problematic for tests that assume
non-negative keys. Additionally, the assertions in the tests are using the
older JUnit 4 style, which is less readable and not as flexible as JUnit 5.
### Changes
1. **Key Generation Fix**: Replaced `Math.abs(random.nextLong())` with
`ThreadLocalRandom.current().nextLong(Long.MAX_VALUE)` to ensure that all
generated keys are non-negative.
2. **Assertion Updates**: Updated all assertions in the
`ConcurrentLongHashSetTest` and `ConcurrentLongLongHashMapTest` to use JUnit 5
style. This includes using `assertEquals`, `assertTrue`, `assertFalse`, and
`assertNull` for better readability and consistency.
Signed-off-by: ZhangJian He <[email protected]>
---
.../collections/ConcurrentLongHashSetTest.java | 53 +++++++--------
.../collections/ConcurrentLongLongHashMapTest.java | 79 +++++++++++-----------
2 files changed, 63 insertions(+), 69 deletions(-)
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashSetTest.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashSetTest.java
index 1c6bf12c69..aa308d9447 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashSetTest.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashSetTest.java
@@ -20,23 +20,24 @@
*/
package org.apache.bookkeeper.util.collections;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
-import java.util.Random;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.atomic.AtomicReference;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
/**
* Test the ConcurrentLongHashSet class.
@@ -188,10 +189,8 @@ public class ConcurrentLongHashSetTest {
final int threadIdx = i;
futures.add(executor.submit(() -> {
- Random random = new Random();
-
for (int j = 0; j < n; j++) {
- long key = Math.abs(random.nextLong());
+ long key =
ThreadLocalRandom.current().nextLong(Long.MAX_VALUE);
// Ensure keys are unique
key -= key % (threadIdx + 1);
@@ -222,10 +221,8 @@ public class ConcurrentLongHashSetTest {
final int threadIdx = i;
futures.add(executor.submit(() -> {
- Random random = new Random();
-
for (int j = 0; j < n; j++) {
- long key = Math.abs(random.nextLong());
+ long key =
ThreadLocalRandom.current().nextLong(Long.MAX_VALUE);
// Ensure keys are unique
key -= key % (threadIdx + 1);
@@ -251,15 +248,15 @@ public class ConcurrentLongHashSetTest {
.autoShrink(true)
.mapIdleFactor(0.25f)
.build();
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
assertTrue(map.add(1));
assertTrue(map.add(2));
assertTrue(map.add(3));
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
map.clear();
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
}
@Test
@@ -270,31 +267,31 @@ public class ConcurrentLongHashSetTest {
.autoShrink(true)
.mapIdleFactor(0.25f)
.build();
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
assertTrue(map.add(1));
assertTrue(map.add(2));
assertTrue(map.add(3));
// expand hashmap
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(1));
// not shrink
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(2));
// shrink hashmap
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
// expand hashmap
assertTrue(map.add(4));
assertTrue(map.add(5));
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
//verify that the map does not keep shrinking at every remove()
operation
assertTrue(map.add(6));
assertTrue(map.remove(6));
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
}
@Test
@@ -354,7 +351,7 @@ public class ConcurrentLongHashSetTest {
});
future.get();
- assertTrue(ex.get() == null);
+ assertNull(ex.get());
// shut down pool
executor.shutdown();
}
@@ -368,29 +365,29 @@ public class ConcurrentLongHashSetTest {
.mapIdleFactor(0.25f)
.build();
final long initCapacity = map.capacity();
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
assertTrue(map.add(1));
assertTrue(map.add(2));
assertTrue(map.add(3));
// expand hashmap
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(1));
// not shrink
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(2));
// shrink hashmap
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
assertTrue(map.remove(3));
// Will not shrink the hashmap again because shrink capacity is less
than initCapacity
// current capacity is equal than the initial capacity
- assertTrue(map.capacity() == initCapacity);
+ assertEquals(map.capacity(), initCapacity);
map.clear();
// after clear, because current capacity is equal than the initial
capacity, so not shrinkToInitCapacity
- assertTrue(map.capacity() == initCapacity);
+ assertEquals(map.capacity(), initCapacity);
}
@Test
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongHashMapTest.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongHashMapTest.java
index 8121e3364c..b9a67e17d1 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongHashMapTest.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongHashMapTest.java
@@ -20,10 +20,11 @@
*/
package org.apache.bookkeeper.util.collections;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
@@ -31,15 +32,15 @@ import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
-import java.util.Random;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
import
org.apache.bookkeeper.util.collections.ConcurrentLongLongHashMap.LongLongFunction;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
/**
* Test the ConcurrentLongLongHashMap class.
@@ -117,15 +118,15 @@ public class ConcurrentLongLongHashMapTest {
.autoShrink(true)
.mapIdleFactor(0.25f)
.build();
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
- assertTrue(map.put(1, 1) == -1);
- assertTrue(map.put(2, 2) == -1);
- assertTrue(map.put(3, 3) == -1);
+ assertEquals(-1, map.put(1, 1));
+ assertEquals(-1, map.put(2, 2));
+ assertEquals(-1, map.put(3, 3));
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
map.clear();
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
}
@Test
@@ -136,29 +137,29 @@ public class ConcurrentLongLongHashMapTest {
.autoShrink(true)
.mapIdleFactor(0.25f)
.build();
- assertTrue(map.put(1, 1) == -1);
- assertTrue(map.put(2, 2) == -1);
- assertTrue(map.put(3, 3) == -1);
+ assertEquals(-1, map.put(1, 1));
+ assertEquals(-1, map.put(2, 2));
+ assertEquals(-1, map.put(3, 3));
// expand hashmap
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(1, 1));
// not shrink
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(2, 2));
// shrink hashmap
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
// expand hashmap
- assertTrue(map.put(4, 4) == -1);
- assertTrue(map.put(5, 5) == -1);
- assertTrue(map.capacity() == 8);
+ assertEquals(-1, map.put(4, 4));
+ assertEquals(-1, map.put(5, 5));
+ assertEquals(8, map.capacity());
//verify that the map does not keep shrinking at every remove()
operation
- assertTrue(map.put(6, 6) == -1);
+ assertEquals(-1, map.put(6, 6));
assertTrue(map.remove(6, 6));
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
}
@Test
@@ -217,7 +218,7 @@ public class ConcurrentLongLongHashMapTest {
});
future.get();
- assertTrue(ex.get() == null);
+ assertNull(ex.get());
// shut down pool
executor.shutdown();
}
@@ -231,28 +232,28 @@ public class ConcurrentLongLongHashMapTest {
.mapIdleFactor(0.25f)
.build();
final long initCapacity = map.capacity();
- assertTrue(map.capacity() == 4);
- assertTrue(map.put(1, 1) == -1);
- assertTrue(map.put(2, 2) == -1);
- assertTrue(map.put(3, 3) == -1);
+ assertEquals(4, map.capacity());
+ assertEquals(-1, map.put(1, 1));
+ assertEquals(-1, map.put(2, 2));
+ assertEquals(-1, map.put(3, 3));
// expand hashmap
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(1, 1));
// not shrink
- assertTrue(map.capacity() == 8);
+ assertEquals(8, map.capacity());
assertTrue(map.remove(2, 2));
// shrink hashmap
- assertTrue(map.capacity() == 4);
+ assertEquals(4, map.capacity());
assertTrue(map.remove(3, 3));
// Will not shrink the hashmap again because shrink capacity is less
than initCapacity
// current capacity is equal than the initial capacity
- assertTrue(map.capacity() == initCapacity);
+ assertEquals(map.capacity(), initCapacity);
map.clear();
// after clear, because current capacity is equal than the initial
capacity, so not shrinkToInitCapacity
- assertTrue(map.capacity() == initCapacity);
+ assertEquals(map.capacity(), initCapacity);
}
@Test
@@ -346,10 +347,8 @@ public class ConcurrentLongLongHashMapTest {
final int threadIdx = i;
futures.add(executor.submit(() -> {
- Random random = new Random();
-
for (int j = 0; j < n; j++) {
- long key = Math.abs(random.nextLong());
+ long key =
ThreadLocalRandom.current().nextLong(Long.MAX_VALUE);
// Ensure keys are uniques
key -= key % (threadIdx + 1);
@@ -381,10 +380,8 @@ public class ConcurrentLongLongHashMapTest {
final int threadIdx = i;
futures.add(executor.submit(() -> {
- Random random = new Random();
-
for (int j = 0; j < n; j++) {
- long key = Math.abs(random.nextLong());
+ long key =
ThreadLocalRandom.current().nextLong(Long.MAX_VALUE);
// Ensure keys are uniques
key -= key % (threadIdx + 1);
@@ -531,10 +528,10 @@ public class ConcurrentLongLongHashMapTest {
.build();
assertEquals(map.addAndGet(0, 0), 0);
- assertEquals(map.containsKey(0), true);
+ assertTrue(map.containsKey(0));
assertEquals(map.get(0), 0);
- assertEquals(map.containsKey(5), false);
+ assertFalse(map.containsKey(5));
assertEquals(map.addAndGet(0, 5), 5);
assertEquals(map.get(0), 5);
