Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #78 ARQ-1944 Avoid packaging hamcrest in arquillian-junit.jar #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iwein
Copy link

@iwein iwein commented Oct 27, 2016

Short description of what this resolves:

This patch removes the hamcrest classes from arquillian-junit.jar. This way users can chose their own hamcrest version and avoid linkage errors when running with Tomcat. See #78

Changes proposed in this pull request:

  • remove hamcrest from arquillian-junit.jar

Fixes #98, ARQ-1944

This change will not be strictly backwards compatible because users will be required to add the hamcrest dependency explicitly.

…it.jar

This way Arquillian tests can use hamcrest matchers, without getting a LinkageError.

This change will not be strictly backwards compatible because users will be required to add the hamcrest dependency explicitly.
@bartoszmajsak
Copy link
Member

Many thanks for your contribution!

I'm just wondering if such a simple removal is really what we want here. This might cause quite a bit of issues for tests which are already realying on built-in hamcrest (which I believe is shipped with junit as transitive dependency).

I could think of doing it a bit differently - introducing a configuration entry exclude_hamcrest and extend the logic of the JUnitDeploymentAppender to respect this setting. Defaults to true to preserve old behaviour.

WDYT?

@iwein
Copy link
Author

iwein commented Nov 1, 2016

@bartoszmajsak I decided to go the same route that Kent Beck did with JUnit. JUnit used to package hamcrest in their jar, and dropped that in 4.10(?) because of the depencency problems this caused in maven projects.

My thinking was that if JUnit can get away with such a breaking change, arquillian can too. The extra complexity is more harmful than the fact that people would have to change their dependencies when upgrading arquillian.

@bartoszmajsak
Copy link
Member

WDYT @aslakknutsen? Shall we do the same?

@iwein
Copy link
Author

iwein commented Feb 15, 2017

Is there any news on this PR @aslakknutsen @bartoszmajsak? I'm now running a fork, but I prefer to do things properly.

@Thadir
Copy link

Thadir commented Aug 14, 2017

Wat is the state of this pull request?

@bartoszmajsak
Copy link
Member

In this shape, it's not what we would confidently ship as next version. Conditional excluding as suggested in #113 (comment) sounds more feasible I believe.

@starksm64
Copy link
Member

this is now dated, but as of the latest junit4 junit:junit:jar:4.13.2, hamcrest is still an included dependency and this is where arquillian is picking up the hamcrest dependency from. If I look at the dependency tree in the testenrichers/ejb submodule that does make use of hamcrest, this is the output:

[INFO] org.jboss.arquillian.testenricher:arquillian-testenricher-ejb:jar:1.7.3.Final-SNAPSHOT
[INFO] +- org.jboss.arquillian.test:arquillian-test-spi:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  +- org.jboss.arquillian.core:arquillian-core-spi:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  |  \- org.jboss.arquillian.core:arquillian-core-api:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  \- org.jboss.arquillian.test:arquillian-test-api:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] +- org.jboss.arquillian.container:arquillian-container-test-spi:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  +- org.jboss.arquillian.container:arquillian-container-spi:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  |  +- org.jboss.arquillian.config:arquillian-config-api:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  |  +- org.jboss.arquillian.config:arquillian-config-impl-base:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  |  |  +- org.jboss.arquillian.config:arquillian-config-spi:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] |  |  |  \- org.jboss.shrinkwrap.descriptors:shrinkwrap-descriptors-spi:jar:2.0.0:compile
[INFO] |  |  \- org.jboss.shrinkwrap.descriptors:shrinkwrap-descriptors-api-base:jar:2.0.0:compile
[INFO] |  \- org.jboss.arquillian.container:arquillian-container-test-api:jar:1.7.3.Final-SNAPSHOT:compile
[INFO] +- org.jboss.shrinkwrap:shrinkwrap-impl-base:jar:1.2.6:test
[INFO] |  +- org.jboss.shrinkwrap:shrinkwrap-api:jar:1.2.6:compile
[INFO] |  \- org.jboss.shrinkwrap:shrinkwrap-spi:jar:1.2.6:test
[INFO] +- junit:junit:jar:4.13.2:test
[INFO] |  \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] \- org.jboss.spec.javax.ejb:jboss-ejb-api_3.1_spec:jar:1.0.2.Final:provided

A simple exclusion property could still be added for use in JUnitDeploymentAppender

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants