Fix relative path test on Windows#618
Conversation
You and me both! |
kytrinyx
left a comment
kytrinyx
left a comment
There was a problem hiding this comment.
If we can figure out the correct way to use os.PathSeparator then we can inline the test back into the main file.
|
|
||
| assert.Equal(t, 1, len(submittedFiles)) | ||
| assert.Equal(t, "This is a file.", submittedFiles["/file.txt"]) | ||
| assert.Equal(t, "This is a file.", submittedFiles["\\file.txt"]) |
There was a problem hiding this comment.
OMG. Of course.
This should be string(os.PathSeparator) + "file.txt" or something like that. I wonder if that will automatically escape the string.
There was a problem hiding this comment.
I don't think it will escape, but escaping is not needed as the key actually is \file.txt. Changing to string(os.PathSeparator) + "file.txt" works as expected. However, I have a question why does the filename need to have the path separator? Should it not just be the filename?
There was a problem hiding this comment.
It's what the exercism API sends back, relative to the workspace directory, including the path separator. We could have done it either way, I don't think we had any reason to do one over the other.
There was a problem hiding this comment.
s/sends back/expects/ (sends back on the download)
There was a problem hiding this comment.
Wait. uhm.... is it just setup in the test that does this? In that case I think you're right, we should just remove it.
There was a problem hiding this comment.
I think the most reasonable thing right now is to keep the leading slash, since there are so many moving parts. It could be worth reconsidering down the road, as whether we change this now or later, we still have to be able to take into account the exercises that do have the leading slash.
There was a problem hiding this comment.
I agree with you on this one; especially since I have not been able to give a lot of time. I would hate to start a larger thing right now and not be able to follow it all the way through.
If you are okay with it I will merge this in.
There was a problem hiding this comment.
Yepp, I think that's fine. I'll go ahead and merge.
|
|
||
| tmpDir, err := ioutil.TempDir("", "relative-path") | ||
| assert.NoError(t, err) | ||
| defer os.RemoveAll(tmpDir) |
There was a problem hiding this comment.
Nice. I need to add this logic everywhere.
|
I made a tweak to the branches on this repository. TL;DR: You need to edit this PR to be against
Once we've made sure everything looks right we can delete the nextercism branch. |
This changes fixes the one test for Windows, but I am not entirely confident that the test passing means that it will work 100% of the time on Windows. I am still wrapping my head around how the submission stuff should play out.
nywilken
force-pushed
the
windows-path-testing
branch
from
4ced209 to
4672b49
Compare
2 participants
This changes fixes the one test for Windows, but I am not entirely
confident that the test passing means that it will work 100% of the time
on Windows. I am still wrapping my head around how the submission stuff
should play out.