bug fix for Volume binding bug in windows #1338 #1347
Conversation
| */ | ||
| @Test | ||
| public void testResolveAbsoluteWindowsVolumePath() { | ||
| String volumeString = format(BIND_STRING_FMT, "C:\\dir/subdir/../", CONTAINER_PATH); |
There was a problem hiding this comment.
your test is passing on windows but not on Linux. I think we should refactor this to pick different volume strings depending upon underlying OS
There was a problem hiding this comment.
I wonder whether there is annotation for JUnit to run tests on specific platforms only. If this is possible we can just create two tests, one for windows and one for unix like operating systems.
There was a problem hiding this comment.
As far as I know the annotation for running Test on a specific platform (@EnabledOnOs) is Junit 5 specific. I added
Assume.assumeTrue(System.getProperty("os.name").toLowerCase().startsWith("win"));
in the test code
There was a problem hiding this comment.
I think for now this is okay too. Let's try to get this merged. We can refactor this later in case we migrate to Junit5 in future
| /** | ||
| * A dot representing the parent directory | ||
| */ | ||
| private static final String DOUBLE_DOT = "."; |
There was a problem hiding this comment.
Maybe name this a bit more specific that this is a Windows special thing ? I mean its only a single dot, why should it be called double dot ?
There was a problem hiding this comment.
Or is this just a typo ? If so, I wonder why your tests passed ?
There was a problem hiding this comment.
It was a typo the real value shoud be "..".
Fixed.
the test passes because I'm using String.contains that is also true for "."
| @@ -181,6 +195,13 @@ public static String resolveRelativeVolumeBinding(File baseDir, String bindingSt | |||
| } catch (IOException e) { | |||
| throw new RuntimeException("Unable to canonicalize '" + resolvedFile + "'"); | |||
| } | |||
| } else if (SystemUtils.IS_OS_WINDOWS && (localPath.contains(DOUBLE_DOT))) { | |||
There was a problem hiding this comment.
I think you can always resolve the absolut path, regardless wether its on Window/Linux or whether it contains .. or not. In the worst case its a no-operation, but it would be easier to read.
There was a problem hiding this comment.
My first try was exactly this, but doing so make cause some test failures and I don't know this project enough to understand how to fix the additional failures
Codecov Report
@@ Coverage Diff @@
## master #1347 +/- ##
============================================
- Coverage 62.02% 59.00% -3.03%
+ Complexity 2150 1989 -161
============================================
Files 166 162 -4
Lines 9472 9044 -428
Branches 1438 1368 -70
============================================
- Hits 5875 5336 -539
- Misses 3083 3215 +132
+ Partials 514 493 -21 |
|
I pushed a new commit con my branch, let me know if you see the new version |
rohanKanojia
force-pushed
the
fix_1338_volume_binding_windows
branch
from
c46148f to
7d5e94f
Compare
+ Fix VolumeBindingUtil to handle absolute windows paths as well Signed-off-by: Giovanni Cuccu <gcuccu@imolainformatica.it>
rohanKanojia
force-pushed
the
fix_1338_volume_binding_windows
branch
from
7d5e94f to
74abe8d
Compare
Hi this is a pull request for issue #1338
I fixed only the specific issue I was having because when I tried to make the code more generic some tests were failing. The current code considers the initial rel of 'rel:/path/to/container/dir' as an absolute path which is not, when I tried to align the code considering rel as a relative some tests were failing and I do not have enough knowledge about this project in order to make a wider fix.
Fix #1338