Hugolib/helpers - first stab at extra tests

All,

First the good news. After a bit of hacking around in the
hugolib/helpers module I managed to up the test coverage from 25.5% to
74.8%. That is without touching the pygments module(*) which looks to be
difficult to test in it’s current form. And I don’t yet have pygments
installed to start testing it.

The result is available on github in the ‘extend-hugolib-tests’ branch at:

Note: this branch is not in the final PR form, so hold off merging it.

The bad news is we have a fairly large amount of test failures,
especially in the path.go module. See the output of the ‘go test’
command below.

So before I create panic :), at least of some of these are going to
be my fault because I don’t yet understand Steve’s intention with some
of these, the

TestUrlPrep
test being a case in point.

The remaining tests:

TestReplaceExtension
TestFilename
TestFileAndExt
TestGuessSection

Look reasonable, but fail. At the moment I’m not sure if I’ve
misinterpreted what the function is supposed to do, or not?

TestFindCWD
I believe my understanding and therefore the test

implementation is wrong. It looks like the code is trying to find out
where the server was started from based on the location of the exe on
Windows. Is this correct? But can’t the whole thing be replaced with a
simple call to os.Getwd() - which will work on Windows and Linux.

Anyhow can you take a quick look and point me in the right direction so
I can close off these test failures (or misunderstands). Once everything
passes I’ll wrap everything up in a neat little PR.

Once the PR is out of the way I propose to just take the module with the
next lowest test coverage and start working to improve the test coverage
there.

I can see where TLC can be applied in the code I’ve looked at, but I’m
not keen on refactoring stuff on a large scale until the project as a
whole has a lot more test coverage. Refactoring without tests gives me
the hebejebes :wink:

Owen

go test output:


$ go test -cover ./...
    
--- FAIL: TestReplaceExtension (0.00 seconds)
        path_test.go:95: Test 0 failed. Expected "/some/randompath/file.html"
got "file.html"
        path_test.go:95: Test 1 failed. Expected "/banana.HTML" got
"banana.HTML"
        path_test.go:95: Test 2 failed. Expected "./banana.xml" got
"banana.xml"
        path_test.go:95: Test 3 failed. Expected "banana/pie/index.xml" got
"index.xml"
        path_test.go:95: Test 4 failed. Expected "../pies/fish/index.xml" got
"index.xml"
--- FAIL: TestFilename (0.00 seconds)
        path_test.go:371: Test 5 failed. Expected "filename-no-ext" got
"./filename-no-ext"
        path_test.go:371: Test 6 failed. Expected "filename-no-ext" got
"/filename-no-ext"
        path_test.go:371: Test 8 failed. Expected "" got "directoy/"
--- FAIL: TestFileAndExt (0.00 seconds)
        path_test.go:396: Test 5 failed. Expected filename "filename-no-ext"
got "./filename-no-ext".
        path_test.go:396: Test 6 failed. Expected filename "filename-no-ext"
got "/filename-no-ext".
        path_test.go:396: Test 8 failed. Expected filename "" got "directoy/".
--- FAIL: TestGuessSection (0.00 seconds)
        path_test.go:427: Test 2 failed. Expected "/" got ""
        path_test.go:427: Test 3 failed. Expected "/" got ""
        path_test.go:427: Test 4 failed. Expected "/" got "content"
        path_test.go:427: Test 5 failed. Expected "/blog" got ""
        path_test.go:427: Test 6 failed. Expected "/blog/" got "blog"
        path_test.go:427: Test 7 failed. Expected "blog" got ""
        path_test.go:427: Test 8 failed. Expected "/blog" got ""
        path_test.go:427: Test 9 failed. Expected "/blog/" got "blog"
        path_test.go:427: Test 10 failed. Expected "/blog/" got "content"
--- FAIL: TestFindCWD (0.00 seconds)
        path_test.go:453: Test 0 failed. Expected
"/home/owen/go/src/github.com/owenwaller/hugo/helpers" but got
"/tmp/go-build092258256/github.com/owenwaller/hugo/helpers/_test"
NO error returning url = "/section/name/"
Ugly case. Returning x = "/section/name.html"
--- FAIL: TestUrlPrep (0.00 seconds)
        url_test.go:65: Test #0 failed. Expected "/section/name/index.html" got
"/section/name/"
FAIL
coverage: 74.8% of statements
FAIL    github.com/owenwaller/hugo/helpers 0.013s
2 Likes

First of all. @OwenWaller Great work! This will really facilitate our ability to refactor and improve Hugo more nimbly and accept contributions with greater confidence.

ReplaceExtension

Replace Extension is probably poorly named, but the intent of the function is to accept a path and return only the file name with a new extension. It’s intentionally designed to strip out the path and only provide the name. We should probably rename the function to be more explicit at some point.

AbsPathify

The behavior you are seeing is the intended behavior. It is the result of calling filepath.clean.
What AbsPathify does is make a path absolute (if it’s absolute already fine, otherwise make it absolute by prepending the working directory). Then it runs filepath.clean on it which makes the path as simple as possible.

So /home/steve/docs/…/documentation -> /home/steve/documentation
and docs/static/js/…/…/content -> $WD/docs/content

Filename & FileAndExt

Filename wraps FileAndExt so fixing the latter will fix the former. We are simply using filepath.Base here, but I agree that the output isn’t quite what is desired. I think these are legitimate failures and we should write code to handle these cases.

