Add JPMS module test #2685
Add JPMS module test #2685
Conversation
Previously the build would not have detected if `module-info.class` was missing, or incorrectly configured.
Defines properties in the parent POM which determine whether the plugins should be skipped, and then sets the properties in the Maven modules.
Marcono1234
force-pushed
the
marcono1234/jpms-module-test
branch
from
1ef331c
to
f141d13
Compare
| * | ||
| * <blockquote> | ||
| * | ||
| * Can't compile test sources when main sources are missing a module descriptor |
There was a problem hiding this comment.
Maybe there is another way to work around this; the Maven Compiler Plugin checks if the main output directory exists, but maybe that check is a bit brittle?
https://lists.apache.org/thread/c3f71v8tf3643dto6bwf3ctcos4gt5cq suggests it might even depend on the JUnit version being used (?).
So for now having this dummy module declaration might be the most reliable workaround.
| <module>jpms-test</module> | ||
| <module>graal-native-image-test</module> | ||
| <module>shrinker-test</module> |
There was a problem hiding this comment.
Maybe it would make sense to change the names of these modules to be test-... instead of ...-test so that they are grouped in the file system, and it is more obvious that they are tests.
What do you think?
There was a problem hiding this comment.
I think that would be an excellent idea for a follow-up PR.
There was a problem hiding this comment.
Have created #2691
| "com.google.gson.reflect", | ||
| "com.google.gson.stream"); | ||
| // Gson currently does not export packages to specific modules only | ||
| assertThat(packageExports.stream().filter(Exports::isQualified)).isEmpty(); |
There was a problem hiding this comment.
I think noneMatch would be a bit more succinct.
There was a problem hiding this comment.
Do you mean assertThat(stream.noneMatch(...)).isTrue()? (Or is there a Truth method I am overlooking?)
The disadvantage might be that if there are actually matching packages, the assertion failure message would just say that true was expected but it was false, whereas this approach should (hopefully) produce a more helpful assertion failure message.
There was a problem hiding this comment.
Yes, good point. It's better the way it is.
| <module>jpms-test</module> | ||
| <module>graal-native-image-test</module> | ||
| <module>shrinker-test</module> |
There was a problem hiding this comment.
I think that would be an excellent idea for a follow-up PR.
I tried the approach described in https://github.com/google/gson/blob/main/ReleaseProcess.md#testing-maven-release-workflow-locally (except for GPG signing), and it looks like everything was still working. So I hope this shouldn't break anything. But yes, in the worst case we notice it for the next release... (or hopefully earlier) (Also sorry that I had missed that the GPG plugin was not disabled for the graal-native-image-test module, which you then fixed in #2675.) |
Purpose
Adds a Maven module to test Gson's
module-info.classDescription
Previously the build would not have detected if
module-info.classwas missing, or incorrectly configured. Therefore this pull request adds a new Maven module to test using themodule-info.class.The new integration tests might not work when run from the IDE, but they will work when run with
mvn ...from the command line.Please let me know if you think it is too verbose, if the test should cover additional cases, or if there might be a better way to implement this. Any feedback is appreciated.
To simplify disabling certain Maven plugins (e.g. install, gpg and deploy) for the integration test modules I have instead added two properties in the parent project which can disable the plugins. See the second commit.
I have done a short test of the release workflow (but without gpg), and everything still seems to work as desired: The build succeeds and only
gson-parentandgsonare deployed.Checklist
This is automatically checked by
mvn verify, but can also be checked on its own usingmvn spotless:check.Style violations can be fixed using
mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.null@since $next-version$(
$next-version$is a special placeholder which is automatically replaced during release)TestCase)mvn clean verify javadoc:jarpasses without errors