Skip to content

Fix relative path test on Windows #618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kytrinyx merged 1 commit into from
Jul 14, 2018
Merged

Fix relative path test on Windows #618

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cmd/submit_relative_path_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
)

func TestSubmitRelativePath(t *testing.T) {
t.Skip("The Windows build is failing and needs to be debugged.\nSee https://ci.appveyor.com/project/kytrinyx/cli/build/110")
//t.Skip("The Windows build is failing and needs to be debugged.\nSee https://ci.appveyor.com/project/kytrinyx/cli/build/110")

oldOut := Out
oldErr := Err
Expand All @@ -30,6 +30,7 @@ func TestSubmitRelativePath(t *testing.T) {

tmpDir, err := ioutil.TempDir("", "relative-path")
assert.NoError(t, err)
defer os.RemoveAll(tmpDir)
Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I need to add this logic everywhere.


dir := filepath.Join(tmpDir, "bogus-track", "bogus-exercise")
os.MkdirAll(dir, os.FileMode(0755))
Expand All @@ -55,5 +56,5 @@ func TestSubmitRelativePath(t *testing.T) {
assert.NoError(t, err)

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"])
Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG. Of course.

This should be string(os.PathSeparator) + "file.txt" or something like that. I wonder if that will automatically escape the string.

Copy link
Copy Markdown
Contributor Author

@nywilken nywilken Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/sends back/expects/ (sends back on the download)

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Jul 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@nywilken nywilken Jul 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Jul 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yepp, I think that's fine. I'll go ahead and merge.

}