You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by MartinPayne <gi...@git.apache.org> on 2018/06/10 19:41:19 UTC
[GitHub] nifi pull request #2780: NIFI-5289 - Changed nifi-mock junit Dependency to C...
GitHub user MartinPayne opened a pull request:
https://github.com/apache/nifi/pull/2780
NIFI-5289 - Changed nifi-mock junit Dependency to Compile Scope
Resolves NoClassDefFoundError for org.junit.Assert when using nifi-mock.
Thank you for submitting a contribution to Apache NiFi.
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 master)?
- [ ] Is your initial contribution a single, squashed commit?
### 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?
- [ ] 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 travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MartinPayne/nifi upstream/pr/mock-junit-dependency
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2780.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2780
----
commit 9e43ef9a188f5afc717cb505a33b464ddaf43755
Author: Martin Payne <de...@...>
Date: 2018-06-10T18:48:57Z
Changed nifi-mock junit Dependency to Compile Scope
Resolves NoClassDefFoundError for org.junit.Assert when using nifi-mock.
----
---
[GitHub] nifi issue #2780: NIFI-5289 - Changed nifi-mock junit Dependency to Compile ...
Posted by MartinPayne <gi...@git.apache.org>.
Github user MartinPayne commented on the issue:
https://github.com/apache/nifi/pull/2780
@MikeThomsen I'm not too sure what you mean. At the moment you'll get a NoClassDefFoundError unless JUnit 4 happens to be on the test classpath (it won't be on the test classpath if you're using JUnit 5). That's because nifi-mock declares JUnit 4 with a "provided" scope, when there is nothing providing JUnit 4. It should be "compile" scope, because nifi-mock is making use of JUnit assertions in non-test code.
---
[GitHub] nifi issue #2780: NIFI-5289 - Changed nifi-mock junit Dependency to Compile ...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2780
> (it won't be on the test classpath if you're using JUnit 5)
The global declaration for JUnit is JUnit 4.12. If someone
> It should be "compile" scope, because nifi-mock is making use of JUnit assertions in non-test code.
nifi-mock is designed to support JUnit tests and I'm not aware of any components that use JUnit 5.
---
[GitHub] nifi issue #2780: NIFI-5289 - Changed nifi-mock junit Dependency to Compile ...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2780
it should be fine for junit dep to be compile scope for nifi-mock. nifi-mock will always be used under test scope so the transitive compile scoped deps will be ok.
---
[GitHub] nifi issue #2780: NIFI-5289 - Changed nifi-mock junit Dependency to Compile ...
Posted by MartinPayne <gi...@git.apache.org>.
Github user MartinPayne commented on the issue:
https://github.com/apache/nifi/pull/2780
> The global declaration for JUnit is JUnit 4.12. If someone is using v5, they'll have to work around that.
Yes, we're using a workaround at the moment. We have to declare JUnit 4 as a dependency with "test" scope, and configure the Maven Dependency Plugin to force JUnit 4 as used to prevent it from being reported as a declared and unused dependency. I would prefer to have the fix upstream so that we can remove the workaround.
> nifi-mock is designed to support JUnit tests and I'm not aware of any components that use JUnit 5.
I am using JUnit 5, and I imagine over time many other people will be migrating to it too. Code wise there is nothing in nifi-mock which means it shouldn't work with JUnit 5, and I would expect it to work with other test frameworks too.
I've provided a link to a table of Maven dependency scopes to demonstrate that this won't result in JUnit ending up in non-test code. Are there any further concerns about this fix?
---
[GitHub] nifi issue #2780: NIFI-5289 - Changed nifi-mock junit Dependency to Compile ...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2780
+1 LGTM (thanks for the reviews and comments!), I ran a full build and verified that no junit JARs are present anywhere in the NiFi assembly. Thanks for the improvement! Merging to master
---
[GitHub] nifi issue #2780: NIFI-5289 - Changed nifi-mock junit Dependency to Compile ...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2780
we need to ensure this change doesnt make this lib get included all over the place as part of the build. I dont understand why it should not have been test but compile seems wrong..
---
[GitHub] nifi issue #2780: NIFI-5289 - Changed nifi-mock junit Dependency to Compile ...
Posted by MartinPayne <gi...@git.apache.org>.
Github user MartinPayne commented on the issue:
https://github.com/apache/nifi/pull/2780
@joewitt Compile scope is the correct scope in this case. If JUnit 4 was only used in the tests for NiFi Mock, test scope would be correct. However, the [NiFi Mock code uses JUnit 4 as a compile time dependency](https://github.com/apache/nifi/blob/f8466cb16d6723ddc3bf5f0e7f8ce8a47d27cbe5/nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java#L74), so JUnit 4 needs to be brought into consuming projects as a transitive dependency.
As per the [Maven dependency scope table](https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Scope), making it a compile scope dependency means it is added to the test classpath of a consuming project if that project declares NiFi Mock with test scope. It would only get included all over the place if consuming projects have declared NiFi Mock with a compile or runtime scope. If that was the case here, NiFi Mock would also be getting included all over the place.
---
[GitHub] nifi pull request #2780: NIFI-5289 - Changed nifi-mock junit Dependency to C...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2780
---