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 2021/08/12 11:34:29 UTC
[GitHub] [nifi] MikeThomsen opened a new pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
MikeThomsen opened a new pull request #5304:
URL: https://github.com/apache/nifi/pull/5304
…ion of JUnit 5 and JUnit Vintage. Updated nifi-mock to be exclusively JUnit 5 and updated a few modules to demonstrate a successful conversion to all JUnit 5 while the rest of the build is slowly converted to JUnit 5.
<!--
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.
-->
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFI-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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
[GitHub] [nifi] MikeThomsen commented on pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#issuecomment-897565386
@exceptionfactory for some reason, I couldn't add you as a requested reviewer.
--
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
[GitHub] [nifi] MikeThomsen commented on pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#issuecomment-898085956
Thanks for the quick review!
--
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
[GitHub] [nifi] MikeThomsen commented on pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#issuecomment-897674342
@exceptionfactory made the changes
--
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#discussion_r687818449
##########
File path: nifi-mock/pom.xml
##########
@@ -66,8 +66,8 @@
<dependency>
<!-- Dependency marked as compile, not test, because we have assertion
methods in our MockSession & MockFlowFile -->
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
Review comment:
To be more precise in dependency declarations, this should be `junit-jupiter-api` instead of `junit-jupiter-engine`.
```suggestion
<artifactId>junit-jupiter-api</artifactId>
```
##########
File path: pom.xml
##########
@@ -267,10 +267,21 @@
</dependency>
<!-- These junit/mockito/groovy/spock/hamcrest dependencies are here to encourage consistent unit test library usage -->
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ <version>5.7.2</version>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.vintage</groupId>
+ <artifactId>junit-vintage-engine</artifactId>
+ <version>5.7.2</version>
+ </dependency>
Review comment:
We should leverage the JUnit Bill-of-Materials dependency to declare the version once and ensure consistency.
```suggestion
<dependency>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.7.2</version>
<type>pom</type>
<scope>import</scope>
</dependency>
```
##########
File path: nifi-nar-bundles/nifi-graph-bundle/nifi-graph-processors/src/test/java/org/apache/nifi/processors/graph/TestExecuteGraphQuery.java
##########
@@ -41,9 +41,10 @@
public class TestExecuteGraphQuery {
protected TestRunner runner;
- @Rule public MockitoRule mockitoRule = MockitoJUnit.rule();
+ @Rule
+ public MockitoRule mockitoRule = MockitoJUnit.rule();
Review comment:
The `@Rule` annotation is a JUnit 4 feature that is implemented differently in JUnit 5. In this particular test class, it looks like the annotation and the `MockitoRule` variable can be removed
##########
File path: pom.xml
##########
@@ -455,8 +466,13 @@
<dependencies>
<dependency>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
Review comment:
The `junit-jupiter-api` should also be declared as a direct dependency in addition to the engines.
```suggestion
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifiactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
```
--
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
[GitHub] [nifi] MikeThomsen commented on pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#issuecomment-897801383
@exceptionfactory updates made.
--
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
[GitHub] [nifi] MikeThomsen commented on a change in pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#discussion_r687732448
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-services/src/main/java/org/apache/nifi/mongodb/MongoDBLookupService.java
##########
@@ -67,6 +66,10 @@
"then the entire MongoDB result document minus the _id field will be returned as a record."
)
public class MongoDBLookupService extends JsonInferenceSchemaRegistryService implements LookupService<Object> {
+ public static final PropertyDescriptor LOCAL_SCHEMA_NAME = new PropertyDescriptor.Builder()
+ .fromPropertyDescriptor(SCHEMA_NAME)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
Review comment:
Yeah. When expressionLanguageSupported was refactored to use an enum, it was changed to be for FLOWFILE_VARIABLE and not VARIABLE_REGISTRY which is the only option a controller service can reasonably accept. The breakage didn't appear until I ran the integration test, so I fixed it here.
--
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
[GitHub] [nifi] MikeThomsen commented on a change in pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#discussion_r687732448
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-services/src/main/java/org/apache/nifi/mongodb/MongoDBLookupService.java
##########
@@ -67,6 +66,10 @@
"then the entire MongoDB result document minus the _id field will be returned as a record."
)
public class MongoDBLookupService extends JsonInferenceSchemaRegistryService implements LookupService<Object> {
+ public static final PropertyDescriptor LOCAL_SCHEMA_NAME = new PropertyDescriptor.Builder()
+ .fromPropertyDescriptor(SCHEMA_NAME)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
Review comment:
Yeah. When expressionLanguageSupported was refactored to use an enum, it was changed to be for FLOWFILE_VARIABLE and not VARIABLE_REGISTRY which is the only option a controller service can reasonably accept.
--
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
[GitHub] [nifi] asfgit closed pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5304:
URL: https://github.com/apache/nifi/pull/5304
--
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5304: NIFI-9041 Replaced JUnit 4 only testing configuration with a combinat…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5304:
URL: https://github.com/apache/nifi/pull/5304#discussion_r687668668
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-services/src/main/java/org/apache/nifi/mongodb/MongoDBLookupService.java
##########
@@ -67,6 +66,10 @@
"then the entire MongoDB result document minus the _id field will be returned as a record."
)
public class MongoDBLookupService extends JsonInferenceSchemaRegistryService implements LookupService<Object> {
+ public static final PropertyDescriptor LOCAL_SCHEMA_NAME = new PropertyDescriptor.Builder()
+ .fromPropertyDescriptor(SCHEMA_NAME)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
Review comment:
Is there a reason for introducing this change as part of this PR?
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-services/src/test/java/org/apache/nifi/mongodb/MongoDBLookupServiceIT.java
##########
@@ -243,23 +246,28 @@ public void testServiceParameters() {
Map<String, Object> criteria = new HashMap<>();
criteria.put("uuid", "x-y-z");
- boolean error = false;
- try {
- service.lookup(criteria);
- } catch(Exception ex) {
- error = true;
- }
+// boolean error = false;
+// try {
+// service.lookup(criteria);
+// } catch(Exception ex) {
+// error = true;
+// }
Review comment:
Is there a reason for leaving these lines commented out? If they are no longer functional, recommend removing them completely, as well as other commented lines.
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-services/src/test/java/org/apache/nifi/mongodb/MongoDBLookupServiceIT.java
##########
@@ -243,23 +246,28 @@ public void testServiceParameters() {
Map<String, Object> criteria = new HashMap<>();
criteria.put("uuid", "x-y-z");
- boolean error = false;
- try {
- service.lookup(criteria);
- } catch(Exception ex) {
- error = true;
- }
+// boolean error = false;
+// try {
+// service.lookup(criteria);
+// } catch(Exception ex) {
+// error = true;
+// }
- Assert.assertFalse("An error was thrown when no error should have been thrown.", error);
- error = false;
+// Assertions.assertFalse(error, "An error was thrown when no error should have been thrown.");
- try {
- service.lookup(new HashMap());
- } catch (Exception ex) {
- error = true;
- Assert.assertTrue("The exception was the wrong type", ex instanceof LookupFailureException);
- }
+ assertDoesNotThrow(() -> service.lookup(criteria));
+
+// error = false;
+
+ assertThrows(LookupFailureException.class, () -> service.lookup(new HashMap<>()));
- Assert.assertTrue("An error was not thrown when the input was empty", error);
+// try {
+// service.lookup(new HashMap());
+// } catch (Exception ex) {
+// error = true;
+// Assertions.assertTrue(ex instanceof LookupFailureException, "The exception was the wrong type");
+// }
+//
+// Assertions.assertTrue(error, "An error was not thrown when the input was empty");
Review comment:
It looks like these lines could be removed.
--
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