GEODE-3469: prevent zero pid from AvailablePid for tests This closes #724
Project: http://git-wip-us.apache.org/repos/asf/geode/repo Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/28616a27 Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/28616a27 Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/28616a27 Branch: refs/heads/feature/GEODE-3416 Commit: 28616a27ec79c0693fb2fbb1a135cf3df3ad4150 Parents: c4def6b Author: Kirk Lund <[email protected]> Authored: Fri Aug 18 14:33:25 2017 -0700 Committer: Kirk Lund <[email protected]> Committed: Mon Aug 21 14:59:26 2017 -0700 ---------------------------------------------------------------------- .../internal/process/lang/AvailablePid.java | 119 ++++++++++++++++--- .../internal/process/lang/AvailablePidTest.java | 81 +++++++++++-- 2 files changed, 170 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/geode/blob/28616a27/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePid.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePid.java b/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePid.java index d26f73e..c74bd10 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePid.java +++ b/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePid.java @@ -12,55 +12,107 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ - package org.apache.geode.internal.process.lang; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.apache.geode.internal.process.ProcessUtils.isProcessAlive; import java.util.Arrays; import java.util.HashSet; import java.util.Random; import java.util.Set; +import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; -import org.apache.geode.internal.util.StopWatch; +import com.google.common.base.Stopwatch; /** * Finds unused pids for use in tests. */ public class AvailablePid { - static final int LOWER_BOUND = 1; - static final int UPPER_BOUND = 64000; + static final int DEFAULT_LOWER_BOUND = 1; + static final int DEFAULT_UPPER_BOUND = 64000; static final int DEFAULT_TIMEOUT_MILLIS = 60 * 1000; + private final int lowerBound; + private final int upperBound; private final Random random; private final int timeoutMillis; /** - * Construct with no seed and default timeout of 1 minute. + * Construct with: + * <ul> + * <li>default {@link Bounds} of {@link #DEFAULT_LOWER_BOUND} (inclusive) and + * {@link #DEFAULT_UPPER_BOUND} (inclusive) + * <li>Random with no see + * <li>default timeout of 1 minute. + * </ul> */ public AvailablePid() { - this(new Random(), DEFAULT_TIMEOUT_MILLIS); + this(new Bounds(DEFAULT_LOWER_BOUND, DEFAULT_UPPER_BOUND), new Random(), + DEFAULT_TIMEOUT_MILLIS); + } + + /** + * Construct with: + * <ul> + * <li>default {@link Bounds} of {@link #DEFAULT_LOWER_BOUND} (inclusive) and + * {@link #DEFAULT_UPPER_BOUND} (inclusive) + * <li>Random with specified seed + * <li>default timeout of 1 minute + * </ul> + */ + public AvailablePid(final long seed) { + this(new Bounds(DEFAULT_LOWER_BOUND, DEFAULT_UPPER_BOUND), new Random(seed), + DEFAULT_TIMEOUT_MILLIS); } /** - * Construct with specified seed and timeout. + * Construct with: + * <ul> + * <li>default {@link Bounds} of {@link #DEFAULT_LOWER_BOUND} (inclusive) and + * {@link #DEFAULT_UPPER_BOUND} (inclusive) + * <li>specified Random instance + * <li>default timeout of 1 minute + * </ul> */ - public AvailablePid(final long seed, final int timeoutMillis) { - this(new Random(seed), timeoutMillis); + public AvailablePid(final Random random) { + this(new Bounds(DEFAULT_LOWER_BOUND, DEFAULT_UPPER_BOUND), random, DEFAULT_TIMEOUT_MILLIS); } /** - * Construct with specified Random implementation. + * Construct with: + * <ul> + * <li>specified {@link Bounds} of {@code lowerBound} (inclusive) and {@code upperBound} + * (inclusive) + * <li>Random with no seed + * <li>default timeout of 1 minute + * </ul> */ - public AvailablePid(final Random random, final int timeoutMillis) { + public AvailablePid(final Bounds bounds) { + this(bounds, new Random(), DEFAULT_TIMEOUT_MILLIS); + } + + /** + * Construct with: + * <ul> + * <li>specified {@link Bounds} of {@code lowerBound} (inclusive) and {@code upperBound} + * (inclusive) + * <li>specified Random instance + * <li>specified default timeout millis + * </ul> + */ + public AvailablePid(final Bounds bounds, final Random random, final int timeoutMillis) { + this.lowerBound = bounds.lowerBound; + this.upperBound = bounds.upperBound; this.random = random; this.timeoutMillis = timeoutMillis; } /** - * Returns specified pid if it's unused. Else returns randomly unused pid. + * Returns specified pid if it's unused. Else returns randomly unused pid between + * {@code lowerBound} (inclusive) and {@code upperBound} (inclusive). */ public int findAvailablePid(final int pid) throws TimeoutException { if (isProcessAlive(pid)) { @@ -70,13 +122,14 @@ public class AvailablePid { } /** - * Returns randomly unused pid. + * Returns randomly unused pid between {@code lowerBound} (inclusive) and {@code upperBound} + * (inclusive). */ public int findAvailablePid() throws TimeoutException { - StopWatch stopWatch = new StopWatch(true); + Stopwatch stopwatch = Stopwatch.createStarted(); int pid = random(); while (isProcessAlive(pid)) { - if (stopWatch.elapsedTimeMillis() > timeoutMillis) { + if (stopwatch.elapsed(MILLISECONDS) > timeoutMillis) { throw new TimeoutException( "Failed to find available pid within " + timeoutMillis + " millis."); } @@ -86,7 +139,8 @@ public class AvailablePid { } /** - * Returns specified number of randomly unused pids. + * Returns specified number of unique, randomly unused pids between {@code lowerBound} (inclusive) + * and {@code upperBound} (inclusive). */ public int[] findAvailablePids(final int number) throws TimeoutException { Set<Integer> pids = new HashSet<>(); @@ -99,8 +153,37 @@ public class AvailablePid { return Arrays.stream(pids.toArray(new Integer[0])).mapToInt(Integer::intValue).toArray(); } - private int random() { - return random.nextInt(UPPER_BOUND - LOWER_BOUND); + /** + * Returns a value between {@code lowerBound} (inclusive) and {@code upperBound} (inclusive) + */ + int random() { + return random.nextInt(upperBound + 1 - lowerBound) + lowerBound; + } + + /** + * Lower and upper bounds for desired PIDs. Both are inclusive -- if you specify + * {@code new Bounds(1, 100)} then {@code AvailablePid} will return values of 1 through 100. + * + * <ul> + * <li>{@code lowerBound} must be an integer greater than zero. + * <li>{@code upperBound} must be an integer greater than {@code lowerBound}. + * </ul> + */ + static class Bounds { + final int lowerBound; + final int upperBound; + + Bounds(final int lowerBound, final int upperBound) { + if (lowerBound < 1) { + throw new IllegalArgumentException("lowerBound must be greater than '0'"); + } + if (upperBound <= lowerBound) { + throw new IllegalArgumentException( + "upperBound must be greater than lowerBound '" + lowerBound + "'"); + } + this.lowerBound = lowerBound; + this.upperBound = upperBound; + } } } http://git-wip-us.apache.org/repos/asf/geode/blob/28616a27/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java b/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java index 00beb67..48ba235 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java @@ -12,13 +12,16 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ - package org.apache.geode.internal.process.lang; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.apache.geode.internal.process.ProcessUtils.identifyPid; import static org.apache.geode.internal.process.ProcessUtils.isProcessAlive; -import static org.apache.geode.internal.process.lang.AvailablePid.DEFAULT_TIMEOUT_MILLIS; +import static org.apache.geode.internal.process.lang.AvailablePid.DEFAULT_LOWER_BOUND; +import static org.apache.geode.internal.process.lang.AvailablePid.DEFAULT_UPPER_BOUND; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.awaitility.Awaitility.await; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.spy; @@ -29,9 +32,12 @@ import java.util.Random; import java.util.Set; import java.util.stream.Collectors; +import com.google.common.base.Stopwatch; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.junit.rules.Timeout; import org.apache.geode.test.junit.categories.UnitTest; @@ -43,6 +49,9 @@ public class AvailablePidTest { private AvailablePid availablePid; + @Rule + public Timeout timeout = Timeout.builder().withTimeout(20, SECONDS).build(); + @Before public void before() throws Exception { availablePid = new AvailablePid(); @@ -50,39 +59,39 @@ public class AvailablePidTest { @Test public void lowerBoundShouldBeLegalPid() throws Exception { - assertThat(isProcessAlive(AvailablePid.LOWER_BOUND)).isIn(true, false); + assertThat(isProcessAlive(DEFAULT_LOWER_BOUND)).isIn(true, false); } @Test public void upperBoundShouldBeLegalPid() throws Exception { - assertThat(isProcessAlive(AvailablePid.UPPER_BOUND)).isIn(true, false); + assertThat(isProcessAlive(DEFAULT_UPPER_BOUND)).isIn(true, false); } - @Test(timeout = DEFAULT_TIMEOUT_MILLIS) + @Test public void findAvailablePidShouldNotReturnLocalPid() throws Exception { int pid = availablePid.findAvailablePid(); assertThat(pid).isNotEqualTo(identifyPid()); } - @Test(timeout = DEFAULT_TIMEOUT_MILLIS) + @Test public void findAvailablePidShouldNotReturnLivePid() throws Exception { int pid = availablePid.findAvailablePid(); assertThat(isProcessAlive(pid)).isFalse(); } - @Test(timeout = DEFAULT_TIMEOUT_MILLIS) - public void findAvailablePidShouldReturnRandomPid() throws Exception { + @Test + public void findAvailablePidShouldUseRandom() throws Exception { Random random = spy(new Random()); - availablePid = new AvailablePid(random, DEFAULT_TIMEOUT_MILLIS); + availablePid = new AvailablePid(random); availablePid.findAvailablePid(); verify(random, atLeastOnce()).nextInt(anyInt()); } - @Test(timeout = DEFAULT_TIMEOUT_MILLIS) + @Test public void findAvailablePidsShouldReturnSpecifiedNumberOfPids() throws Exception { assertThat(availablePid.findAvailablePids(1)).hasSize(1); assertThat(availablePid.findAvailablePids(2)).hasSize(2); @@ -91,7 +100,7 @@ public class AvailablePidTest { assertThat(availablePid.findAvailablePids(8)).hasSize(8); } - @Test(timeout = DEFAULT_TIMEOUT_MILLIS) + @Test public void findAvailablePidsShouldReturnNoDuplicatedPids() throws Exception { assertThatNoPidIsDuplicated(availablePid.findAvailablePids(1)); assertThatNoPidIsDuplicated(availablePid.findAvailablePids(2)); @@ -100,7 +109,55 @@ public class AvailablePidTest { assertThatNoPidIsDuplicated(availablePid.findAvailablePids(8)); } - private void assertThatNoPidIsDuplicated(int[] pids) { + @Test + public void findAvailablePidShouldReturnGreaterThanOrEqualToLowerBound() throws Exception { + availablePid = new AvailablePid(new AvailablePid.Bounds(1, 10)); + Stopwatch stopwatch = Stopwatch.createStarted(); + + do { + assertThat(availablePid.findAvailablePid()).isGreaterThanOrEqualTo(1); + } while (stopwatch.elapsed(SECONDS) < 2); + } + + @Test + public void findAvailablePidShouldReturnLessThanOrEqualToUpperBound() throws Exception { + availablePid = new AvailablePid(new AvailablePid.Bounds(1, 10)); + Stopwatch stopwatch = Stopwatch.createStarted(); + + do { + assertThat(availablePid.findAvailablePid()).isLessThanOrEqualTo(10); + } while (stopwatch.elapsed(SECONDS) < 2); + } + + @Test + public void randomLowerBoundIsInclusive() throws Exception { + availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3)); + + await().atMost(10, SECONDS).until(() -> assertThat(availablePid.random()).isEqualTo(1)); + } + + @Test + public void randomUpperBoundIsInclusive() throws Exception { + availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3)); + + await().atMost(10, SECONDS).until(() -> assertThat(availablePid.random()).isEqualTo(3)); + } + + @Test + public void lowerBoundMustBeGreaterThanZero() throws Exception { + assertThatThrownBy(() -> new AvailablePid(new AvailablePid.Bounds(0, 1))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("lowerBound must be greater than '0'"); + } + + @Test + public void upperBoundMustBeGreaterThanLowerBound() throws Exception { + assertThatThrownBy(() -> new AvailablePid(new AvailablePid.Bounds(1, 1))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("upperBound must be greater than lowerBound '1'"); + } + + private void assertThatNoPidIsDuplicated(final int[] pids) { Set<Integer> pidSet = Arrays.stream(pids).boxed().collect(Collectors.toSet()); assertThat(pidSet).hasSize(pids.length); }