GuessSection

This function is designed to take the input of a full path (relative to the BASE URL) and provide the section it contains. By definition a section is the first directory from the root. A section never has a slash in it. I believe these failing tests all have the wrong desired expected result… I’ll illustrate with a couple for example.

content/blog should return “” because there is no directory from root
/content/blog should return “content” because it is a full directory from root.
/blog/ should return “blog”
"" is the result of any input that doesn’t have a full directory ("/", “”, “fooo”, “bar/”, etc)

FindCWD

To be completely honest. I wrote this a long time ago and I can’t remember the reason why I took this route instead of os.Getwd(). I’ve been looking through the code and it doesn’t help me remember. One possible reason is that FindCWD can be much quicker, but I don’t think that’s the reason I wrote it in the first place. One feature of FindCWD that isn’t ideal is that when you do go run main.go or go test ./… it actually tells you the location of the temporary compiled executable, not the place you called go run or go test from. We should explore switching to os.Getwd() and see if that works better.

UrlPrep

Pretty Urls never include the “index.html” portion. Instead they end with the directory/ . The file corresponding to it would end in index.html, but the whole point of pretty urls is to avoid the “index.html” part.

Hi Steve,

OK if you take a look now at

You’ll see I’ve updated the test cases for:
TestReplaceExtension
TestFilename
TestFileAndExt

And that all of the test cases now pass.

I have also completely rewritten FileAndExt so that it does what we
might reasonably expect. Basically, path.Base doesn’t quiet work as you
might expect and it also strips any trailing slashes which makes it hard
to find out if FileAndExt had been passed a path that is just a
directory path.

For path.Base try these cases:
path.Base(".") -> “.”
path.Base("./") -> “.”
path.Base("") -> “.”
path.Base("/…") -> “…”
path.Base("/.") -> “.”
path.base("…/") -> “…”
path.base("/mydir/") -> “mydir”

Which helped me make its behaviour clear.

The new version of FileAndExt now picks up on all of the cases where the
path supplied does not end in a filename (plus the optional extension).
In these cases both the filename and the extension will be returned as
empty strings. If the path does end in a filename then the non-empty
filename will be returned and plus the possibly empty extension.

See the commit comment and the code diff for exact details of the
change.

For the other functions:

AbsPathify, my test cases pass, but what AbsPathify internally relies on
viper.GetString(“WorkingDir”). Which means to test this the test case
needs to know about viper and the hugo config file. Neither of which are
practical for the test. What I propose is that AbsPathify takes a second
string parameter which is the value of "viper.GetString(“WorkingDir”).
What are your thoughts?

GuessSection, I can update my test cases to take account of your
comments. I’ve just not done it yet…

FindCWD, I can have a look at switching to os.Getwd and see what
happens. One thing I would say is that (from memory) FindCWD is only
called from one place, so if os.Getwd works then it may be simpler to
remove the function completely.

UrlPrep I get. But on then I don’t understand is, do you intended Ugly
and Pretty to be inverses of each other? So in other words should:

Ugly(Pretty(myUrl) == myUrl

where myUrl is something like http://myhost.com/section/ramdom.html

At the minute I don’t think it is - this is what the failing test is
showing. The question is should they be inverses?

Owen

Hi Steve,

OK after a mad few weeks professionally, I’ve finally got back to
looking the the tests in hugo.

I think there is still a bug in GuessSection, but I want to clarify the
behaviour first.

I’ve pushed the code to my extend-hugolib-tests branch as normal and
this is what I am seeing:

The tests that fail are:

Test number value, expected result
9 {"/content/blog/", “blog”},
10 {"/content/blog", “blog”},
11 {“content/blog/”, “”},
12 {"/contents/myblog/", “contents”},
14 {"/contents/ourblog/", “contents”},

With:

— FAIL: TestGuessSection (0.00 seconds)
path_test.go:442: Test 9 failed. Expected “blog” got "content/blog"
path_test.go:442: Test 10 failed. Expected “blog” got "content"
path_test.go:442: Test 11 failed. Expected “” got “blog"
path_test.go:442: Test 12 failed. Expected “contents” got
"contents/myblog"
path_test.go:442: Test 14 failed. Expected “contents” got
"contents/ourblog”

Now, I know GuessSection correctly filters out “content” as the first
part of the path, so that the standard hugo content directory is
removed. But what is the intended behaviour when more then one
path/directory element is passed?

As I understood things, see your explanation below, the path has to
start with a “/” and a section is the first part of the path. With any
"content" chopped. And a section can’t have a “/” anywhere within it.
Which implies to me that the section is always the bit between the first
two “/”'s, after “content” has been removed.

Can you confirm if this is the correct understanding and I’ll update the
code and the tests to reflect this.

Thanks

Owen

This is correct.

Hi Steve,

OK thanks. In which case there is a bug in GuessSection :slight_smile:
I’ll see if I can patch it early this week if I can grab an hour!

Ta.

Owen

@spf13 - Steve I need you to run your eyes over a bug fix.

I’ve (finally) got around to squashing the GuessSection bug we’ve been discussing, My updated code is here:


Note: that’s in a new FixGuessSection branch

I’ve also updated the test cases in the path_test.go. See:

Can you please sanity check that this is the behaviour you expect and that the “content” directory is correctly handled.

Thanks

Owen