> On Jan 3, 2022, at 5:30 AM, Volkan Yazıcı <[email protected]> wrote:
>
> Fantastic work Ralph! Please see my comments below:
>
> On Tue, Dec 28, 2021 at 9:39 AM Ralph Goers <[email protected]>
> wrote:
>> Most of the components that were generating test jars have been split
> into two modules - the main component,
>> which only builds the jar, and a -test component that builds a -test jar
> and then runs the unit tests.
>
> For completeness and consistency across all modules *and* to (hopefully)
> mitigate the problem you have mentioned in #2, I think we should place all
> tests to their own modules. Even though we all agree that this is ugly,
> IIRC, our most recent Maven and JPMS research pointed us in this direction.
I would recommend against this for the following reasons:
1. It is very clear which modules create test jars if they are the only ones
that
have a Maven test module.
2. Having a Maven module that has nothing in src/main means you don’t have
an artifact. This module would have to be declared as type “pom” and skip the
deploy step.
3. When the source and unit tests reside in the same Maven module it seems
no module-info.java is required for the tests and everything can be tested on
the module path as a JPMS module. Maven determines whether to populate
the module path and test as a module based on the presence of a
module-info.java
in src/main. If the project has no source I don’t believe it will test it as a
JPMS
module, which is something we very much want to do.
>
> I also propose backporting every "moving tests to a module" change to
> `release-2.x`. Otherwise, it will be really difficult to synchronize
> changes on both branches in the future.
To be honest, I haven’t been able to cherry-pick much of anything across the
branches in quite a while. At this point with the work Matt has been doing on
dependency injection and splitting things out into separate modules almost
everything has problems with cherry-picking.
>
>> 1. log4j-core uses toolchains on MacOS to use Java 17. There is a bug in
> javac on Macs that causes compilation
>> to fail. It is fixed in Java 17 but was not back-ported to Java 11.
>
> For one, I am fine if we set the build (not target!) JDK requirement to
> Java 17 for all platforms to work around this problem. This said, I am
> confused on why we still need Maven toolchains in `master`. Shouldn't we
> simply remove all Maven toolchains configuration and configure
> maven-compiler-plugin and maven-enforcer plugin to require 17 for build and
> target 11?
The default for the build is Java 11. You can’t simply switch the Java version
of
the compilerin the middle of the build without using toolchains.
>
>> 2. In general the test jars have a second compilation step to create the
> module-info.java. Unfortunately, the unit
>> tests cannot be modularized because:
>> a. the unit tests open the main module (i.e. they use the same
> package names).
>> b. the test jar requires the main module.
>> c. the unit tests require the test module.
>> This creates a circularity because the unit tests depend on the test
> module and it requires the same packages
>> as the unit tests.
>
> Will this still pose a problem if we move all tests to their own module?
See my answer above as to why moving all tests to their own module is
a bad idea.
>
>> 3. I tried building entirely with Java 17 but there were some unit tests
> in log4j-core-test that fail. There is one class
>> that is trying to modify a final variable in the Constant class and
> it seems that the technique it is using is no
>> longer valid.
>> 4. log4j-jul must use Junit 4. It seems Junit 5 is hard-wired to use
> java.util.logging and so it isn’t possible to set the
>> system property in time to have the unit tests use Log4j’s LogManager.
>
> Do we have tickets for these two?
I could create a ticket for Junit 5 but I don’t see the point. At this point it
would be a great deal of effort for them to fix it just for us.
Ralph
