[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
