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

[MNG-7818] Removed exclusion of hamcrest from junit 4 #1178

Merged
merged 1 commit into from
Jun 23, 2023
Merged

[MNG-7818] Removed exclusion of hamcrest from junit 4 #1178

merged 1 commit into from
Jun 23, 2023

Conversation

lprimak
Copy link

@lprimak lprimak commented Jun 19, 2023

Fixes https://issues.apache.org/jira/browse/MNG-7818

Removed exclusion of hamcrest from JUnit 4

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MNG-XXX] SUMMARY,
    where you replace MNG-XXX and SUMMARY with the appropriate JIRA issue.
  • Also format the first line of the commit message like [MNG-XXX] SUMMARY.
    Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@lprimak lprimak changed the title Fixes https://issues.apache.org/jira/browse/MNG-7818 [MNG-7818] Removed exclusion of hamcrest from junit 4 Jun 19, 2023
@cstamas
Copy link
Member

cstamas commented Jun 21, 2023

Nope, this result in "mixed" dependencies: excerpt from maven-core dep tree:

...
[INFO] +- org.hamcrest:hamcrest:jar:2.2:test
[INFO] \- junit:junit:jar:4.13.2:test
[INFO]    \- org.hamcrest:hamcrest-core:jar:1.3:test

Copy link
Member

@cstamas cstamas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results in "mixed" deps, so is a no go.

Copy link
Member

@cstamas cstamas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, i meant "no go" 😄

@lprimak
Copy link
Author

lprimak commented Jun 21, 2023

Oh I can fix that :)

@lprimak lprimak requested a review from cstamas June 22, 2023 01:27
@lprimak
Copy link
Author

lprimak commented Jun 22, 2023

Test failures are the "locking" issues, not sure how to clean maven repo in GH actions :(

<scope>test</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>${junitVersion}</version>
<scope>test</scope>
<exclusions>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you actually keep the exclusion to not pull older version of hamcrest that conflicts with the one you defined on the top level?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. <dependencyManagement> section will take precedence and correctly pull in the newer version, as desired.
However, it won't pull it in unless required (also correct)
It's not desirable to have hamcrest included unless required (as in JUnit 5)

@cstamas
Copy link
Member

cstamas commented Jun 22, 2023

Test failures are the "locking" issues, not sure how to clean maven repo in GH actions :(

You mean the IT MNG-7819 ? As that will cleared up once Resolver 1.9.13 released and updated in Maven.
Frankly, I "merged too early" the IT (as vote is ongoing for one more day), but I did not expect any other 3.9.x change as according to JIRA is "done" (resolver update pending only), sorry for messing up.

@cstamas
Copy link
Member

cstamas commented Jun 22, 2023

From test-arq-suite branch JUnit4 w/ removed workaround (locally built maven-3.9.x this PR + master shrinkwap-resolver + test-arg-suite, all updated with corresponding snapshots):

[DEBUG]    junit:junit:jar:4.13.2:test
[DEBUG]       org.hamcrest:hamcrest-core:jar:2.2:test (scope managed from compile) (version managed from 1.3)
[DEBUG]          org.hamcrest:hamcrest:jar:2.2:test (scope managed from compile) (version managed from 2.2)

So, what happens, is that by placing hamcrest BEFORE junit in depMgt, the hamcrest will "overrule" the 1.3 from junit, and 2.2 (empty jar) depends on hamcrest:jar 2.2, so all good.

Copy link
Member

@cstamas cstamas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go IMHO, we may or we may not wait for resolver 1.9.13 (to get rid of one failing IT)

@cstamas cstamas merged commit b050257 into apache:maven-3.9.x Jun 23, 2023
@elharo
Copy link
Contributor

elharo commented Jun 23, 2023

Not sure if this is related but I see some warnigns in this area:

[INFO] <<< maven-dependency-plugin:3.6.0:analyze (default-cli) < test-compile @ maven-api-meta <<<
[INFO] 
[INFO] 
[INFO] --- maven-dependency-plugin:3.6.0:analyze (default-cli) @ maven-api-meta ---
[WARNING] Unused declared dependencies found:
[WARNING]    org.junit.jupiter:junit-jupiter-engine:jar:5.9.1:test
[WARNING]    org.hamcrest:hamcrest-core:jar:2.2:test
[INFO] 

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.

4 participants