You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "exceptionfactory (via GitHub)" <gi...@apache.org> on 2023/05/09 04:25:48 UTC
[GitHub] [nifi] exceptionfactory opened a new pull request, #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
exceptionfactory opened a new pull request, #7233:
URL: https://github.com/apache/nifi/pull/7233
# Summary
[NIFI-11532](https://issues.apache.org/jira/browse/NIFI-11532) Removes JUnit 4 and Groovy Test modules from the list of default dependencies in the project root configuration, and also removes the Hamcrest dependency from default dependencies.
These changes highlight the extensive work completed to migrate the vast majority of tests to JUnit 5 and Java, leaving a small number of modules and classes on JUnit 4 and Groovy.
Removing JUnit 4 and Groovy Test dependencies from the list of defaults helps avoid introducing JUnit 4 and Groovy code in new modules. A small number of modules with remaining dependencies on JUnit 4 and Groovy have declared dependencies on the JUnit Vintage Engine and the Groovy Test library.
Additional changes include removing several unnecessary test classes from `nifi-socket-utils`, removing duplicative Registry toolkit tests, and removing the integration test classes from the NiFI Kudu module.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [X] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [X] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [X] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [X] Pull Request based on current revision of the `main` branch
- [X] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [X] Build completed using `mvn clean install -P contrib-check`
- [X] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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 pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #7233:
URL: https://github.com/apache/nifi/pull/7233#issuecomment-1572567158
@MikeThomsen I rebased this PR following several recent test updates. Do you have any additional feedback?
--
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] mattyb149 closed pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 closed pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
URL: https://github.com/apache/nifi/pull/7233
--
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] mattyb149 commented on pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 commented on PR #7233:
URL: https://github.com/apache/nifi/pull/7233#issuecomment-1589910375
Thanks for this! Merging to main
--
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 #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "MikeThomsen (via GitHub)" <gi...@apache.org>.
MikeThomsen commented on PR #7233:
URL: https://github.com/apache/nifi/pull/7233#issuecomment-1542952879
Your reasoning sounds good to me on those points. I'll keep going tomorrow with the 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] exceptionfactory commented on pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #7233:
URL: https://github.com/apache/nifi/pull/7233#issuecomment-1548171034
Rebased to address conflicts with recent changes in main branch.
--
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 diff in pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "MikeThomsen (via GitHub)" <gi...@apache.org>.
MikeThomsen commented on code in PR #7233:
URL: https://github.com/apache/nifi/pull/7233#discussion_r1189191908
##########
nifi-nar-bundles/nifi-kudu-bundle/nifi-kudu-controller-service/pom.xml:
##########
@@ -88,12 +88,6 @@
<artifactId>kudu-client</artifactId>
<version>${kudu.version}</version>
</dependency>
- <dependency>
- <groupId>org.apache.kudu</groupId>
Review Comment:
Why was this removed?
##########
nifi-commons/nifi-security-utils-api/src/test/groovy/org/apache/nifi/security/util/TlsConfigurationTest.groovy:
##########
@@ -1,90 +0,0 @@
-/*
Review Comment:
Why are you removing a bunch of unit tests without Java replacements in this PR? Did I miss that scoped into the ticket?
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-test-utils/pom.xml:
##########
@@ -65,9 +65,9 @@ language governing permissions and limitations under the License. -->
<!-- testcontainers requires elements JUnit 4 (even when running in JUnit 5 mode), expected to be removed in 2.x
see https://github.com/testcontainers/testcontainers-java/issues/970
-->
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <scope>compile</scope>
+ <groupId>org.junit.vintage</groupId>
+ <artifactId>junit-vintage-engine</artifactId>
+ <scope>test</scope>
Review Comment:
IIRC, that was set as compile because it doesn't come in as a transitive dependency when `nifi-elasticsearch-test-utils` is imported by another project.
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-test-utils/pom.xml:
##########
@@ -65,9 +65,9 @@ language governing permissions and limitations under the License. -->
<!-- testcontainers requires elements JUnit 4 (even when running in JUnit 5 mode), expected to be removed in 2.x
see https://github.com/testcontainers/testcontainers-java/issues/970
-->
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <scope>compile</scope>
+ <groupId>org.junit.vintage</groupId>
+ <artifactId>junit-vintage-engine</artifactId>
+ <scope>test</scope>
Review Comment:
If that happens, the tests may silently not run because there's no engine.
--
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] mattyb149 commented on pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 commented on PR #7233:
URL: https://github.com/apache/nifi/pull/7233#issuecomment-1581374282
+1 LGTM, @MikeThomsen do you want another crack at this? If you're good I will merge to main
--
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 pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #7233:
URL: https://github.com/apache/nifi/pull/7233#issuecomment-1543979988
> Your reasoning sounds good to me on those points. I'll keep going tomorrow with the review.
Thanks for the reply @MikeThomsen, feel free to follow up with any questions as you proceed. I pushed an update to correct the scope of `junit-vintage-engine` for Elasticsearch integration tests and confirmed that works as expected.
--
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 diff in pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7233:
URL: https://github.com/apache/nifi/pull/7233#discussion_r1189212766
##########
nifi-nar-bundles/nifi-kudu-bundle/nifi-kudu-controller-service/pom.xml:
##########
@@ -88,12 +88,6 @@
<artifactId>kudu-client</artifactId>
<version>${kudu.version}</version>
</dependency>
- <dependency>
- <groupId>org.apache.kudu</groupId>
Review Comment:
See the comments summarizing the removals.
--
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 diff in pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7233:
URL: https://github.com/apache/nifi/pull/7233#discussion_r1189212662
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-test-utils/pom.xml:
##########
@@ -65,9 +65,9 @@ language governing permissions and limitations under the License. -->
<!-- testcontainers requires elements JUnit 4 (even when running in JUnit 5 mode), expected to be removed in 2.x
see https://github.com/testcontainers/testcontainers-java/issues/970
-->
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <scope>compile</scope>
+ <groupId>org.junit.vintage</groupId>
+ <artifactId>junit-vintage-engine</artifactId>
+ <scope>test</scope>
Review Comment:
Good point, although if that is case, then we should add the dependency to referencing modules, since this module does not appear to have any direction dependencies on JUnit 4. I will look at making some adjustments.
--
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 diff in pull request #7233: NIFI-11532 Remove JUnit 4 and Groovy Test from default dependencies
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7233:
URL: https://github.com/apache/nifi/pull/7233#discussion_r1189211851
##########
nifi-commons/nifi-security-utils-api/src/test/groovy/org/apache/nifi/security/util/TlsConfigurationTest.groovy:
##########
@@ -1,90 +0,0 @@
-/*
Review Comment:
Thanks for the feedback, I put a short explanation for some items in the pull request description, but it is admittedly not described in the Jira issue, so I can certainly adjust the scope one way or the other.
Most of the removals are related directly to JUnit 4 or Groovy Test dependencies, where the tests themselves did not seem worth migrating. If there are some that you think are worth keeping, I can make the adjustments accordingly. To categorize the removals:
1. TLS Configuration and Socket Tests: These tests were limited to checking available TLS versions, primarily for earlier versions of Java 8. With Java 11 as the baseline for the main branch, these tests no longer seem to provide useful checks.
2. Record Utility Tests: These tests were very narrowly scoped with limited methods, and some of the functionality is already covered in other tests
3. Kudu Integration Tests: These tests cover basic uses, but as integration tests, they are not run regularly. [NIFI-11138](https://issues.apache.org/jira/browse/NIFI-11138) outlines potential migration to Testcontainers. These tests require JUnit 4 and since they are narrowly focused, it seems better to rewrite them from scratch using Testcontainers versus building off the existing approach.
4. Registry Properties Tests: With refactoring several versions back, the runtime components are already tested in shared properties classes, so these Registry-specific tests do not add much coverage and even if they were migrated, they should be scaled down significantly.
5. Registry-specific encrypt-config Toolkit Tests: These are Groovy-based tests that also have a lot of overlap with shared encrypt-config code, and shared flow encryption and property encryption modules in nifi-commons.
With that background, I'm open to making adjustments to keep certain tests if you think there is particular value in some cases. I will also update the Jira description to note that some tests should be removed as part of this effort.
--
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