[GitHub] Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows
Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows URL: https://github.com/apache/maven-surefire/pull/198#discussion_r232479128 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/JarManifestForkConfiguration.java ## @@ -150,10 +150,9 @@ private static String toClasspathElementUri( @Nonnull Path parent, { try { -return new URI( null, -parent.relativize( classPathElement.toPath() ).toString(). -replace( File.separatorChar, '/' ), -null ).toASCIIString(); +Path relativePath = parent.relativize( classPathElement.toPath() ); Review comment: @jglick Not sure about Vagrant but I know that Docker works with Windows 10. our Jenkins in ASF has Windows server 2008. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows
Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows URL: https://github.com/apache/maven-surefire/pull/198#discussion_r231991410 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/JarManifestForkConfiguration.java ## @@ -150,10 +150,9 @@ private static String toClasspathElementUri( @Nonnull Path parent, { try { -return new URI( null, -parent.relativize( classPathElement.toPath() ).toString(). -replace( File.separatorChar, '/' ), -null ).toASCIIString(); +Path relativePath = parent.relativize( classPathElement.toPath() ); Review comment: @jglick There is one IT test `Surefire855AllowFailsafeUseArtifactFileIT` which asserts Class-Path in manifest and surefire.properties, but it's hard to develop such test running on any platform and with any number of disks. Even in our case, using ASF Jenkins CI, we have workspace on `F:\` but the TEMP is on `C:\`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows
Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows URL: https://github.com/apache/maven-surefire/pull/198#discussion_r231991410 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/JarManifestForkConfiguration.java ## @@ -150,10 +150,9 @@ private static String toClasspathElementUri( @Nonnull Path parent, { try { -return new URI( null, -parent.relativize( classPathElement.toPath() ).toString(). -replace( File.separatorChar, '/' ), -null ).toASCIIString(); +Path relativePath = parent.relativize( classPathElement.toPath() ); Review comment: @jglick There is one IT test `Surefire855AllowFailsafeUseArtifactFileIT` which asserts Class-Path in manifest and surefire.properties, but it's hard to develope such test runing on any platform and any number of dists. Even in our case, using ASF Jenkins CI, we have workspace on `F:\` but the TEMP is on `C:\`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows
Tibor17 commented on a change in pull request #198: [SUREFIRE-1593] Correcting relativization logic to produce valid URIs on Windows URL: https://github.com/apache/maven-surefire/pull/198#discussion_r231979549 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/JarManifestForkConfiguration.java ## @@ -150,10 +150,9 @@ private static String toClasspathElementUri( @Nonnull Path parent, { try { -return new URI( null, -parent.relativize( classPathElement.toPath() ).toString(). -replace( File.separatorChar, '/' ), -null ).toASCIIString(); +Path relativePath = parent.relativize( classPathElement.toPath() ); Review comment: This won't tested by ITs because we cannot occupy any disk on Jenkins, but this algorithm could be tested by unit test and Mockito. At least Line 153 in mock and next two lines are real and tested. If the Line went to private static method, we could mock it via PowerMock, and then the next two lines together in another method calling the first static method. We would assert the returned value has Unix slashes and white spaces are escaped, paths are relative, etc. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services