-
Notifications
You must be signed in to change notification settings - Fork 542
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
[SUREFIRE-2245] Upgrade to Parent 42 and Maven 3.6.3 #737
Conversation
11da24f
to
6cf9afe
Compare
6cf9afe
to
f4d0f15
Compare
"Requested toolchain specification did not match any configured toolchain: " | ||
+ getJdkToolchain()); | ||
} | ||
tc = tcs.get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guys, please check wether this is correct logically...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks similar to reflection way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but simply hides code complexity. It is has no further use. I wouldn't necessarily delete it.
new MockToolchainManager(expected, null), | ||
mock(MavenSession.class), | ||
emptyMap()); | ||
Toolchain actual = invokeMethod(AbstractSurefireMojo.class, "getToolchain"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have disabled a few tests because they do not work, but I believe that they are redundant now because we don't use any reflection. I am inclined to delete all disabled ones. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete it
@sormuras and @juliette-derancourt: Can you have a look at my changes in:
Maven Parent 42 updated to latest JUnit 1.10.x/5.10.x the use case started to fail because of junit-team/junit5@b41ae69. Mixed dependencies, ABI incompat. My questions to you both:
|
This IT should include a 5.10.2/1.8.5 row for JUnit 5.10.2 and JQwik 1.8.5: maven-surefire/surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformIT.java Line 58 in 19b16d9
And soon one for JUnit 5.11.0 (could already inserted as 5.11.0-M2 ) too.
I don't see the "Mixed dependencies, ABI incompat." in the linked commit. Can you please expound? |
Will do.
When you run the ITs manually from the dir to analyze the failure you see this classpath:
The mix of 1.10.2 and 1.9.3/5.9.3 don't play nice together, if you want I can procude the error for you. I had this signature missing due to the broken CP: |
According to https://junit.org/junit5/docs/current/user-guide/#running-tests-build-maven "it is recommended to use the JUnit Platform BOM to align the versions of all JUnit 5 artifacts." Perhaps the Surefire ITs should follow this advice. Reading on it says:
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<version>5.10.2</version> <!-- can be omitted when using the BOM -->
<scope>test</scope>
</dependency> |
f4d0f15
to
49f0e4b
Compare
Just found another outdated IT: maven-surefire/surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformEnginesIT.java Line 82 in 19b16d9
|
Tried to following:
and I am back to
because of the plugin classpath:
with the fix it is:
|
In lacking a line for
Going from |
IIRC there's some mapping magic in Surefire code that tries to find the correct Platform version based on the Jupiter version used in tests. Perhaps, that magic is outdated too? 🤔 |
That maybe true, but that is out of scope for this issue. |
Did, no change. fails with:
|
@sormuras, here is the magic: Lines 2929 to 3090 in 19b16d9
|
This works:
I'll try that... |
Tried, while it works it gives a different classpath, thus different results. Profile |
Note: With the internal upgrade to JUnit 5.10.x we need to force the junit-platform-engine version in the plugin classpath in some cases otherwise we will have a mismatch. This closes #737
20ed7fa
to
70527c1
Compare
Note: With the internal upgrade to JUnit 5.10.x we need to force the junit-platform-engine version in the plugin classpath in some cases otherwise we will have a mismatch which will lead to undefined behavior. This closes #737
70527c1
to
71d2214
Compare
Following this checklist to help us incorporate your
contribution quickly and easily:
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.
[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles
,where you replace
SUREFIRE-XXX
with the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean install
to make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean install
).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.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.