You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/08/01 16:00:48 UTC
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6262: NIFI-10298: changed tests in nifi-framework-cluster to use Java if po…
exceptionfactory commented on code in PR #6262:
URL: https://github.com/apache/nifi/pull/6262#discussion_r934682145
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -60,16 +60,18 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
public class TestThreadPoolRequestReplicator {
- @BeforeClass
+ @BeforeAll
public static void setupClass() {
System.setProperty(NiFiProperties.PROPERTIES_FILE_PATH, "src/test/resources/conf/nifi.properties");
Review Comment:
This test class should be updated to include a new `clearProperty()` method, annotated with `AfterAll`, the calls `System.clearProperty(NiFiProperties.PROPERTIES_FILE_PATH)`
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -192,7 +194,8 @@ public void testRequestChainWithIdentityProviderGroups() {
}, Response.Status.OK, 0L, null, expectedRequestChain, expectedProxiedEntityGroups);
}
- @Test(timeout = 15000)
+ @Test
+ @Timeout(value = 16000, unit = TimeUnit.MILLISECONDS)
Review Comment:
This can be simplified to `Timeout(15)`, since the default time unit is seconds.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -221,7 +224,8 @@ public void testLongWaitForResponse() {
}, Status.OK, 1000, new ProcessingException(new SocketTimeoutException()));
}
- @Test(timeout = 15000)
+ @Test
+ @Timeout(value = 15000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(15)
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -246,8 +250,9 @@ public void testCompleteOnError() {
}, null, 0L, new IllegalArgumentException("Exception created for unit test"));
}
- @Test(timeout = 15000)
- public void testMultipleRequestWithTwoPhaseCommit() {
+ @Test
+ @Timeout(value = 15000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(15)
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -352,26 +353,20 @@ protected NodeResponse replicateRequest(final PreparedRequest request, final Nod
}
};
- try {
- // set the user
+ final Exception exception = assertThrows(Exception.class, () -> {
final Authentication authentication = new NiFiAuthenticationToken(new NiFiUserDetails(StandardNiFiUser.ANONYMOUS));
SecurityContextHolder.getContext().setAuthentication(authentication);
final AsyncClusterResponse clusterResponse = replicator.replicate(nodeIds, HttpMethod.POST,
new URI("http://localhost:80/processors/1"), new ProcessorEntity(), new HashMap<>(), true, true);
clusterResponse.awaitMergedResponse();
-
- Assert.fail("Expected to get an IllegalClusterStateException but did not");
- } catch (final IllegalClusterStateException e) {
- // Expected
- } catch (final Exception e) {
- Assert.fail(e.toString());
- } finally {
- replicator.shutdown();
- }
+ });
+ assertTrue(exception instanceof IllegalClusterStateException);
Review Comment:
This can be changed to `assertInstanceOf`, or even better, `assertThrows` should be changed to expect `IllegalClusterException`
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/firewall/impl/FileBasedClusterNodeFirewallTest.java:
##########
@@ -80,18 +82,14 @@ public void setup() throws Exception {
/**
* We have two garbage lines in our test config file, ensure they didn't get turned into hosts.
*/
- @Ignore("This does not run consistently on different environments")
+ @Disabled("This does not run consistently on different environments")
@Test
public void ensureBadDataWasIgnored() {
Review Comment:
This test method can be removed.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/heartbeat/TestAbstractHeartbeatMonitor.java:
##########
@@ -139,7 +139,7 @@ public synchronized Future<Void> requestNodeDisconnect(final NodeIdentifier node
assertTrue(requestedToConnect.isEmpty());
}
- @Ignore("this test is too unstable in terms of timing on different size/types of testing envs")
+ @Disabled("this test is too unstable in terms of timing on different size/types of testing envs")
@Test
public void testDisconnectionOfTerminatedNodeDueToLackOfHeartbeat() throws Exception {
Review Comment:
We should take the opportunity to remove this test method.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/okhttp/OkHttpReplicationClientTest.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.cluster.coordination.http.replication.okhttp;
+
+import org.apache.nifi.security.util.TemporaryKeyStoreBuilder;
+import org.apache.nifi.security.util.TlsConfiguration;
+import org.apache.nifi.util.NiFiProperties;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class OkHttpReplicationClientTest {
+ private static TlsConfiguration tlsConfiguration;
+
+ @BeforeAll
+ public static void setUpOnce() {
+ tlsConfiguration = new TemporaryKeyStoreBuilder().build();
+ }
+
+ @Test
+ public void testShouldReplaceNonZeroContentLengthHeader() {
+ // Arrange
+ final Map<String, String> headers = new HashMap<>();
+ headers.put("Content-Length", "123");
+ headers.put("Other-Header", "arbitrary value");
+
+ // must be case-insensitive
+ final String[] methods = new String[] {"DELETE", "delete", "DeLeTe"};
+
+ final NiFiProperties mockProperties = mockNiFiProperties();
+
+ final OkHttpReplicationClient client = new OkHttpReplicationClient(mockProperties);
+
+ for (final String method: methods) {
+ // Act
+ client.prepareRequest(method, headers, null);
+
+ // Assert
+ assertEquals(2, headers.size());
+ assertEquals("0", headers.get("Content-Length"));
+ }
Review Comment:
The `Arrange/Act/Assert` comments should be removed.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -317,7 +317,8 @@ public NodeConnectionStatus answer(InvocationOnMock invocation) throws Throwable
return coordinator;
}
- @Test(timeout = 15000)
+ @Test
+ @Timeout(value = 15000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(15)
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/integration/ClusterConnectionIT.java:
##########
@@ -32,43 +32,46 @@
import org.apache.nifi.cluster.coordination.node.NodeConnectionState;
import org.apache.nifi.cluster.coordination.node.NodeConnectionStatus;
import org.apache.nifi.cluster.protocol.NodeIdentifier;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
public class ClusterConnectionIT {
private Cluster cluster;
- @BeforeClass
+ @BeforeAll
public static void setup() {
System.setProperty("nifi.properties.file.path", "src/test/resources/conf/nifi.properties");
}
- @Before
+ @BeforeEach
public void createCluster() throws IOException {
cluster = new Cluster();
cluster.start();
}
- @After
+ @AfterEach
public void destroyCluster() {
if (cluster != null) {
cluster.stop();
}
}
- @Test(timeout = 20000)
- public void testSingleNode() throws InterruptedException {
+ @Test
+ @Timeout(value = 20000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(20)
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -352,26 +353,20 @@ protected NodeResponse replicateRequest(final PreparedRequest request, final Nod
}
};
- try {
- // set the user
+ final Exception exception = assertThrows(Exception.class, () -> {
final Authentication authentication = new NiFiAuthenticationToken(new NiFiUserDetails(StandardNiFiUser.ANONYMOUS));
SecurityContextHolder.getContext().setAuthentication(authentication);
final AsyncClusterResponse clusterResponse = replicator.replicate(nodeIds, HttpMethod.POST,
new URI("http://localhost:80/processors/1"), new ProcessorEntity(), new HashMap<>(), true, true);
clusterResponse.awaitMergedResponse();
-
- Assert.fail("Expected to get an IllegalClusterStateException but did not");
- } catch (final IllegalClusterStateException e) {
- // Expected
- } catch (final Exception e) {
- Assert.fail(e.toString());
- } finally {
- replicator.shutdown();
- }
+ });
+ assertTrue(exception instanceof IllegalClusterStateException);
+ replicator.shutdown();
}
- @Test(timeout = 5000)
+ @Test
+ @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(5)
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/node/TestNodeClusterCoordinator.java:
##########
@@ -174,7 +176,8 @@ void notifyOthersOfNodeStatusChange(NodeConnectionStatus updatedStatus, boolean
assertEquals(5, response.getTryLaterSeconds());
}
- @Test(timeout = 5000)
+ @Test
+ @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(5)
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -413,18 +407,16 @@ public void run() {
// Pass in Collections.emptySet() for the node ID's so that an Exception is thrown
replicator.replicate(Collections.emptySet(), "GET", new URI("localhost:8080/nifi"), Collections.emptyMap(),
- updatedHeaders, true, null, true, true, monitor);
- Assert.fail("replicate did not throw IllegalArgumentException");
- } catch (final IllegalArgumentException iae) {
- // expected
- }
+ updatedHeaders, true, null, true, true, monitor);
+ });
// wait for monitor to be notified.
postNotifyLatch.await();
});
}
- @Test(timeout = 5000)
+ @Test
+ @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(5)
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/http/replication/TestThreadPoolRequestReplicator.java:
##########
@@ -477,7 +469,8 @@ public void run() {
}
- @Test(timeout = 5000)
+ @Test
+ @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS)
Review Comment:
```suggestion
@Timeout(5)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org