ascheman commented on code in PR #3392:
URL: https://github.com/apache/maven-surefire/pull/3392#discussion_r3613824886
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/ModularClasspathForkConfiguration.java:
##########
Review Comment:
Good point — added Javadoc for `createArgsFile` including the `patchFile`
semantics in d05524c7f.
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/ModularClasspathForkConfiguration.java:
##########
@@ -184,6 +187,16 @@ File createArgsFile(
.append('"')
.append(NL);
+ // Check for module-info-patch.args generated by
maven-compiler-plugin 4.x
+ File patchArgs = findModuleInfoPatchArgs(patchFile);
+ if (patchArgs != null) {
+ // Merge the file's directives, except
--add-reads/--add-modules
+ // which surefire manages itself (see
appendModuleInfoPatchArgs)
+ appendModuleInfoPatchArgs(args, patchArgs, moduleName);
+ }
+
+ // Always auto-generate --add-opens for test packages (JUnit
needs reflection access).
+ // module-info-patch.maven cannot use ALL-UNNAMED in
add-opens, so surefire handles this.
Review Comment:
Agreed with the direction, one empirical addition: the constraint is not
only on the test methods but already on the test classes. The common JUnit 5
idiom uses package-private test classes, and `--add-exports` only grants access
to public types — the engine then needs `setAccessible` (i.e. `--add-opens`)
even for public methods in such classes, so exports-only as default would break
that idiom.
This becomes actionable in the #3090 implementation (in progress, stacked on
this PR and #3394), where the `--add-opens` generation already moved to the
mojo side: I would add a configuration option along the lines of
`testModuleAccess` = `opens` | `exports` | `none`. Given the JUnit 5 idiom I
lean towards `opens` as the default — happy to follow your preference there.
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/ModularClasspathForkConfiguration.java:
##########
@@ -184,6 +187,16 @@ File createArgsFile(
.append('"')
.append(NL);
+ // Check for module-info-patch.args generated by
maven-compiler-plugin 4.x
+ File patchArgs = findModuleInfoPatchArgs(patchFile);
+ if (patchArgs != null) {
+ // Merge the file's directives, except
--add-reads/--add-modules
+ // which surefire manages itself (see
appendModuleInfoPatchArgs)
+ appendModuleInfoPatchArgs(args, patchArgs, moduleName);
+ }
+
+ // Always auto-generate --add-opens for test packages (JUnit
needs reflection access).
+ // module-info-patch.maven cannot use ALL-UNNAMED in
add-opens, so surefire handles this.
Review Comment:
Yes — exports vs. opens becomes exactly that configuration choice in #3090,
and surefire keeps generating one of the two for the test packages (they are
not in `module-info-patch.args` since compilation does not need them, as you
say).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
