Surefire 1593 #199
Surefire 1593 #199
Conversation
| String uri = toClasspathElementUri( parent, classPathElement, dumpLogDirectory ); | ||
| cp.append( uri ); | ||
| if ( classPathElement.isDirectory() && !uri.endsWith( "/" ) ) | ||
| if ( new File( classPathElement ).isDirectory() && !uri.endsWith( "/" ) ) |
There was a problem hiding this comment.
Files.isDirectory will be more consistent with the new use of Paths.
What do you think?
There was a problem hiding this comment.
@eolivelli
yes, and moreover we should change the method signature from this:
toClasspathElementUri( String parent, String classPathElement, File dumpLogDirectory )
to this:
toClasspathElementUri( Path parent, Path classPathElement, Path dumpLogDirectory )
Which requires calling dumpLogDir.toPath() in ForkStarter (Line 580) but pls do not go deeper changing File to Path in all project since it is not a matter of this PR.
There was a problem hiding this comment.
It would be nice to have Path parent created once before the loop.
There was a problem hiding this comment.
In the scope of this change which is about using Paths it seemed to me strange to add a line which is using the legacy API.
Btw the result will be correct
There was a problem hiding this comment.
I am totally with you. Let's wait for @jglick until he makes all changes.
off-topic: We have one JUnit5 issue and some easy issues to fix. I guess we can release a new release candidate next week,
There was a problem hiding this comment.
we should change the method signature
No, I deliberately used String parameters so as to make it possible to write unit tests. Paths can (as far as I know) only be constructed using platform-specific methods, making the tests behave differently according to the machine running them.
There was a problem hiding this comment.
Files.isDirectorywill be more consistent with the new use ofPaths.
I chose to make a more minimal change to solve the immediate regression, rather than rewriting the whole class to use NIO methods.
There was a problem hiding this comment.
Thst's correct. The Strings are already platform specific.
| .isEqualTo( "file" ); | ||
|
|
||
| assertThat( testDirUri.getRawPath() ) | ||
| .isEqualTo( testDir.toURI().getRawPath() ); |
There was a problem hiding this comment.
This will likely fail when run on Windows using a project directory that is a UNC path rather than a drive letter: just as File.toURL had numerous problems corrected only by File.toUri, so File.toUri has some problems corrected by Path.toUri.
I would suggest dropping shouldRelativizeOnRealPlatform and shouldMakeAbsoluteUriOnRealPlatform. If you want unit tests, that is what I originally wrote. If you want something more realistic, demonstrating that the result is what was intended, shouldMakeRelativeUriOnRealPlatform is better.
There was a problem hiding this comment.
ok, I will use Path.toUri then, but I am wondering who wants to run Jenkins build on Surefire project that is located on shared network folder.
There was a problem hiding this comment.
I am wondering who wants to run Jenkins build on Surefire project that is located on shared network folder
\\vboxsvr\linux perhaps?
| @PrepareForTest( { JarManifestForkConfiguration.class, InPluginProcessDumpSingleton.class } ) | ||
| public class JarManifestForkConfigurationTest | ||
| { | ||
| private static final String BASE_DIR = System.getProperty( "user.dir" ); |
There was a problem hiding this comment.
This will pollute the CWD when mvn is run using -f rather than defaulting the basedir.
If you want a temporary directory, I would suggest using the TemporaryFolder rule.
There was a problem hiding this comment.
These are files for testing purposes in target folder. Here we run the build mvn install in the root and not in a child. Even if you run this particular test in IntelliJIDEA, the user.dir is maven-surefire-common/pom.xml, so I think it is fine for this test but we can create the temporal folders in TEMP if you like, this does not harm the goal of the test.
|
Since I cannot edit the description or title, a note to viewers: this is an amendment of #198. |
|
@jglick |
...est/java/org/apache/maven/plugin/surefire/booterclient/JarManifestForkConfigurationTest.java
Outdated
Show resolved
Hide resolved
…fest.util.Files).
|
@jglick |
|
@jglick |
No description provided.