Set reuseForks to true for unit tests, false for ITs#2007
Set reuseForks to true for unit tests, false for ITs#2007Manno15 merged 2 commits intoapache:mainfrom
Conversation
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
Overall looks good. I recommend creating separate properties for surefire/failsafe, as in #1756, though, so it can be easily disabled.
The main question I have is: is it worth it? What's the difference in the total test runtime with reuseForks turned off and turned on? (mvn clean package runtime)
ctubbsii
added
build
enhancement
Github actions seems to show roughly have the time of completion for the unit-test portion. ~14m down to ~7-8 min. Locally, due to me running the tests in parallel, I only save roughly ~1 min. ~6min to ~5min. |
* Create a profile to be able to set both values from either false or true
|
@Manno15 This change actually broke the build. The exclude/include strategy won't work without renaming the tests or relocating the package they are in, because the maven-failsafe-plugin cannot load test classes from test jars that exist in the same package as classes loaded from the non-test jars, due to jar sealing violations. Unit tests run by maven-surefire-plugin don't have this problem because it doesn't load classes from jars, but from the See https://docs.oracle.com/javase/tutorial/deployment/jar/sealman.html for more info about jar sealing. |
|
@ctubbsii That is my mistake. I will take a look at this next week. My apologies. |
|
No problem. I knew about this possibility, but it didn't occur to me to bring it up before. |
|
@ctubbsii What do you think the most appropriate solution would be? Remove the exclude/include and simply rename the packages to an 'IT' variant or keep the include/exclude just to list the ones that break the build and then either rename or move them? I noticed at least one of the tests will have issues with moving based on a class it depends on. |
|
@Manno15 Either way, in order to avoid jar sealing violations, they will need to be moved to a package (or packages) that does not exist in the regular, non-test jars. If you want to keep the current names, so you have a list of excludes/includes to track, that's fine. But rename or not, they do need to be in different packages if they are being run by failsafe instead of surefire. |
Set reuseForks=false for maven-surefire-plugin and remove includes/excludes added to maven-surefire-plugin and maven-failsafe-plugin as part of #2007 to avoid build failures. These were originally implemented to speed up the build, and the includes/excludes were implemented to force some tests that didn't behave well in a reused fork to run in their own fork as an IT instead of as a unit test. However, that breaks jar sealing and some tests are hard to relocate to a different package without cascading impacts. So, the workaround for these tests are disabled by default until a workaround for those test failures can be addressed in a different way, but can be manually re-enabled by setting the appropriate flag..
|
@Manno15 Okay, so, I did a partial revert, changing the default value for reuseForks to false for surefire, and removing the includes/excludes, just to get things working again, so we can get other PRs merged. If some tests can be renamed to the |
|
I will investigate further. The extra execution of surefire is interesting. Either way, having it configurable is a good place to be in the meantime. Thanks for taking a look and doing the partial revert @ctubbsii |
2 participants
Fixes #2015 .
This sets the reuseForks flag in maven to 'true' for unit tests while keeping it 'false' for ITs. There are some unit tests that fail with this change so they have been excluded directly in the pom file.
Though unit tests pass consistently now, there may be others that are affected by this change and may need to be investigated further.