exceptionfactory commented on code in PR #6272:
URL: https://github.com/apache/nifi/pull/6272#discussion_r938933490
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestConvertRecord.java:
##########
@@ -49,18 +35,26 @@
import org.apache.nifi.util.MockFlowFile;
import org.apache.nifi.util.TestRunner;
import org.apache.nifi.util.TestRunners;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledOnOs;
+import org.junit.jupiter.api.condition.OS;
import org.xerial.snappy.SnappyInputStream;
-public class TestConvertRecord {
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.Map;
- //Apparently pretty printing is not portable as these tests fail on windows
- @BeforeClass
- public static void setUpSuite() {
- Assume.assumeTrue("Test only runs on *nix",
!SystemUtils.IS_OS_WINDOWS);
- }
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@DisabledOnOs(value = OS.WINDOWS, disabledReason = "Test only runs on *nix")
Review Comment:
The `disabledReason` should read `pretty-printing is not portable across
operating systems`.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListenRELP.java:
##########
@@ -91,18 +92,26 @@ public class TestListenRELP {
@Mock
private RestrictedSSLContextService sslContextService;
+ private AutoCloseable closeable;
+
private RELPEncoder encoder;
private TestRunner runner;
- @Before
+ @BeforeEach
public void setup() {
encoder = new RELPEncoder(CHARSET);
ListenRELP mockRELP = new MockListenRELP();
runner = TestRunners.newTestRunner(mockRELP);
+ closeable = MockitoAnnotations.openMocks(this);
Review Comment:
Instead of using `MockitoAnnotations.openMocks()`, the class should be
annotated with `@ExtendWith(MockitoExtension.class)`
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListenUDP.java:
##########
@@ -47,7 +44,9 @@
import java.util.List;
import java.util.concurrent.BlockingQueue;
-@RunWith(MockitoJUnitRunner.class)
Review Comment:
It looks like this class needs to be annotated with
`@ExtendWith(MockitoExtension.class)` in order to initialized member variables
annotated with `@Mock`.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestExecuteProcess.java:
##########
@@ -83,7 +83,7 @@ public void testSplitArgs() {
assertEquals("", twoArgOneWholeQuotedArgOneEmptyArg.get(3));
}
- @Ignore // won't run under Windows
+ @DisabledOnOs(OS.WINDOWS) // won't run under Windows
Review Comment:
The comment can be removed
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/ITestHandleHttpRequest.java:
##########
@@ -273,7 +276,8 @@ public void run() {
mff.assertAttributeExists("http.headers.multipart.content-disposition");
}
- @Test(timeout = 30000)
+ @Test
+ @Timeout(value = 30000, unit = TimeUnit.MILLISECONDS)
Review Comment:
This can be changed to `@Timeout(30)` and the `TimeUnit` can be removed.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestDetectDuplicate.java:
##########
@@ -31,22 +25,40 @@
import org.apache.nifi.logging.ComponentLog;
import org.apache.nifi.reporting.InitializationException;
import org.apache.nifi.state.MockStateManager;
-import org.apache.nifi.util.MockControllerServiceInitializationContext;
import org.apache.nifi.util.MockComponentLog;
+import org.apache.nifi.util.MockControllerServiceInitializationContext;
import org.apache.nifi.util.TestRunner;
import org.apache.nifi.util.TestRunners;
-import org.junit.Test;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
public class TestDetectDuplicate {
- static {
+ @BeforeAll
+ public static void setup() {
System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
System.setProperty("org.slf4j.simpleLogger.log.nifi.io.nio", "debug");
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.DetectDuplicate",
"debug");
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.TestDetectDuplicate",
"debug");
}
+ @AfterAll
+ public static void cleanup() {
+ System.clearProperty("org.slf4j.simpleLogger.defaultLogLevel");
+ System.clearProperty("org.slf4j.simpleLogger.showDateTime");
+ System.clearProperty("org.slf4j.simpleLogger.log.nifi.io.nio");
+
System.clearProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.DetectDuplicate");
+
System.clearProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.TestDetectDuplicate");
+ }
Review Comment:
Additional logging should not be necessary in tests, so this is a good
opportunity to remove all of these system properties.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/AbstractTestTailFileScenario.java:
##########
@@ -125,7 +126,7 @@ public void testScenario(List<Action> actions) throws
Exception {
public void testScenario(List<Action> actions, boolean
stopAfterEachTrigger) throws Exception {
if (actions.contains(Action.ROLLOVER)) {
- Assume.assumeTrue("Test wants to rename an open file which is not
allowed on Windows", !SystemUtils.IS_OS_WINDOWS);
+ assumeTrue(!SystemUtils.IS_OS_WINDOWS, "Test wants to rename an
open file which is not allowed on Windows");
Review Comment:
References to this test method should be refactored and annotated with
`DisabledOnOs(OS.WINDOWS)` as opposed to using this `assumeTrue` approach.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncryptContent.java:
##########
@@ -47,14 +29,34 @@
import org.bouncycastle.bcpg.BCPGInputStream;
import org.bouncycastle.bcpg.SymmetricKeyEncSessionPacket;
import org.bouncycastle.jce.provider.BouncyCastleProvider;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledOnOs;
+import org.junit.jupiter.api.condition.OS;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.lang.reflect.Method;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
+import java.security.Security;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+
+import static org.bouncycastle.openpgp.PGPUtil.getDecoderStream;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+
+@DisabledOnOs(value = OS.WINDOWS, disabledReason = "Test only runs on *nix")
Review Comment:
Is it clear why this fails on Windows? It would be helpful to include a more
descriptive reason if possible, otherwise this is acceptable.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestExecuteSQL.java:
##########
@@ -99,15 +91,29 @@ public class TestExecuteSQL {
+ " from persons PER, products PRD, relationships REL"
+ " where PER.ID < ? AND REL.ID < ?";
-
- @BeforeClass
+ @BeforeAll
public static void setupClass() {
+ System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
+ System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
+ System.setProperty("org.slf4j.simpleLogger.log.nifi.io.nio", "debug");
+
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.ExecuteSQL",
"debug");
+
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.TestExecuteSQL",
"debug");
Review Comment:
These logging system properties should be removed
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestExecuteProcess.java:
##########
@@ -83,7 +83,7 @@ public void testSplitArgs() {
assertEquals("", twoArgOneWholeQuotedArgOneEmptyArg.get(3));
}
- @Ignore // won't run under Windows
+ @DisabledOnOs(OS.WINDOWS) // won't run under Windows
@Test
public void testEcho() {
System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi",
"TRACE");
Review Comment:
This system property should be removed
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestDistributeLoad.java:
##########
@@ -16,26 +16,33 @@
*/
package org.apache.nifi.processors.standard;
-import static org.junit.Assert.assertEquals;
-
-import java.util.List;
-
import org.apache.nifi.util.MockFlowFile;
import org.apache.nifi.util.TestRunner;
import org.apache.nifi.util.TestRunners;
-import org.junit.Assert;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
public class TestDistributeLoad {
- @BeforeClass
+ @BeforeAll
public static void before() {
System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.DistributeLoad",
"debug");
}
+ @AfterAll
+ public static void cleanup() {
+ System.clearProperty("org.slf4j.simpleLogger.defaultLogLevel");
+ System.clearProperty("org.slf4j.simpleLogger.showDateTime");
+
System.clearProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.DistributeLoad");
+ }
Review Comment:
All logging-related system properties should be removed.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListFile.java:
##########
@@ -177,7 +175,7 @@ private void runNext() throws InterruptedException {
@Test
- @Ignore("Intended only for manual testing, as is very expensive to run as
a unit test. Performs listing of 1,000,000 files (doesn't actually create the
files, though - injects them in) to " +
+ @Disabled("Intended only for manual testing, as is very expensive to run
as a unit test. Performs listing of 1,000,000 files (doesn't actually create
the files, though - injects them in) to " +
"ensure performance is not harmed")
Review Comment:
This should be changed to `EnableIfSystemProperty` so that it could be run
optionally, instead of disabled.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestExecuteSQLRecord.java:
##########
@@ -106,14 +98,29 @@ public class TestExecuteSQLRecord {
+ " where PER.ID < ? AND REL.ID < ?";
- @BeforeClass
+ @BeforeAll
public static void setupClass() {
+ System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
+ System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
+ System.setProperty("org.slf4j.simpleLogger.log.nifi.io.nio", "debug");
+
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.ExecuteSQLRecord",
"debug");
+
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.TestExecuteSQLRecord",
"debug");
Review Comment:
SLF4J logging system properties should be removed. The Derby logging
property can remain.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestExecuteStreamCommand.java:
##########
@@ -33,31 +44,28 @@
import java.util.Set;
import java.util.regex.Pattern;
-import com.fasterxml.jackson.databind.JsonNode;
-import com.fasterxml.jackson.databind.ObjectMapper;
-import org.apache.commons.io.FileUtils;
-import org.apache.commons.lang3.SystemUtils;
-import org.apache.nifi.components.PropertyDescriptor;
-import org.apache.nifi.expression.ExpressionLanguageScope;
-import org.apache.nifi.processors.standard.util.ArgumentUtils;
-import org.apache.nifi.util.MockFlowFile;
-import org.apache.nifi.util.TestRunner;
-import org.apache.nifi.util.TestRunners;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Ignore;
-import org.junit.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+@DisabledOnOs(value = OS.WINDOWS, disabledReason = "Test only runs on *nix")
public class TestExecuteStreamCommand {
- @BeforeClass
+ @BeforeAll
public static void init() {
- Assume.assumeTrue("Test only runs on *nix",
!SystemUtils.IS_OS_WINDOWS);
System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.ExecuteStreamCommand",
"debug");
System.setProperty("org.slf4j.simpleLogger.log.nifi.processors.standard.TestExecuteStreamCommand",
"debug");
Review Comment:
SLF4J logging system properties should be removed.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestPutSQL.java:
##########
@@ -338,14 +342,12 @@ public void
testFailInMiddleWithBadStatementRollbackOnFailure() throws Initializ
runner.enqueue("INSERT INTO PERSONS_AI (NAME, CODE) VALUES ('Tom',
3)".getBytes());
runner.enqueue("INSERT INTO PERSONS_AI (NAME, CODE) VALUES ('Harry',
44)".getBytes());
- try {
- runner.run();
- fail("ProcessException should be thrown");
- } catch (AssertionError e) {
- assertTrue(e.getCause() instanceof ProcessException);
- runner.assertTransferCount(PutSQL.REL_FAILURE, 0);
- runner.assertTransferCount(PutSQL.REL_SUCCESS, 0);
- }
+ final AssertionError e = assertThrows(AssertionError.class, () -> {
+ runner.run();
+ });
+ assertTrue(e.getCause() instanceof ProcessException);
Review Comment:
This should be changed to use `assertInstanceOf`
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestMergeRecord.java:
##########
@@ -451,7 +451,7 @@ public void testMaxSize() {
}
@Test
- @Ignore("This unit test depends on timing and could potentially cause
problems in an automated build environment. However, it can be useful for
manual testing")
+ @Disabled("This unit test depends on timing and could potentially cause
problems in an automated build environment. However, it can be useful for
manual testing")
Review Comment:
This should be changed to use `EnabledIfSystemProperty`
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestMergeContent.java:
##########
@@ -57,15 +56,23 @@
import java.util.Set;
import java.util.zip.ZipInputStream;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
public class TestMergeContent {
- @BeforeClass
+ @BeforeAll
public static void setup() {
System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.standard",
"DEBUG");
}
+ @AfterAll
+ public static void cleanup() {
+
System.clearProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.standard");
+ }
Review Comment:
These methods can be removed
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestPutSQL.java:
##########
@@ -498,14 +498,12 @@ public void
testFailInMiddleWithBadParameterValueRollbackOnFailure() throws Init
runner.enqueue(data, goodAttributes);
runner.enqueue(data, goodAttributes);
- try {
- runner.run();
- fail("ProcessException should be thrown");
- } catch (AssertionError e) {
- assertTrue(e.getCause() instanceof ProcessException);
- runner.assertTransferCount(PutSQL.REL_FAILURE, 0);
- runner.assertTransferCount(PutSQL.REL_SUCCESS, 0);
- }
+ final AssertionError e = assertThrows(AssertionError.class, () -> {
+ runner.run();
+ });
+ assertTrue(e.getCause() instanceof ProcessException);
Review Comment:
All similar checks should be changed to use `assertInstanceOf`
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListenRELP.java:
##########
@@ -91,18 +92,26 @@ public class TestListenRELP {
@Mock
private RestrictedSSLContextService sslContextService;
+ private AutoCloseable closeable;
+
private RELPEncoder encoder;
private TestRunner runner;
- @Before
+ @BeforeEach
public void setup() {
encoder = new RELPEncoder(CHARSET);
ListenRELP mockRELP = new MockListenRELP();
runner = TestRunners.newTestRunner(mockRELP);
+ closeable = MockitoAnnotations.openMocks(this);
+ }
+
+ @AfterEach
+ public void cleanup() throws Exception {
+ closeable.close();
}
Review Comment:
This should be unnecessary for a mocked object.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestQueryRecord.java:
##########
@@ -50,27 +52,36 @@
import java.time.LocalDate;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
-import java.util.Arrays;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
public class TestQueryRecord {
- static {
+ @BeforeAll
+ public static void setup() {
System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
System.setProperty("org.slf4j.simpleLogger.log.nifi.io.nio", "debug");
System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.standard.SQLTransform",
"debug");
}
+ @AfterAll
+ public static void cleanup() {
+ System.clearProperty("org.slf4j.simpleLogger.defaultLogLevel");
+ System.clearProperty("org.slf4j.simpleLogger.showDateTime");
+ System.clearProperty("org.slf4j.simpleLogger.log.nifi.io.nio");
+
System.clearProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.standard.SQLTransform");
+ }
Review Comment:
These system properties can be removed.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestPutJMS.java:
##########
@@ -46,13 +25,36 @@
import org.apache.nifi.util.MockFlowFile;
import org.apache.nifi.util.TestRunner;
import org.apache.nifi.util.TestRunners;
-import org.junit.Ignore;
-import org.junit.Test;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
+import javax.jms.Connection;
+import javax.jms.JMSException;
+import javax.jms.Message;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import java.lang.reflect.Field;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+import java.util.concurrent.LinkedBlockingQueue;
+
+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.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
+
@SuppressWarnings("deprecation")
-@Ignore("Processor requires updates to work with Mockito 2.x, but is
deprecated.")
+@Disabled("Processor requires updates to work with Mockito 2.x, but is
deprecated.")
Review Comment:
If this class is not functional, it should be removed to avoid compatibility
issues with Mockito upgrades. Did you getting it to work with current library
versions?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
