Skip to content

Commit

Permalink
Fix improper negative key generation and update assertions in tests (#…
Browse files Browse the repository at this point in the history 
…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]>
  • Loading branch information
ZhangJian He authored May 7, 2024
1 parent 1ae4be0 commit c4fa8c2
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 69 deletions.
53 changes: 25 additions & 28 deletions ...erver/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashSetTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -188,10 +189,8 @@ public void concurrentInsertions() throws Throwable {
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);

Expand Down Expand Up @@ -222,10 +221,8 @@ public void concurrentInsertionsAndReads() throws Throwable {
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);

Expand All @@ -251,15 +248,15 @@ public void testClear() {
.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
Expand All @@ -270,31 +267,31 @@ public void testExpandAndShrink() {
.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
Expand Down Expand Up @@ -354,7 +351,7 @@ public void testConcurrentExpandAndShrinkAndGet() throws Throwable {
});

future.get();
assertTrue(ex.get() == null);
assertNull(ex.get());
// shut down pool
executor.shutdown();
}
Expand All @@ -368,29 +365,29 @@ public void testExpandShrinkAndClear() {
.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
Expand Down
79 changes: 38 additions & 41 deletions ...r/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongHashMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,27 @@
*/
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;
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.
Expand Down Expand Up @@ -117,15 +118,15 @@ public void testClear() {
.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
Expand All @@ -136,29 +137,29 @@ public void testExpandAndShrink() {
.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
Expand Down Expand Up @@ -217,7 +218,7 @@ public void testConcurrentExpandAndShrinkAndGet() throws Throwable {
});

future.get();
assertTrue(ex.get() == null);
assertNull(ex.get());
// shut down pool
executor.shutdown();
}
Expand All @@ -231,28 +232,28 @@ public void testExpandShrinkAndClear() {
.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
Expand Down Expand Up @@ -346,10 +347,8 @@ public void concurrentInsertions() throws Throwable {
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);

Expand Down Expand Up @@ -381,10 +380,8 @@ public void concurrentInsertionsAndReads() throws Throwable {
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);

Expand Down Expand Up @@ -531,10 +528,10 @@ public void testAddAndGet() {
.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);
Expand Down

0 comments on commit c4fa8c2

Please sign in to comment.