You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "dmlloyd (via GitHub)" <gi...@apache.org> on 2023/11/10 14:33:29 UTC
[PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
dmlloyd opened a new pull request, #686:
URL: https://github.com/apache/maven-surefire/pull/686
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SUREFIRE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Format the pull request title like `[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `SUREFIRE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean install` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [X] You have run the integration tests successfully (`mvn -Prun-its clean install`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [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)
--
The recent change (47eb1974ef2fb, since 3.2.0) to support more additional class path items during test has broken the use case of testing multi-release projects where ordering of class path elements is significant by changing the additional class path items collection from a `List` to a `HashSet`, which has unreliable ordering semantics. This simple fix preserves the original class path order specified in the POM by using a `LinkedHashSet`, which preserves insertion order, instead of a `HashSet`, which does not.
/cc @kwin
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "dmlloyd (via GitHub)" <gi...@apache.org>.
dmlloyd commented on code in PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#discussion_r1391327748
##########
surefire-its/src/test/resources/additional-classpath/src/test/java/additionalClasspath/BasicTest.java:
##########
@@ -12,4 +18,31 @@ public void testExtraResource()
assertNotNull( BasicTest.class.getResourceAsStream( "/test2.txt" ) );
}
+ public void testExtraResourceOrder()
+ {
+ Enumeration<URL> resources = BasicTest.class.getClassLoader().getResources("/order-test.txt");
+ assertTrue( resources.hasMoreElements() );
+ URL url = resources.nextElement();
+ InputStream is = url.openStream();
+ assertNotNull( is );
+ try ( InputStream i = is ) {
Review Comment:
I went ahead and changed 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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#issuecomment-1808458713
@dmlloyd Looks good to me. Thanks. Just one minor stylistic remark.
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#issuecomment-1830806541
The mailing list is the proper place to ask for a release.
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "dmlloyd (via GitHub)" <gi...@apache.org>.
dmlloyd commented on code in PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#discussion_r1389483563
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -2569,7 +2569,7 @@ private TestClassPath generateTestClasspath() throws MojoFailureException {
Map<String, Artifact> dependencyConflictIdsProjectArtifacts = classpathArtifacts.stream()
.collect(Collectors.toMap(Artifact::getDependencyConflictId, Function.identity()));
- Set<String> additionalClasspathElements = new HashSet<>();
+ Set<String> additionalClasspathElements = new LinkedHashSet<>();
Review Comment:
I can find some time to do this next week hopefully.
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin merged PR #686:
URL: https://github.com/apache/maven-surefire/pull/686
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#discussion_r1391321028
##########
surefire-its/src/test/resources/additional-classpath/src/test/java/additionalClasspath/BasicTest.java:
##########
@@ -12,4 +18,31 @@ public void testExtraResource()
assertNotNull( BasicTest.class.getResourceAsStream( "/test2.txt" ) );
}
+ public void testExtraResourceOrder()
+ {
+ Enumeration<URL> resources = BasicTest.class.getClassLoader().getResources("/order-test.txt");
+ assertTrue( resources.hasMoreElements() );
+ URL url = resources.nextElement();
+ InputStream is = url.openStream();
+ assertNotNull( is );
+ try ( InputStream i = is ) {
Review Comment:
There is no need for nested try with resources. One can open multiple objects in the same try: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "dmlloyd (via GitHub)" <gi...@apache.org>.
dmlloyd commented on code in PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#discussion_r1391325407
##########
surefire-its/src/test/resources/additional-classpath/src/test/java/additionalClasspath/BasicTest.java:
##########
@@ -12,4 +18,31 @@ public void testExtraResource()
assertNotNull( BasicTest.class.getResourceAsStream( "/test2.txt" ) );
}
+ public void testExtraResourceOrder()
+ {
+ Enumeration<URL> resources = BasicTest.class.getClassLoader().getResources("/order-test.txt");
+ assertTrue( resources.hasMoreElements() );
+ URL url = resources.nextElement();
+ InputStream is = url.openStream();
+ assertNotNull( is );
+ try ( InputStream i = is ) {
Review Comment:
Sure; I always just find this way to be more readable. Is it important to change 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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "dmlloyd (via GitHub)" <gi...@apache.org>.
dmlloyd commented on PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#issuecomment-1830269037
Is there some procedure to request a release of Surefire with this fix? @Tibor17 are you the person to ask?
Thanks...
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "dmlloyd (via GitHub)" <gi...@apache.org>.
dmlloyd commented on PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#issuecomment-1830304120
/cc @slawekjaranowski, could I please request a release including this fix? Thank you!
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "dmlloyd (via GitHub)" <gi...@apache.org>.
dmlloyd commented on PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#issuecomment-1808424078
I've added a test, however it's not clear that this will always detect a regression. The reason is that the problem was caused by the unpredictable iteration order of `HashSet`; it's possible that the iteration order of `HashSet` is *coincidentally* correct for a given set, thus it's difficult to detect the error with 100% certainty. Still, having the test may catch a regression someday.
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "dmlloyd (via GitHub)" <gi...@apache.org>.
dmlloyd commented on PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#issuecomment-1808555798
Pushed up a fix to a funny little class loader bug relating to canonicalization of paths when using `ClassLoader#getResources` with an absolute path name. I'm sure it's fine...
--
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
Re: [PR] [SUREFIRE-2210] - Restore ordering of additional class path elements [maven-surefire]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #686:
URL: https://github.com/apache/maven-surefire/pull/686#discussion_r1389480006
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -2569,7 +2569,7 @@ private TestClassPath generateTestClasspath() throws MojoFailureException {
Map<String, Artifact> dependencyConflictIdsProjectArtifacts = classpathArtifacts.stream()
.collect(Collectors.toMap(Artifact::getDependencyConflictId, Function.identity()));
- Set<String> additionalClasspathElements = new HashSet<>();
+ Set<String> additionalClasspathElements = new LinkedHashSet<>();
Review Comment:
LGTM, but a test case would be really appreciated so that we prevent regressions in the future.
--
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