Move ITs to JDK 8 / JUnit 5 #176
Conversation
|
Woosh |
|
There is one problem I see and this we have discussed with @mthmulders already. If we move IT to Java 8, then we officially drop everything below 3.9.0 because it cannot be tested otherwise. |
It should be possible to setup a branch of this project and use that one to test maven on JDK 7. This would eventually mean back porting some tests on those two branches when the fixes are back ported to maven 3.8.x or older versions. |
|
There is also another thing: if we do this, we can throw out all IT pre 3.9.0 because they would not run anyway |
I don't see why. If they run on JDK 7, they will run on JDK 8 with maven 3.9.0. |
|
No, I mean if this is now Java 8 and only 3.9.0 supports its I see no need to keep ITs which ate skipped anyway. |
Ah, yes, this definitely gives the opportunity to clean up things a bit. |
|
Or maybe we should have two Maven modules: one for the tests up to 3.9 (running with Java 1.7) and one for 3.9 onward (running Java 8)? |
What do you mean by two modules? |
We could, but this kinda defeat the purpose which was to upgrade things a bit (my goal was mainly the junit upgrade, the JDK is actually just a consequence of it). I think it would be easier to create a branch, raise the version on master to |
|
I agree here with @gnodet . Branch off maven-3.8.x and move master forward. |
Create two submodules with integration tests (where we now have one).
Absolutely. Let's drop the idea immediately :-) |
Drop it whilte it's hot. |
|
awesome changes! |
core-it-suite/src/test/java/org/apache/maven/it/IntegrationTestSuite.java
Outdated
Show resolved
Hide resolved
core-it-suite/src/test/java/org/apache/maven/it/IntegrationTestSuite.java
Outdated
Show resolved
Hide resolved
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
Show resolved
Hide resolved
gnodet
force-pushed
the
jdk8-junit5
branch
5 times, most recently
from
c6d3e23
to
5cb3d85
Compare
|
In this spirit, the master shall be updated to 36 and POMs cleaned out, no? |
|
While at least now all ITs pass, I constantly see this: dumps and Any idea? I am on: Same for "11.0.14" and "17.0.2" |
|
@michael-o I have the same problem locally (osx + 3.9.0-SNAPSHOT), and it also happens on GitHub, so this seem to happen on all environments. I'll try to investigate tomorrow. For the cleanup, I agree, we also need to remove all tests which are not executed on maven >= 3.9.x. But maybe another subsequent PR would be better for more cleanup ? |
Yes, separate PR after this has been stabilized and merged. |
There was a problem hiding this comment.
Old (master):
[INFO] Tests run: 864, Failures: 0, Errors: 0, Skipped: 13, Time elapsed: 566.069 s - in org.apache.maven.it.IntegrationTestSuite
new:
[WARNING] Tests run: 859, Failures: 0, Errors: 0, Skipped: 33
- I wonder why there more more skips now?
- Where does the 5 tests diff come from?
- The overall elapsed time is gone :-(
core-it-suite/src/test/java/org/apache/maven/it/TestSuiteOrdering.java
Outdated
Show resolved
Hide resolved
Unreferenced tests are automatically added at the beginning, but it looks more coherent to add them anyway
New master is now: Note that with the previous setup, I don't think tests were counted as skipped if the maven version was not matching the range. |
I agree with the last statement. I have noticed this too. Will review again. |
As for the overall output (including the wall-time of all tests), this would require a surefire extension in order to rework the output completely as it was before. I'm not sure it's worth it at this point. |
OK, then we can postpone this. Would nice to have at some point in time. |
|
Looks much better now. I have one failure I need to analyze and will get back to you. |
|
Tackled another one... |
|
I went now through all skipped tests, disabled ones aren't counted -- good.
This one we missed to remove. Needs another PR.
These are all skipped for some reason, though they are valid tests...please investigate/reproduce. Original output: |
The resources from 5889 are used by 6223 which has a range of "[3.5.1,)". I did not want to mess things in the previous PR, so I decided to keep it disabled for now. I can create a PR to clean those two tests later.
For some lines, the number of skipped tests is really weird, as it is higher than the actual number of tests. For example the 7474 test is supposed to only have a single test, not 7. |
Agree, a proper cleanup in a separate PR is necessary.
Let me doublecheck... |
|
Double checked, it was indeed a bad setup, it looks much better now, but still have these and the Surefire XML contains interleaving tests:
Totallly fishy... |
|
Wrong test count reporting can be an issue of surefire ... |
|
@Tibor17 Any idea why the surefire XML contains totally unrelated tests? |
|
Guys, this issue has already been reported: https://issues.apache.org/jira/browse/SUREFIRE-2032 |
There was a problem hiding this comment.
Since we know now has the appearance is a Surefire issue, @gnodet Please add an appropriate comment in core-it-suite/src/test/java/org/apache/maven/it/TestSuiteOrdering.java.
Moreover, this should have a JIRA issue as well with a brief description of the purpose.
When this is done, from my PoV, a squash, summary change and merge is good.
|
|
||
| /** | ||
| * This is a test set for <a href="https://issues.apache.org/jira/browse/MNG-3038">MNG-3038</a> | ||
| * | ||
| * @author Joakim Erdfelt | ||
| * | ||
| */ | ||
| @Disabled( "cannot reproduce" ) |
There was a problem hiding this comment.
I suppose that now is working with junit 4, maybe TODO remarks to verify later
This requires apache/maven-verifier#31