[SUREFIRE-1909] Replace --add-exports with --add-opens

[NilsRenaud]

This PR follows the discussion on the associated Jira issue : https://issues.apache.org/jira/browse/SUREFIRE-1909

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

[Tibor17]

Hi @NilsRenaud ,

I have approved the CI. Now it is running. If it would pass successfully, we should obtain a new distinct integration test from you related to your use case. Would you have some spare time to write one? Thx

[Tibor17]

@NilsRenaud
If you like adding a new config param, pls let's discuss the Javadoc with text and purpose of the parameter in prior of coding something. Thx

[Tibor17]

Pls name the title of the PR, Jira and commit same.

[eolivelli]

Lgtm

Thanks

[Tibor17]

@NilsRenaud
Is it this IT maven-multimodule-project-with-jpms you are looking for?

[Tibor17]

@NilsRenaud
The open appears in the documentation and the integration test. If you remove the keyword open, would the test work? Pls try.

[NilsRenaud]

@Tibor17 :

I have approved the CI. Now it is running. If it would pass successfully, we should obtain a new distinct integration test from you related to your use case. Would you have some spare time to write one? Thx

Yes, I've updated the PR with a new integration test using this new option (not enabled by default as advised by @sormuras

If you like adding a new config param, pls let's discuss the Javadoc with text and purpose of the parameter in prior of coding something. Thx

Oops. I saw you message a bit too late. But I kept these changes in a separate commit for now, feel free to comment my code with your advices

Pls name the title of the PR, Jira and commit same.

I'll squash my commits in a single one with a correct name after your reviews, OK ?

Is it this IT maven-multimodule-project-with-jpms you are looking for?

Nop, it's not related to multi modules, I've created a dedicated test with a single module tested by a non modularised tests

The open appears in the documentation and the integration test. If you remove the keyword open, would the test work? Pls try.

It does not work without the open keyword. Since the tests are running in a module, I'm not sure it uses the same underlying ForkConfiguration though, but I've not investigated.

I've added an integration test and a parameter usable directly from the XML file in the last version of this PR. Let me know what you think about it.

[Tibor17]

Why we should not stick only to the directive to --add-open? The relation is between surefire libraries (engine if src/test/java does not have module descriptor) and the tests. Is it somehow harmful?

--add-opens
surefire.jpms.bug/surefire.test=ALL-UNNAMED

[NilsRenaud]

I tend to agree with you @Tibor17, but @sormuras said that this drop in replacement could "break other use cases".

@sormuras, could you give us an example ?

[sormuras]

"Could" as in might. I have no concrete example at hand.

My assumption was that it affects user test code and their expectations regarding modular boundaries. The proposed change replaces one directive with another directive. Is the new directive the one users want(ed)?

[Tibor17]

@NilsRenaud
@sormuras
IIUC the directive --add-opens surefire.jpms.bug/surefire.test=ALL-UNNAMED creates two alternatives as a contract between

1. surefire libraries and tests (not the junit engine) if src/test/java/module-path.java exists - the user has to obey the libraries surefire-booter and surefire-platform-provider which cannot be excluded from having this priviledge. The user has to trust it if he uses surefire/failsafe.
2. surefire libraries + junit engines and the tests if src/test/java/module-path.java does not exist - the test world excluding src/main behaves as the same as if it was a pure classpath where reflection was used for years. Here are two worlds existing in parallel. The user still can follow (1) by adding the test module descriptor.

[NilsRenaud]

I'm sorry @Tibor17 , I'm not sure to understand your point.

There is currently 2 strategies working with this PR :

• The featured example of JPMS with JUnit 5, ie : to have an "open" module-info.java for the /src/test/ using a different package name as the main package). In this case there is no need of --add-opens option. But package-private classes are not reachable during test.
• Not to have a module-info.java set for src/test, and to have the tests using the same package names as main code, then to --patch-module with the compiled tests at runtime and eventually to --add-opens the test packages (which are usually the same as the application packages) to permit the test engine to perform deep reflection on the test classes.

Is that what you meant ?

[Tibor17]

@NilsRenaud
It means that you should use module-info.java in src/main/java if you want to use JPMS with this plugin, and in that case test dependencies, JUniy5 engines and the surefire libraries would appear on the classpath. The modulepath would contain project dependencies.

But you can additionally use the module descriptor in src/test/java and the classpath would change in the CLI java @args, so it means that the classpath would not use JUnit5 engines, and the only surefire libraries would be in classpath, and so all the dependencies would appear on the modulepath.

[Tibor17]

@NilsRenaud
From your comment it reminds me to say that we should maybe think of adaptive control of --add-opens in regards of package name, existence of module descriptor in src/main and src/test. Do you think, it would have a positive effect? Do you think it would have impact to the open in the The featured example of JPMS with JUnit 5 as you have showed me?

[NilsRenaud]

Do you think it would have impact to the open in the The featured example of JPMS with JUnit 5 as you have showed me?

In this example, my feature has no impact as you can see in the arg file used to run the tests :

--module-path
"/me/mod-test/target/test-classes:/me/mod-test/target/classes:/me/.m2/..."
--class-path
"/me/.m2/..."
--add-modules
com.foo.test   <------ The module is added through add module, not with patch-module + add-exports/opens
--add-opens
org.junit.platform.commons/org.junit.platform.commons.util=ALL-UNNAMED
--add-opens
org.junit.platform.commons/org.junit.platform.commons.logging=ALL-UNNAMED
org.apache.maven.surefire.booter.ForkedBooter

And playing with the example a bit further : if I remove the "open" in the test's module-info.java : my JUnit tests methods HAVE TO be public.

As reminder, here is the output when tests are not modularised :

--module-path
"/me/mod-test/target/classes"
--class-path
"/me/.m2/..."
--patch-module
com.foo.impl="/me/mod-test/target/test-classes"
--add-opens
com.foo.impl/com.foo.implt=ALL-UNNAMED
--add-modules
com.foo.impl
--add-reads
com.foo.impl=ALL-UNNAMED
org.apache.maven.surefire.booter.ForkedBooter

From your comment it reminds me to say that we should maybe think of adaptive control of --add-opens in regards of package name, existence of module descriptor in src/main and src/test ?

I don't think so, as stated above when the tests are modularised themselves we are using their module description and we're not interfering. In case of non-modularised tests the compiled code will be patched into the "main" module, then exported/opened. I don't think there is more control to offer to the user : they can reuse the package names or not, it's fine, everything will be packed together anyway.

[Tibor17]

@NilsRenaud
I am trying to understand new config parameter.
Would it make sense against hard coded --add-opens?
Would the default value make sense?

[NilsRenaud]

I don't see any use case where we would prefer --add-exports instead of --add-opens BUT if you specifically want to test that some of your classes are not accessible through deep reflection. But again, I'm not expert and I've made my tests only with JUnit so far.
So I would rather go with --add-opens as default value instead of --add-exports.

I still think having a dedicated parameter instead of an hard coded value makes sense.
This way, if we happen to break a user's test suite (for unknown reasons) he/she will be able to rollback to the --add-exports option easily.

[Tibor17]

@NilsRenaud
I would prefer not adding a new parameter unless really necessary. We can do that in milestone versions and then listen to the feedback from users. Can you please extract it to another PR? We will keep it open long time.

[NilsRenaud]

PR created : #471

[Tibor17]

Let's keep this survivor open during next few versions.