Fix test failures when build under Windows #834
Conversation
Codecov Report
@@ Coverage Diff @@
## master #834 +/- ##
============================================
+ Coverage 47.6% 47.63% +0.03%
- Complexity 1072 1074 +2
============================================
Files 134 134
Lines 6936 6942 +6
Branches 913 914 +1
============================================
+ Hits 3302 3307 +5
Misses 3350 3350
- Partials 284 285 +1
|
| @@ -501,7 +501,7 @@ private File findDockerFileFile(Logger log) { | |||
| if (dockerFileDir == null) { | |||
| return dFile; | |||
| } else { | |||
| if (dFile.isAbsolute()) { | |||
| if (dFile.isAbsolute() || !EnvUtil.isValidWindowsFileName(dockerFile)) { | |||
There was a problem hiding this comment.
Could you please add this as an extra check with an extra error message for the invalide windows file name case ? Otherwise its confusing when the exception message does not fit to the error.
|
|
||
| public static boolean isValidWindowsFileName(String text) { | ||
|
|
||
| if (!isWindows()) { |
There was a problem hiding this comment.
Please put the windows checks outside this method (e.g. where it is called) as otherwise the semantics of this check is confusing (e.g. returning different return values depending on the same argument, depending on on which OS the method is called)
| return true; | ||
| } | ||
|
|
||
| Pattern pattern = Pattern.compile( |
There was a problem hiding this comment.
Could you please add a reference to the SO answer as comment above ?
| @@ -392,4 +392,32 @@ public static Date loadTimestamp(File tsFile) throws MojoExecutionException { | |||
| public static boolean isWindows() { | |||
| return System.getProperty("os.name").toLowerCase().contains("windows"); | |||
| } | |||
|
|
|||
| public static boolean isValidWindowsFileName(String text) { | |||
There was a problem hiding this comment.
Some unit tests for this methods would be awesome (shouldn't be hard to build)
| @@ -504,6 +504,9 @@ private File findDockerFileFile(Logger log) { | |||
| if (dFile.isAbsolute()) { | |||
| throw new IllegalArgumentException("<dockerFile> can not be absolute path if <dockerFileDir> also set."); | |||
| } | |||
| if (EnvUtil.isWindows() && !EnvUtil.isValidWindowsFileName(dockerFile)) { | |||
| throw new IllegalArgumentException("<dockerFile> can not be absolute path if <dockerFileDir> also set."); | |||
There was a problem hiding this comment.
Shouldn't this be something like
throw new IllegalArgumentException(String.format("Invalid windows file name %s for dockerFile", dockerFile))
|
Thanks ! |
Fix test failures when build under Windows.
The validation of the Windows filename is copied from stackoverflow: https://stackoverflow.com/a/6804755