diff --git a/src/main/java/org/kiwiproject/test/curator/CuratorTestingServerExtension.java b/src/main/java/org/kiwiproject/test/curator/CuratorTestingServerExtension.java index 45c70cd3..4e56dbfa 100644 --- a/src/main/java/org/kiwiproject/test/curator/CuratorTestingServerExtension.java +++ b/src/main/java/org/kiwiproject/test/curator/CuratorTestingServerExtension.java @@ -1,6 +1,7 @@ package org.kiwiproject.test.curator; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -39,13 +40,20 @@ * state is STOPPED, which will be the case if there is more than one nested test class, and in that case we must * re-initialize the testing server and CuratorFramework. I am not sure if this is the "correct" way to do this in * Jupiter, but it works and doesn't seem like it will hurt anything. + *

+ * If you need the Curator {@link TestingServer} to start immediately instead of waiting for the normal + * JUnit Jupiter lifecycle, you can use one of the {@link #newStartingImmediately()} factory methods. You might need + * to start immediately in situations where there are other components (e.g. other extensions or code in a test) + * that relies on a ZooKeeper server to be running when instantiated. When starting immediately, this extension + * should be the first one registered, so that it starts the testing server before other code attempts to connect to + * it. Also note we consider starting the testing server immediately to be the exception, not the general case, and + * should generally be considered as a last resort. */ @Slf4j public class CuratorTestingServerExtension implements BeforeAllCallback, AfterAllCallback { private static final LocalPortChecker PORT_CHECKER = new LocalPortChecker(); - private static final boolean START_SERVER = false; private static final int SESSION_TIMEOUT_MS = 5_000; private static final int CONNECTION_TIMEOUT_MS = 1_000; private static final int SLEEP_BETWEEN_RETRY_MS = 500; @@ -69,7 +77,43 @@ public CuratorTestingServerExtension() { * @param port the port to use */ public CuratorTestingServerExtension(int port) { - initialize(port); + this(port, false); + } + + /** + * Create a new extension that uses the first open port above 1024 that it finds, and which starts the Curator + * {@link TestingServer} immediately (before the normal JUnit Jupiter lifecycle). + * + * @return a new extension instance + * @see #newStartingImmediately(int) + */ + public static CuratorTestingServerExtension newStartingImmediately() { + var port = findOpenPortOrThrow(); + return newStartingImmediately(port); + } + + /** + * Create a new extension using the given port and specifying whether to start the {@link TestingServer} + * immediately (before the normal JUnit Jupiter lifecycle). + * + * @param port the port to use + * @return a new extension instance + */ + public static CuratorTestingServerExtension newStartingImmediately(int port) { + return new CuratorTestingServerExtension(port, true); + } + + /** + * Create a new extension using the given port and specifying whether to start the {@link TestingServer} + * immediately. + * + * @param port the port to use + * @param shouldStartImmediately use {@code true} to start the testing server immediately, or {@code false} to start + * during the normal JUnit Jupiter lifecycle (which will be in the + * {@link org.junit.jupiter.api.BeforeAll} callback method) + */ + private CuratorTestingServerExtension(int port, boolean shouldStartImmediately) { + initialize(port, shouldStartImmediately); } @Override @@ -85,7 +129,7 @@ public void beforeAll(ExtensionContext context) throws Exception { LOG.trace("[beforeAll: {}] Re-initialize since client is STOPPED. There is probably more than one @Nested test class.", displayName); var newPort = findOpenPortOrThrow(); - initialize(newPort); + initialize(newPort, false); } LOG.trace("[beforeAll: {}] Starting TestingServer and CuratorFramework client", displayName); @@ -99,13 +143,14 @@ public void beforeAll(ExtensionContext context) throws Exception { testingServer.getTempDirectory()); } - private void initialize(int port) { + private void initialize(int port, boolean startServer) { checkArgument(port >= 0 && port <= 0xFFFF, "Invalid port: %s", port); - LOG.trace("Using {} as testing server port", port); + LOG.trace("Using {} as testing server port. Starting server now? {}", port, startServer); try { - testingServer = new TestingServer(port, START_SERVER); + checkPortIsAvailable(port); + testingServer = new TestingServer(port, startServer); } catch (Exception e) { throw new CuratorTestingServerException("Error creating testing server on port " + port, e); } @@ -118,6 +163,10 @@ private void initialize(int port) { ); } + private void checkPortIsAvailable(int port) { + checkState(new LocalPortChecker().isPortAvailable(port), "port %s is not available", port); + } + private static int findOpenPortOrThrow() { return PORT_CHECKER.findFirstOpenPortAbove(1024).orElseThrow(IllegalStateException::new); } diff --git a/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionImmediateStartTest.java b/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionImmediateStartTest.java new file mode 100644 index 00000000..d77e38bb --- /dev/null +++ b/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionImmediateStartTest.java @@ -0,0 +1,128 @@ +package org.kiwiproject.test.curator; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; + +import lombok.Getter; +import lombok.extern.slf4j.Slf4j; +import org.apache.curator.framework.CuratorFramework; +import org.apache.curator.framework.CuratorFrameworkFactory; +import org.apache.curator.framework.imps.CuratorFrameworkState; +import org.apache.curator.retry.RetryOneTime; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.util.concurrent.TimeUnit; + +/** + * This tests the case when a test has requested the Curator {@link org.apache.curator.test.TestingServer} to start + * immediately, instead of waiting for the @{@link org.junit.jupiter.api.extension.BeforeAllCallback} during + * the normal JUnit Jupiter lifecycle. To do this, it uses a custom Jupiter extension which attempts to connect to + * the testing ZooKeeper server. This test also contains a nested test class to ensure everything works regardless + * of the test structure. + */ +@DisplayName("CuratorTestingServerExtension with immediate start") +@Slf4j +class CuratorTestingServerExtensionImmediateStartTest { + + @RegisterExtension + static final CuratorTestingServerExtension ZK_TEST_SERVER = CuratorTestingServerExtension.newStartingImmediately(); + + /** + * Simulates another extension that requires a ZooKeeper server. Since {@link org.apache.curator.test.TestingServer} + * doesn't expose any way to check its state (i.e. is it started or not), this gets the {@link CuratorFramework} + * client from the {@link CuratorTestingServerExtension} and attempts to start it, blocking until connected (with + * a timeout to ensure it stops if it cannot connect). This is an indirect way of determining whether the testing + * server has started. + */ + @Slf4j + static class ZooKeeperUsingExtension implements BeforeAllCallback { + + @Getter + private boolean testingServerStarted; + + /** + * Creates a new Curator client and attempts to connect to the testing server. + */ + @Override + public void beforeAll(ExtensionContext extensionContext) throws Exception { + var sessionTimeoutMs = 2_000; + var connectionTimeoutMs = 1_000; + var sleepMsBetweenRetry = 500; + var retryPolicy = new RetryOneTime(sleepMsBetweenRetry); + + try (var client = CuratorFrameworkFactory.newClient(ZK_TEST_SERVER.getConnectString(), + sessionTimeoutMs, + connectionTimeoutMs, + retryPolicy)) { + client.start(); + testingServerStarted = client.blockUntilConnected(1, TimeUnit.SECONDS); + } + } + } + + @RegisterExtension + static final ZooKeeperUsingExtension ZOOKEEPER_USING_EXTENSION = new ZooKeeperUsingExtension(); + + private CuratorFramework client; + + @BeforeEach + void setUp(TestInfo testInfo) { + LOG.trace("Test: {}", testInfo.getDisplayName()); + + client = ZK_TEST_SERVER.getClient(); + } + + @Test + void shouldHaveStartedTestingServerImmediately() { + assertThat(ZOOKEEPER_USING_EXTENSION.isTestingServerStarted()) + .describedAs("The TestingServer should have started immediately (when constructed) but did not") + .isTrue(); + } + + @ParameterizedTest + @ValueSource(ints = {-42, -1, 65_536, 65_537, 90_000}) + void shouldRejectInvalidPorts(int value) { + assertThatIllegalArgumentException() + .isThrownBy(() -> new CuratorTestingServerExtension(value)) + .withMessage("Invalid port: " + value); + } + + @Test + void shouldHavePort() { + assertThat(ZK_TEST_SERVER.getPort()).isPositive(); + } + + @Test + void shouldHaveClient() { + assertThat(client.getState()).isEqualTo(CuratorFrameworkState.STARTED); + } + + @Nested + class CanUseClient { + + @Test + void toCheckExistenceOfPath() throws Exception { + var stat = client.checkExists().forPath("/foo"); + assertThat(stat).isNull(); + } + + @Test + void toCreateZnode() throws Exception { + var path = "/bar/baz"; + var createdPath = client.create().creatingParentContainersIfNeeded().forPath(path); + assertThat(createdPath).isEqualTo(path); + + var stat = client.checkExists().forPath(createdPath); + assertThat(stat).isNotNull(); + } + } +} diff --git a/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionNestedTest.java b/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionNestedTest.java new file mode 100644 index 00000000..4c7784c3 --- /dev/null +++ b/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionNestedTest.java @@ -0,0 +1,122 @@ +package org.kiwiproject.test.curator; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; + +import lombok.extern.slf4j.Slf4j; +import org.apache.curator.framework.CuratorFramework; +import org.apache.curator.framework.imps.CuratorFrameworkState; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * This test is intentionally structured with multiple @{@link Nested} test classes to verify that the + * extension functions properly when there are top-level tests as well as tests within nested test classes. The + * {@link CuratorTestingServerExtensionTest} has the same tests as this one does, except they are all top-level + * tests with no nested test classes. + */ +@Slf4j +@DisplayName("CuratorTestingServerExtension with Nested test classes") +class CuratorTestingServerExtensionNestedTest { + + @RegisterExtension + static final CuratorTestingServerExtension ZK_TEST_SERVER = new CuratorTestingServerExtension(); + + private CuratorFramework client; + + @BeforeEach + void setUp(TestInfo testInfo) { + LOG.trace("Test: {}", testInfo.getDisplayName()); + + client = ZK_TEST_SERVER.getClient(); + } + + /** + * Top-level test. + */ + @Test + void shouldHavePort() { + assertThat(ZK_TEST_SERVER.getPort()).isPositive(); + } + + /** + * Top-level test. + */ + @Test + void shouldHaveConnectionString() { + var port = ZK_TEST_SERVER.getPort(); + assertThat(ZK_TEST_SERVER.getConnectString()).isIn( + "localhost:" + port, + "127.0.0.1:" + port + ); + } + + @Nested + class Validation { + + @ParameterizedTest + @ValueSource(ints = {-42, -1, 65_536, 65_537, 90_000}) + void shouldRejectInvalidPorts(int value) { + assertThatIllegalArgumentException() + .isThrownBy(() -> new CuratorTestingServerExtension(value)) + .withMessage("Invalid port: " + value); + } + } + + @Nested + class Properties { + + @Test + void shouldHavePort() { + assertThat(ZK_TEST_SERVER.getPort()).isPositive(); + } + + @Test + void shouldHaveConnectionString() { + var port = ZK_TEST_SERVER.getPort(); + assertThat(ZK_TEST_SERVER.getConnectString()).isIn( + "localhost:" + port, + "127.0.0.1:" + port + ); + } + + @Test + void shouldHaveTempDirectory() { + assertThat(ZK_TEST_SERVER.getTempDirectory()) + .isDirectory() + .canRead() + .canWrite(); + } + + @Test + void shouldHaveClient() { + assertThat(client.getState()).isEqualTo(CuratorFrameworkState.STARTED); + } + } + + @Nested + class CanUseClient { + + @Test + void toCheckExistenceOfPath() throws Exception { + var stat = client.checkExists().forPath("/foo"); + assertThat(stat).isNull(); + } + + @Test + void toCreateZnode() throws Exception { + var path = "/bar/baz"; + var createdPath = client.create().creatingParentContainersIfNeeded().forPath(path); + assertThat(createdPath).isEqualTo(path); + + var stat = client.checkExists().forPath(createdPath); + assertThat(stat).isNotNull(); + } + } +} diff --git a/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionTest.java b/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionTest.java index 582b45af..37feb4cb 100644 --- a/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionTest.java +++ b/src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionTest.java @@ -2,6 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import lombok.extern.slf4j.Slf4j; import org.apache.curator.framework.CuratorFramework; @@ -16,12 +17,11 @@ import org.junit.jupiter.params.provider.ValueSource; /** - * This test is intentionally structured with multiple @{@link Nested} test classes to ensure that the - * extension functions properly in that situation with regards to the test lifecycle and when - * beforeAll/afterAll methods are called for nested test classes vs. the top-level test. + * This test has only top-level tests (no nested test classes). The {@link CuratorTestingServerExtensionNestedTest} + * has the same tests as this one does, except they are nested inside several @{@link Nested} test classes. */ -@Slf4j @DisplayName("CuratorTestingServerExtension") +@Slf4j class CuratorTestingServerExtensionTest { @RegisterExtension @@ -36,66 +36,65 @@ void setUp(TestInfo testInfo) { client = ZK_TEST_SERVER.getClient(); } - @Nested - class Validation { + @Test + void shouldThrowCuratorTestingServerException_WhenCannotStartServer() { + var port = ZK_TEST_SERVER.getPort(); + + assertThatThrownBy(() -> new CuratorTestingServerExtension(port)) + .isExactlyInstanceOf(CuratorTestingServerException.class) + .hasMessage("Error creating testing server on port %d", port) + .hasRootCauseExactlyInstanceOf(IllegalStateException.class) + .hasRootCauseMessage("port %d is not available", port); + } + + @ParameterizedTest + @ValueSource(ints = {-42, -1, 65_536, 65_537, 90_000}) + void shouldRejectInvalidPorts(int value) { + assertThatIllegalArgumentException() + .isThrownBy(() -> new CuratorTestingServerExtension(value)) + .withMessage("Invalid port: " + value); + } - @ParameterizedTest - @ValueSource(ints = {-42, -1, 65_536, 65_537, 90_000}) - void shouldRejectInvalidPorts(int value) { - assertThatIllegalArgumentException() - .isThrownBy(() -> new CuratorTestingServerExtension(value)) - .withMessage("Invalid port: " + value); - } + @Test + void shouldHavePort() { + assertThat(ZK_TEST_SERVER.getPort()).isPositive(); } - @Nested - class Properties { - - @Test - void shouldHavePort() { - assertThat(ZK_TEST_SERVER.getPort()).isPositive(); - } - - @Test - void shouldHaveConnectionString() { - var port = ZK_TEST_SERVER.getPort(); - assertThat(ZK_TEST_SERVER.getConnectString()).isIn( - "localhost:" + port, - "127.0.0.1:" + port - ); - } - - @Test - void shouldHaveTempDirectory() { - assertThat(ZK_TEST_SERVER.getTempDirectory()) - .isDirectory() - .canRead() - .canWrite(); - } - - @Test - void shouldHaveClient() { - assertThat(client.getState()).isEqualTo(CuratorFrameworkState.STARTED); - } + @Test + void shouldHaveConnectionString() { + var port = ZK_TEST_SERVER.getPort(); + assertThat(ZK_TEST_SERVER.getConnectString()).isIn( + "localhost:" + port, + "127.0.0.1:" + port + ); } - @Nested - class CanUseClient { + @Test + void shouldHaveTempDirectory() { + assertThat(ZK_TEST_SERVER.getTempDirectory()) + .isDirectory() + .canRead() + .canWrite(); + } - @Test - void toCheckExistenceOfPath() throws Exception { - var stat = client.checkExists().forPath("/foo"); - assertThat(stat).isNull(); - } + @Test + void shouldHaveClient() { + assertThat(client.getState()).isEqualTo(CuratorFrameworkState.STARTED); + } + + @Test + void canUseClient_toCheckExistenceOfPath() throws Exception { + var stat = client.checkExists().forPath("/foo"); + assertThat(stat).isNull(); + } - @Test - void toCreateZnode() throws Exception { - var path = "/bar/baz"; - var createdPath = client.create().creatingParentContainersIfNeeded().forPath(path); - assertThat(createdPath).isEqualTo(path); + @Test + void canUseClient_toCreateZnode() throws Exception { + var path = "/bar/baz"; + var createdPath = client.create().creatingParentContainersIfNeeded().forPath(path); + assertThat(createdPath).isEqualTo(path); - var stat = client.checkExists().forPath(createdPath); - assertThat(stat).isNotNull(); - } + var stat = client.checkExists().forPath(createdPath); + assertThat(stat).isNotNull(); } }