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/12/12 19:28:35 UTC
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6775: [NIFI-10877] Conversion of nifi-framework-core tests from Junit4 to J…
exceptionfactory commented on code in PR #6775:
URL: https://github.com/apache/nifi/pull/6775#discussion_r1046275711
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/StandardProcessorNodeIT.java:
##########
@@ -206,9 +204,9 @@ public void testSinglePropertyDynamicallyModifiesClasspath() throws MalformedURL
}
@Test
- public void testNativeLibLoadedFromDynamicallyModifiesClasspathProperty() throws Exception {
+ public void testNativeLibLoadedFromDynamicallyModifiesClasspathProperty() {
// GIVEN
- assumeTrue("Test only runs on Mac OS", new OSUtil(){}.isOsMac());
+ assumeTrue(new OSUtil(){}.isOsMac(), "Test only runs on Mac OS");
Review Comment:
This approach should be replaced with the `EnabledOnOs` annotation
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/StandardFlowServiceTest.java:
##########
@@ -111,13 +112,16 @@ public void testLoadWithFlow() throws IOException {
String expectedFlow = new String(flowBytes).trim();
String actualFlow = new String(baos.toByteArray()).trim();
- Assert.assertEquals(expectedFlow, actualFlow);
+ Assertions.assertEquals(expectedFlow, actualFlow);
Review Comment:
This is a good opportunity to replace `Assertions.assertEquals` with the static import of `assertEquals`:
```suggestion
assertEquals(expectedFlow, actualFlow);
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/repository/TestFileSystemRepository.java:
##########
@@ -75,12 +74,12 @@ public class TestFileSystemRepository {
private final File rootFile = new File("target/content_repository");
private NiFiProperties nifiProperties;
- @BeforeClass
+ @BeforeAll
public static void setupClass() {
- Assume.assumeTrue("Test only runs on *nix", !SystemUtils.IS_OS_WINDOWS);
+ assumeTrue(!SystemUtils.IS_OS_WINDOWS, "Test only runs on *nix");
Review Comment:
Replace with `DisabledOnOs(OS.WINDOWS)` at the class level.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/scheduling/TestStandardProcessScheduler.java:
##########
@@ -390,16 +380,13 @@ public void validateDisabledServiceCantBeDisabled() throws Exception {
final AtomicBoolean asyncFailed = new AtomicBoolean();
for (int i = 0; i < 1000; i++) {
- executor.execute(new Runnable() {
- @Override
- public void run() {
- try {
- scheduler.disableControllerService(serviceNode);
- assertFalse(serviceNode.isActive());
- } catch (final Exception e) {
- e.printStackTrace();
- asyncFailed.set(true);
- }
+ executor.execute(() -> {
+ try {
+ scheduler.disableControllerService(serviceNode);
+ assertFalse(serviceNode.isActive());
+ } catch (final Exception e) {
+ e.printStackTrace();
Review Comment:
```suggestion
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/scheduling/TestStandardProcessScheduler.java:
##########
@@ -431,16 +418,13 @@ public void validateEnabledServiceCanOnlyBeDisabledOnce() throws Exception {
final AtomicBoolean asyncFailed = new AtomicBoolean();
for (int i = 0; i < 1000; i++) {
- executor.execute(new Runnable() {
- @Override
- public void run() {
- try {
- scheduler.disableControllerService(serviceNode);
- assertFalse(serviceNode.isActive());
- } catch (final Exception e) {
- e.printStackTrace();
- asyncFailed.set(true);
- }
+ executor.execute(() -> {
+ try {
+ scheduler.disableControllerService(serviceNode);
+ assertFalse(serviceNode.isActive());
+ } catch (final Exception e) {
+ e.printStackTrace();
Review Comment:
```suggestion
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/scheduling/TestStandardProcessScheduler.java:
##########
@@ -352,16 +345,13 @@ public void validateServiceEnablementLogicHappensOnlyOnce() throws Exception {
final AtomicBoolean asyncFailed = new AtomicBoolean();
for (int i = 0; i < 1000; i++) {
- executor.execute(new Runnable() {
- @Override
- public void run() {
- try {
- scheduler.enableControllerService(serviceNode).get();
- assertTrue(serviceNode.isActive());
- } catch (final Exception e) {
- e.printStackTrace();
- asyncFailed.set(true);
- }
+ executor.execute(() -> {
+ try {
+ scheduler.enableControllerService(serviceNode).get();
+ assertTrue(serviceNode.isActive());
+ } catch (final Exception e) {
+ e.printStackTrace();
Review Comment:
`e.printStackTrace()` should be removed.
```suggestion
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/TestStandardFlowFileQueue.java:
##########
@@ -71,12 +72,12 @@ public class TestStandardFlowFileQueue {
private List<ProvenanceEventRecord> provRecords = new ArrayList<>();
- @BeforeClass
+ @BeforeAll
public static void setupLogging() {
System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi", "INFO");
}
Review Comment:
This method should be removed since additional logging should not be enabled as a general rule.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/queue/clustered/TestContentRepositoryFlowFileAccess.java:
##########
@@ -121,7 +121,7 @@ public void testEOFExceptionIfNotEnoughData() throws IOException {
try {
repoStream.read();
- Assert.fail("Expected EOFException because not enough bytes were in the InputStream for the FlowFile");
+ fail("Expected EOFException because not enough bytes were in the InputStream for the FlowFile");
} catch (final EOFException eof) {
// expected
}
Review Comment:
It looks like this can be changed to `assertThrows(EOFException.class, () -> repoStream.read());`
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/integration/FrameworkIntegrationTest.java:
##########
@@ -368,7 +368,6 @@ public Collection<FlowFileQueue> getAllQueues() {
flowController.initializeFlow(queueProvider);
}
- @After
Review Comment:
It looks like `AfterEach` should be added to this method, or it should be consolidated with `shutdown()`.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/status/history/EmbeddedQuestDbRolloverHandlerTest.java:
##########
@@ -43,7 +42,10 @@
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
-@Ignore("Buggy tests depend on time of day")
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@Disabled("Buggy tests depend on time of day")
Review Comment:
This is a good opportunity to remove this class based on the comments. This file can be deleted.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/groovy/org/apache/nifi/fingerprint/FingerprintFactoryGroovyTest.groovy:
##########
@@ -57,17 +54,17 @@ class FingerprintFactoryGroovyTest extends GroovyTestCase {
}
}
- @Before
+ @BeforeEach
void setUp() throws Exception {
}
- @After
+ @AfterEach
void tearDown() throws Exception {
}
Review Comment:
These empty methods can be removed.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/groovy/org/apache/nifi/controller/repository/crypto/EncryptedFileSystemRepositoryTest.groovy:
##########
@@ -64,9 +61,9 @@ class EncryptedFileSystemRepositoryTest {
(NiFiProperties.CONTENT_REPOSITORY_ENCRYPTION_KEY_PROVIDER_LOCATION) : ""
]
- @BeforeClass
+ @BeforeAll
static void setUpOnce() throws Exception {
- Assume.assumeTrue("Test only runs on *nix", !SystemUtils.IS_OS_WINDOWS)
+ Assumptions.assumeTrue(!SystemUtils.IS_OS_WINDOWS, "Test only runs on *nix")
Review Comment:
This class and others should be changed to use the new `DisabledOnOs(OS.WINDOWS)` JUnit 5 annotation, will clean up some of these `SystemUtils` and `BeforeAll` setup steps.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/integration/FrameworkIntegrationTest.java:
##########
@@ -140,8 +138,8 @@ public class FrameworkIntegrationTest {
//@Rule
public Timeout globalTimeout = Timeout.seconds(20);
- @Rule
- public TestName name = new TestName();
+ /*@Rule
+ public TestName name = new TestName();*/
Review Comment:
This commented line should be removed.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/integration/swap/ClusteredSwapFileIT.java:
##########
@@ -34,20 +34,21 @@
import org.apache.nifi.events.EventReporter;
import org.apache.nifi.integration.FrameworkIntegrationTest;
import org.apache.nifi.integration.processors.GenerateProcessor;
-import org.junit.Ignore;
-import org.junit.Test;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
import org.mockito.Mockito;
import java.io.IOException;
import java.util.Collections;
import java.util.Set;
import java.util.concurrent.ExecutionException;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
-@Ignore("Tests need to be updated")
+@Disabled("Tests need to be updated")
Review Comment:
This is a good opportunity to remove this test class.
--
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