Skip to content
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.

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

fileserver: First attempt to fix failing test on Linux #3625

Merged
merged 2 commits into from
Aug 1, 2020
Merged

Conversation

mholt
Copy link
Member

@mholt mholt commented Jul 31, 2020

I think I updated the wrong test case before. I don't have Linux handy so it's going to be trial-and-error until we can figure this out.

This is a new test case. If I recall correctly, we had problems with .PHP (non-lowercased path splits) that led to unfavorable handling of those requests in the past.

I think I updated the wrong test case before
@mholt mholt requested a review from francislavoie July 31, 2020 21:37
@mholt
Copy link
Member Author

mholt commented Jul 31, 2020

This test might only pass on case-insensitive file systems.

@Vigilans
Copy link
Contributor

Vigilans commented Aug 1, 2020

I debugged the matcher_test, the failure occured after the os.Stat call:

func strictFileExists(file string) bool {
stat, err := os.Stat(file)
if err != nil {
// in reality, this can be any error
// such as permission or even obscure
// ones like "is not a directory" (when
// trying to stat a file within a file);
// in those cases we can't be sure if
// the file exists, so we just treat any
// error as if it does not exist; see
// https://stackoverflow.com/a/12518877/1048862
return false
}

Here it tried to stat testdata/foo.php.PHP/index.php, but linux's file system is case sensitive:

caddy/modules/caddyhttp/fileserver/testdata$ ls
foo.php.php  index.php  notphp.php.txt  remote.php

So the flow fell into err != nil block and false was returned.

I guess what we really are trying to test is the case insensitivity of
firstSplit. So a new test function is better for that.
@mholt mholt merged commit c054a81 into master Aug 1, 2020
@mholt mholt deleted the fix-test-linux branch August 1, 2020 18:43
@mholt
Copy link
Member Author

mholt commented Aug 1, 2020

Thank you @Vigilans ! I bypassed that part of the code for the test in the end so it won't matter what FS it runs on.

@mholt mholt added this to the v2.2.0 milestone Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants