From 9ff557aecb042beb042f0b7821d1fc708db12e81 Mon Sep 17 00:00:00 2001 From: Scott Leberknight <174812+sleberknight@users.noreply.github.com> Date: Fri, 20 Aug 2021 17:08:25 -0400 Subject: [PATCH] Add ability to immediately start ZK testing server in Curator extension * Add two factory methods to CuratorTestingServerExtension that will start the Curator testing server immediately (at construction) instead of during the JUnit lifecycle (i.e. in beforeAll). These are named newStartingImmediately and one will start the testing server on a random port and the other takes a specific port to use. * Before attempting to start the testing server, check that the port is available. Otherwise, when Curator's TestingServer attempts to start, it will never complete. The reason is that TestingServer starts the server in a new Thread and then blocks (without a timeout) until connected. We don't want this behavior in a test extension and instead should fail fast. * Restructure existing test to verify nested test class behavior separate from tests having only top-level tests * Add new test specifically to test the immediate start (necessary since we have to register the CuratorTestingServerExtension with immediate start explicitly) Test restructuring: Split the original CuratorTestingServerExtensionTest test into two separate tests. The first, CuratorTestingServerExtensionTest, contains only top-level tests while the second, CuratorTestingServerExtensionNestedTest, contains both top-level and nested test classes. These two contain essentially the exact same tests, to ensure we are agnostic of how a test is structured. Though as noted in extension's Javadoc, nested test classes will cause us to use multiple testing servers (due to the way the JUnit lifecycle works when using nested test classes). Add CuratorTestingServerExtensionImmediateStartTest to explicitly test the immediate start feature added in this commit. Closes #239 --- .../CuratorTestingServerExtension.java | 61 ++++++++- ...tingServerExtensionImmediateStartTest.java | 128 ++++++++++++++++++ ...ratorTestingServerExtensionNestedTest.java | 122 +++++++++++++++++ .../CuratorTestingServerExtensionTest.java | 113 ++++++++-------- 4 files changed, 361 insertions(+), 63 deletions(-) create mode 100644 src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionImmediateStartTest.java create mode 100644 src/test/java/org/kiwiproject/test/curator/CuratorTestingServerExtensionNestedTest.java 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(); } }