You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/02/08 18:01:15 UTC
[GitHub] [maven-surefire] NilsRenaud opened a new pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
NilsRenaud opened a new pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461
This PR follows the discussion on the associated Jira issue : https://issues.apache.org/jira/browse/SUREFIRE-1909
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1032991215
Pls name the title of the PR, Jira and commit same.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1033654325
@NilsRenaud
The `open` appears in the [documentation](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/site/apt/examples/jpms.apt.vm#L196) and the [integration test](https://github.com/apache/maven-surefire/blob/master/surefire-its/src/test/resources/maven-multimodule-project-with-jpms/com.foo.impl/src/test/java/module-info.java#L20). If you remove the keyword `open`, would the test work? Pls try.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] NilsRenaud commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
NilsRenaud commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1036018758
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](https://maven.apache.org/surefire/maven-surefire-plugin/examples/jpms.html), 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 ?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] NilsRenaud commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
NilsRenaud commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1034131508
@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](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/site/apt/examples/jpms.apt.vm#L196) and the [integration test](https://github.com/apache/maven-surefire/blob/master/surefire-its/src/test/resources/maven-multimodule-project-with-jpms/com.foo.impl/src/test/java/module-info.java#L20). 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 a 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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1039084295
@NilsRenaud
I am trying to understand new config parameter.
Would it make sense against hard coded `--add-opens`?
Would the default value make sense?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] NilsRenaud commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
NilsRenaud commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1041385264
PR created : https://github.com/apache/maven-surefire/pull/471
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1039084295
@NilsRenaud
I am trying to understand tnew config parameter.
Would the default value make sense? Would it make sense against hard coded `--add-opens`?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] NilsRenaud edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
NilsRenaud edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1034131508
@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](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/site/apt/examples/jpms.apt.vm#L196) and the [integration test](https://github.com/apache/maven-surefire/blob/master/surefire-its/src/test/resources/maven-multimodule-project-with-jpms/com.foo.impl/src/test/java/module-info.java#L20). 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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1036052786
@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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1036073488
@NilsRenaud
From your [comment](https://github.com/apache/maven-surefire/pull/461#issuecomment-1036018758) 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](https://maven.apache.org/surefire/maven-surefire-plugin/examples/jpms.html) as you have showed me?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1033654325
@NilsRenaud
The `open` appears in the [documentation](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/site/apt/examples/jpms.apt.vm#L196) and [integration test](https://github.com/apache/maven-surefire/blob/master/surefire-its/src/test/resources/maven-multimodule-project-with-jpms/com.foo.impl/src/test/java/module-info.java#L20). If you remove the keyword `open`, woult the test work? Pls try.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1041506070
Let's keep this survivor open during next few versions.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1039167240
@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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1032980363
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
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1035668621
@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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1035668621
@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 that the `surefire-booter` and `surefire-platform-provider` 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 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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1032980363
Hi @NilsRenaud ,
I have approved the CI. Now it is running. If it would pass successfully, we should obtain from you a new distinct integration test related to your use case. Would you have some spare time to write one? Thx
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1032980363
Hi @NilsRenaud ,
I have approved the CI. Now it is running. If it would pass successfully, we should obtain from you a new distinct integration test related to your use case. Would you have spare time to write one? Thx
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] NilsRenaud commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
NilsRenaud commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1039096076
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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sormuras commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
sormuras commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1034645665
"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)?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1035668621
@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 that the `surefire-booter` and `surefire-platform-provider` 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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1033077736
@NilsRenaud
Is it this IT `maven-multimodule-project-with-jpms` you are looking for?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] NilsRenaud commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
NilsRenaud commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1034634968
I tend to agree with you @Tibor17, but @sormuras said that this drop in replacement could ["break other use cases"](https://issues.apache.org/jira/browse/SUREFIRE-1909?focusedCommentId=17488968&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17488968).
@sormuras, could you give us an example ?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1034232508
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
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] NilsRenaud commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
NilsRenaud commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1036345509
> Do you think it would have impact to the open in the [The featured example of JPMS with JUnit 5](https://maven.apache.org/surefire/maven-surefire-plugin/examples/jpms.html) 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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1032987108
@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
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #461: [SUREFIRE-1909] Replace --add-exports with --add-opens
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #461:
URL: https://github.com/apache/maven-surefire/pull/461#issuecomment-1039084295
@NilsRenaud
I am trying to understand new config parameter.
Would the default value make sense? Would it make sense against hard coded `--add-opens`?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org