-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Extract "validation" tests into separate modules #711
Conversation
@marchof could you please have a look on these changes? if all fine, I will also update documentation. As expected individual profiles are now shorter (#709 (comment)), but there are anyway some duplication in |
907f14b
to
0be0b88
Compare
@Godin Thanks for taking up this effort! I wonder how the dependencies should look like: Currently the base infrastructure is part of module |
7c774cd
to
62e98e8
Compare
62e98e8
to
8b3cd7f
Compare
Good point - done.
You mean to run
Indeed will be better to do after this PR in order to unblock addition of tests for Kotlin. I added update for documentation, so could you please do re-review? |
@Godin Thanks for fixing dependencies!
This is what I wanted to propose. Separate PR fine to me as this would be a new feature anyways. |
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.
Hi @Godin I added some questions to the change set.
org.jacoco.build/pom.xml
Outdated
@@ -708,37 +709,67 @@ | |||
</build> | |||
</profile> | |||
|
|||
<!-- This profile is used to launch tests with compilation into specific bytecode version. --> | |||
<!-- --> |
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.
Is this empty comment on purpose?
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.
Yep - for visual separation of profile that follows this comment from one that is above.
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.
Some profiles are separated but some are not. I assume these are groups of profiles, right? Maybe add a short description into the comment for the profile group like with the profiles at the top?
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 added comments.
org.jacoco.build/pom.xml
Outdated
@@ -776,12 +809,14 @@ | |||
</build> | |||
</profile> | |||
|
|||
<!-- --> |
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.
Empty comment?
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.
Yep - for visual separation of profiles that follow this comment from ones that are above.
org.jacoco.core.test.java7/pom.xml
Outdated
|
||
<artifactId>org.jacoco.core.test.java7</artifactId> | ||
|
||
<!--<name></name>--> |
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.
Commented tag? Remove?
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 was wondering if we want to assign name for consistency with the rest? If omitted, then name is assumed to be equal to artifactId
and here is how it looks currently:
...
[INFO] JaCoCo :: Test :: Core [jar]
[INFO] org.jacoco.core.test.java5 [jar]
...
[INFO] JaCoCo :: Test :: Core ............................. SUCCESS [ 0.039 s]
[INFO] org.jacoco.core.test.java5 ......................... SUCCESS [ 0.021 s]
...
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.
Right, then the new modules should also have proper names. What about this?
JaCoCo :: Test :: Core :: Integration Java 5
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.
Done.
org.jacoco.core.test.java7/pom.xml
Outdated
<!--<name></name>--> | ||
|
||
<properties> | ||
<jacoco.includes>org.jacoco.core.*</jacoco.includes> |
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 would prefer to not record coverage for integration tests at all. In this case this property is not relevant.
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.
They are not included into final report.
All classes are included into instrumentation and agent is enabled by default - see
jacoco/org.jacoco.tests/pom.xml
Lines 49 to 70 in 753d653
<plugin> | |
<groupId>org.jacoco</groupId> | |
<artifactId>jacoco-maven-plugin</artifactId> | |
<version>${project.version}</version> | |
<configuration> | |
<exclClassLoaders>sun.reflect.DelegatingClassLoader:org.jacoco.core.test.TargetLoader:org.jacoco.core.test.InstrumentingLoader</exclClassLoaders> | |
<sessionId>${project.artifactId}</sessionId> | |
<includes> | |
<include>${jacoco.includes}</include> | |
</includes> | |
<excludes> | |
<exclude>${jacoco.excludes}</exclude> | |
</excludes> | |
</configuration> | |
<executions> | |
<execution> | |
<goals> | |
<goal>prepare-agent</goal> | |
</goals> | |
</execution> | |
</executions> | |
</plugin> |
Thus if we want to exclude all classes from instrumentation, then this should be replaced by jacoco.excludes
? Or if we want to completely disable agent, then jacoco.skip
?
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.
Yep, I think JaCoCo agent should not be in the game for integration tests. Targets are instrumented programmatically anyways during the test. So jacoco.skip
would be the best fit here (and jacoco.includes
can be removed at all).
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.
Replaced jacoco.includes
by jacoco.skip
.
org.jacoco.tests/pom.xml
Outdated
<module>../org.jacoco.report.test</module> | ||
<module>../org.jacoco.agent.rt.test</module> | ||
<module>../org.jacoco.agent.test</module> | ||
<module>../org.jacoco.ant.test</module> | ||
<module>../org.jacoco.cli.test</module> | ||
<module>../org.jacoco.examples.test</module> |
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.
Why do we need to move these tests?
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.
Because otherwise they will be executed before modules from profiles.
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.
Sorry for my stupid question: And why would this be a problem?
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.
Because org.jacoco.examples.test
and especially jacoco-maven-plugin.test
are the slowest ones, while perform tests on a way higher level than org.jacoco.core.test.*
.
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.
So you mean like "fail fast"? Where is that important? For local develop/test iterations I mostly run specific modules only anyways. For our CI all tests are executed anyways.
I just find it rather confusing that these modules are now included in specific JDK profiles.
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.
Re module names: For consistency we should also adjust Java package names to the new module names.
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 added org.jacoco.core.test.validation
as parent for org.jacoco.core.test.validation.*
Not sure that I like that amount of folders in root. Maybe later we can think of grouping like for example
validation
|- pom.xml
|- java5
| \- pom.xml
|- java7
| \- pom.xml
\- java8
\- pom.xml
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.
Absolutely! The same for the unit tests. We could simplify the structure if we could move to a more Maven standard project layout. But again... separate issue.
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.
We could simplify the structure if we could move to a more Maven standard project layout.
In truly standard Maven layout, unit tests are located in the same module as main code, but we can't do this because we need to execute JaCoCo for JaCoCo itself - see #312 (comment)
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 know 😉
@Godin Do we still need the constructor |
@Godin Can we add Eclipse configs to the new projects? At least as "plain project" so it can be easily imported. |
Removed. |
@marchof you know that for Java development I mostly use IntelliJ IDEA and nowadays don't use even it, because mostly do development in C++ 😉 I added files, hopefully with correctly linked settings and correctly specified JRE. Could you please check import into IDE? |
@Godin Thanks for the Eclipse settings! I can now import but not compile because the shared settings declare |
@marchof about import into Eclipse IDE - I was thinking about Oomph |
</toolchains> | ||
</pre> | ||
|
||
<p> | ||
Now you should be able to execute maven build with specified version of JDK: | ||
to execute compilation and tests using JDK 5 or 6: |
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.
Shouldn't that be moved after the next section ("Location of toolchains.xml ...")?
Also should start with upper case: "To execute ..."
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.
Here is how this currently looks/reads to me:
You can set property jdk.version to use JDK specified in Maven Toolchains with respective value of version tag. For example you can create following
~/.m2/toolchains.xml
file
...
to execute compilation and tests using JDK 5 or 6:mvn clean install -Djdk.version=5 mvn clean install -Djdk.version=6
Notice that above we refer to default location of toolchains.xml
, after that show that non-default can be specified:
Location of toolchains.xml can be set via an option:
mvn --toolchains path clean install -Djdk.version=version
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.
So for me the page currently has the following structure
- Information about toolchain
- Information how to run the build for specific JDK versions
- Information about toolchain
- Information how to run the build for specific JDK versions
Or do we need a separate toolchain.xml
for step 4? The this should be noted.
@Godin Oomph might be helpful here. But let's fix this separate from this PR. |
@@ -0,0 +1,30 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<projectDescription> | |||
<name>org.jacoco.core.test.java5</name> |
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.
Should now read org.jacoco.core.test.validation.java5
. Same issue with the other new org.jacoco.core.test.validation.*
projects.
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.
arrrrrr
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.
Fixed.
6fed97f
to
0905d24
Compare
Before/Currently packages |
53488f7
to
efa8e21
Compare
efa8e21
to
36ef13f
Compare
@Godin Regarding package naming: Probably it doesn't make sense any more to distinguish between "filter" and "non filter" as validation tests should verify the overall functionality in respect to different language feature. Therefore I would propose to use two packages only per module:
Side note: Some tests should probably be moved to |
6d79b09
to
33a5636
Compare
d12f993
to
f5cf359
Compare
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.
The updated documentations is great! Let's merge now.
I'll work on separate PRs to organize test code package names.
Defaults
org.jacoco.core.test.validation.java5
now contains what previously was inorg.jacoco.core.test/src/org/jacoco/core/test/(validation|filter)
org.jacoco.core.test.validation.java7
what previously was inorg.jacoco.core.test/src-java7
org.jacoco.core.test.validation.java8
what previously was inorg.jacoco.core.test/src-java8
Previously manual configuration was needed for inclusion of the last two sources into build and to open them in IntelliJ IDEA. Now by default all modules included into build and so showup in IntelliJ IDEA.
By default the last two will be compiled into bytecode version 51 (Java 7) and 52 (Java 8) respectively, all other into 49 (Java 5).
And build is simple as
Should be noted that JDKs 9 - 12 can't compile into bytecode versions lower than 50 (Java 6) - see JEP-182. So when they are used modules
org.jacoco.core.test.java7
andorg.jacoco.core.test.java8
will be compiled as above, however all other modules will be compiled into bytecode version 50 (Java 6):And JDK 12 soon will stop compiling into bytecode versions lower than 51 (Java 7) - see JDK-8028563, from which seems that change will be in build right after EA b2 (jdk-12+2) that is currently available.
Previously manual configuration was required to do build with JDK version greater than 8.
Also could be noted that builds with JDK greater than 10 still fail with the same reasons as before, which are unrelated to these changes.
Property
jdk.version
Property
jdk.version
can be specified to compile and run tests using JDK that is different from current.As before location of JDK will be selected from Maven
toolchains.xml
with respective value ofversion
tag, however now values do not have1.
prefix.As before JDK 5 will be used for snapshots and releases.
When value of this property is
5
,6
,7
or8
, defaults are the same as described above for JDK 8,when value is
9
,10
,11
or12
, defaults are the same as described above for JDKs 9 - 12:Additionally, when value of this property is
5
or6
, modulesorg.jacoco.core.test.java7
andorg.jacoco.core.test.java8
will be excluded:When value is
7
, moduleorg.jacoco.core.test.java8
will be excluded:Property
bytecode.version
Property
bytecode.version
can be specified to compile modules into bytecode version different from their defaults described above.Now values of this property should not have
1.
prefix.Modules
org.jacoco.core.test.java7
andorg.jacoco.core.test.java8
will be excluded when value of this property is5
or6
:Module
org.jacoco.core.test.java8
will be excluded when value of this property is7
:Additionally, it can be combined with property
jdk.version
:Also should be noted that
maven-shade-plugin
andmaven-plugin-plugin
can't process bytecode versions 54 (Java 10), 55 (Java 11) and 56 (Java 12). So when value of this property is10
,11
or12
, only test modules (org.jacoco.*.test.*
) will be compiled into specified bytecode version and others will be compiled into 53 (Java 9):Property
ecj
Property
ecj
can be specified to compile all modules using ECJ.For the time being with ECJ we compile all modules into bytecode version 52 (Java 8) unconditionally (and we were testing only this configuration in Travis even before these changes), however it can be combined with properties
bytecode.version
andjdk.version
for selection of modules and JDK for execution of tests: