search

gohugoio / hugo

The world’s fastest framework for building websites.
https://gohugo.io
Apache License 2.0
75.85k stars 7.54k forks source link

Failing unit tests on windows due forward slash vs back slash #660

Closed apsanz closed 9 years ago

apsanz commented 9 years ago

There a number of failing unit tests where it is expecting / but getting \ when I run it on windows:

=== RUN TestHTMLRedirectAlias
--- FAIL: TestHTMLRedirectAlias (0.00 seconds)
        alias_test.go:33: Expected: s/index.html, got: s\index.html
        alias_test.go:33: Expected: /index.html, got: \index.html
        alias_test.go:33: Expected: alias-1/index.html, got: alias-1\index.html
        alias_test.go:33: Expected: alias-2/index.html, got: alias-2\index.html
        alias_test.go:33: Expected: /alias-5.html, got: \alias-5.html
        alias_test.go:33: Expected: /трям.html, got: \трям.html
=== RUN TestPageTranslator
--- FAIL: TestPageTranslator (0.00 seconds)
        page_test.go:33: Tranlate expected return: bar/index.html, got: bar\index.html
        page_test.go:33: Tranlate expected return: foo/index.html, got: foo\index.html
        page_test.go:33: Tranlate expected return: foo/index.html, got: foo\index.html
        page_test.go:33: Tranlate expected return: foo/index.xhtml, got: foo\index.xhtml
        page_test.go:33: Tranlate expected return: section/index.html, got: section\index.html
        page_test.go:33: Tranlate expected return: section/index.html, got: section\index.html
        page_test.go:33: Tranlate expected return: section/foo/index.html, got: section\foo\index.html
        page_test.go:33: Tranlate expected return: section/foo/index.html, got: section\foo\index.html
        page_test.go:33: Tranlate expected return: section/foo/index.rss, got: section\foo\index.rss
=== RUN TestPageTranslatorBase
--- FAIL: TestPageTranslatorBase (0.00 seconds)
        page_test.go:57: Translate expected: a/base/index.html, got: a\base\index.html
        page_test.go:57: Translate expected: a/base/index.html, got: a\base\index.html
=== RUN TestTranslateUglyUrls
--- PASS: TestTranslateUglyUrls (0.00 seconds)
=== RUN TestTranslateDefaultExtension
--- FAIL: TestTranslateDefaultExtension (0.00 seconds)
        page_test.go:91: Translate expected return: baz/index.foobar, got baz\index.foobar
FAIL
FAIL    github.com/spf13/hugo/target    2.361s

I believe the root cause is using path/filesystem to handle what are pretty much URLs. More information I can change the classes to use path which resolves the failing unit tests. However, since I am still trying to understand this code base and a static blog might assume relative URL might equal file system location, I am not sure if this will work for hugo.

Seems OK for the failing tests for page.go and htmlredirect.go but I am not sure if some of the below files also need to be changed too or result in conflict:

$ grep -r 'path/filepath' .
./commands/convert.go:  "path/filepath"
./commands/hugo.go:     "path/filepath"
./commands/list.go:     "path/filepath"
./commands/new.go:      "path/filepath"
./commands/version.go:  "path/filepath"
./create/content.go:    "path/filepath"
./helpers/path.go:      "path/filepath"
./helpers/url.go:       "path/filepath"
./hugolib/page.go:      "path/filepath"
./hugolib/page_test.go: "path/filepath"
./hugolib/path_seperators_test.go:      "path/filepath"
./parser/parse_frontmatter_test.go:     "path/filepath"
./source/file.go:       "path/filepath"
./source/filesystem.go: "path/filepath"
./source/filesystem_test.go:    "path/filepath"
./target/file.go:       "path/filepath"
./tpl/template.go:      "path/filepath"
bep commented 9 years ago

@mohae did an all in replacement of path with filepath recently, no idea why.

@spf13 might also have some insight.

mohae commented 9 years ago

@bjornerik the path to path/filepath replacement was to make path operations target OS compatible. This was done to resolve Windows pathing issues people were reporting with new content generation and ensure path operations, in general, were compatible with the target OS.

All tests completed properly on Linux environments but I did not rerun the tests on Windows. Windows, for the most part, seems to have been not tested since prior to those changes; Hugo wouldn't properly generate anything.

It appears that I changed some that should have remained path, as pointed out by @apsanz

I'll look into this issue.

mohae commented 9 years ago

There are probably numerous testing issues with Windows as most of the tests probably haven't been run on Windows. Testing for Windows is something that should be its own topic for the discussion board and any work on the tests should be coordinated with @owenwaller

mohae commented 9 years ago

Looking at this a bit further, the issue is because URL paths are built using path.go functions, which operate on both file paths and URL paths.

While this works for non-Windows systems, or systems that use / as their separator, this does not work for Windows as its path separator if \.

Either separate functions should be used for filepaths and url paths or, for URLs, the result should be fed through a function to normalize path separators to /.

I'm also not sure if the way expectations are built for filepath based tests is the appropriate way, but I haven't gotten far enough into the testing evaluation to see.

Any suggestions/input on the best way to address this in Hugo?

apsanz commented 9 years ago

If all that matters is the static files that are generated, I think we are OK just using file path on windows and trusting that the server translates it to correct urls. However, I think there might be issues with things like translating relative links to static content in templates and redirects to pretty URLs. I am still trying to figure out how hugo does both of these things.

mohae commented 9 years ago

@tatsushid added a test appveyor integration for Hugo, https://github.com/spf13/hugo/issues/380, with the results of the tests here: https://ci.appveyor.com/project/tatsushid/hugo/build/1.0.2

It appears that there are two main classes of Windows testing errors: URL related and filename related (path.FileAndExt()). I added a discussion topic to cover this at http://discuss.gohugo.io/t/fix-tests-for-windows-path-path-test-go/363. The topic and title were created before I realized that all the Windows test errors can be but in two categories, with the exception of a few tests (which probably aren't valid in Windows).

I think you are spot on about the issues with links and redirect concerns.

Any help you can provide would be greatly appreciated!

apsanz commented 9 years ago

Previously, I didn't have any content to actually use with hugo. I tested with @spf13's site and all the links are using backslashes instead of forward. So, it is not just a matter of fixing the unit tests.

I think I agree with the discussion topic, the concept of the URL needs to be separated from the concept of filesystem path. Ideally, using URL as much as possible than than translating to a filesystem path only when reading and writing files.

The only problem with this is that that the code base uses them interchangeable. Do you agree this needs to be changed and would that have too big of an impact on the code base?

spf13 commented 9 years ago

We should definitely separate out URL support and path support. There are a few challenges to doing so, but ultimately it's the right thing to do.

mohae commented 9 years ago

@spf13 What are the challenges that you can think of?

spf13 commented 9 years ago

Currently the logic is all mixed up. There needs to be a clear separation of source, destination & url. 

mohae commented 9 years ago

Ok.

I'm glad to know that you weren't thinking if any specific or additional challenges besides the clear separation of the logic based on path vs url.

mohae commented 9 years ago

@spf13: would you prefer adding the URL specific path functions to helpers/url.go or add them as a new file; e.g. helpers/url_path.go?

spf13 commented 9 years ago

I need to think through this a bit more. I'm not sure what the right approach is yet.

apsanz commented 9 years ago

I looked into it a little and getting all unit tests to pass is relatively easy. However, it still results in a broken site, I guess we need more tests :). There are a lot of uses of helper/path that will need to be changed to helper/url or helper/url_path. I just wanted to add that this test failure seems to be cause by a possible go standard library bug I just reported:

 --- FAIL: TestPrettifyPath (0.00 seconds)
        Location:       path_test.go:502
        Error:          Not equal: "\\name\\index.xml" (expected)
                                != "\\\\name\\index.xml" (actual)
mohae commented 9 years ago

@apsanz that's why we need to separate out the url path handling from the file path handling. This is a situation where adjusting the test isn't sufficient.

It may be that the handling of file path's in Hugo will also need some additional modification for Windows: I think I came across a bug in creating content on Windows using latest. I need to do some more checking to make sure it isn't me causing it before I can be sure.

bep commented 9 years ago

What's the status on this ... It's kind of annoying with all the failing pull requests, and it creates this "the boy who cried wolf" situation.

I might get my hands on a Windows box (not with pleasure) if you need any help ...

spf13 commented 9 years ago

I would love help. I'm actually traveling this week out of the country.. .well out of my usual country, so I have very limited access to the Internet. 

mohae commented 9 years ago

@bjornerik I agree, which is why I started moving forward with making some initial changes to get this stuff sorted out, but @spf13 had indicated he wanted to think about it a bit more so it was paused until he provided additional feedback.

In response to my own question, I've been thinking that the url related path functions should be part of url.go. If something else gets decided later, then it can be fixed with additional pull requests.

I agree that the appveyor fails is annoying. Also, getting Hugo working consistently on Windows in tip would be a good thing for Hugo and for Windows users as the current state is confusing for me and I'm sure others.

bep commented 9 years ago

There is url.go and path.go so the "path" is given, so to speak.

@mohae I have a Windows box next to me right now (and a Linux box) and could make a stab on a fix for this, if you're not "in the middle of" fixing it.

mohae commented 9 years ago

@bjornerik well, I had just done a git pull on my Windows environment, but if you want to take the baton and run with it, feel free. I'll look at something else or wander back to my Linux VMs to work on something else.

bep commented 9 years ago

Just chime in to say I'm working on this, but the wind washed a little bit out of me after setting up Go and Hugo on a Windows PC.

Man, that path format difference have cost billions in developer cost. Maybe good for paid consultant jobs, of course. Wonder what person inside Windows decided "let's do it slightly diferent".

mohae commented 9 years ago

Glad to hear that. I was going to check to make sure that we didn't get our signals crossed.

I believe you can blame IBM for that as / was used by them as an option flag and MS-DOS was originally created for IBM, iirc. IBM tends to be it's own unique self in the tech world. EBCIDIC vs ASCII is another example. Its ironic that you point out paid consultant jobs since that's what IBM seems to excel in nowadays.

(IBM lovers please don't hate me, the IBM Power series has some amazing technology)

Nowadays, Window's doesn't care as much about / vs \ but it's not something that can be relied on. Powershell is an example of this; it will auto-convert the slashes. I'm not sure how Window's new command shell does it as I haven't tried 10 yet.

Another one is CRLF vs LF. This is especially fun with Windows Git users that haven't really used any other system than Windows and don't set up their autocrlf and other related preferences correctly.

I also understand about being wind washed; I've been doing my Windows/Hugo related stuff in spurts too.

tatsushid commented 9 years ago

@mohae, @bjornerik, I also set up Windows virtual test environment (2012 R2 with Go 1.3.3) on my machine. If there is something I can do for it, feel free to ask.

bep commented 9 years ago

I have worked on this today (what's Saturdays for :)) and I'm almost there.

tatsushid commented 9 years ago

@bjornerik I check the #688. It is caused by Go runtime bug as @mohae wrote. To resolve it, it is needed to rewrite os.Chtimes and syscall.UtimesNano by ourselves for spf13/fsync or just to avoid the error, adding NoTimes = true in config file or specifying noTimes option is enough for it. It changes Hugo's behavior not to sync public directory's mtime and atime. In my test, NoTimes worked good and seemed not to affect hugo server -w behavior.

At any rate, Go 1.4 will be released soon. I think it is the best to use it.

bep commented 9 years ago

@tatsushid understand, as long as we got the NoTimes option this is not a big issue (but we should document it somewhere).

I have added a PR with some fixes related to this issue. Annoyingly enough some tests fail for another reason .... I have done some manual testing, but no guarantees ...

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.