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.

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

Jump to bottom

testing/fstest: TestFS fails on valid FS if it contains any symlinks #44113

Closed
caarlos0 opened this issue Feb 5, 2021 · 5 comments
Closed

testing/fstest: TestFS fails on valid FS if it contains any symlinks #44113

caarlos0 opened this issue Feb 5, 2021 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@caarlos0
Copy link
Contributor

caarlos0 commented Feb 5, 2021 •
edited
Loading

What version of Go are you using (go version)?

$ go version
go version devel +8869086d8f Thu Feb 4 04:46:49 2021 +0000 darwin/amd64

Also tried with go 1.16 rc1

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ goGO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/carlos/Library/Caches/go-build"
GOENV="/Users/carlos/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/carlos/Developer/Go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/carlos/Developer/Go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/carlos/Developer/forks/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/carlos/Developer/forks/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="devel +8869086d8f Thu Feb 4 04:46:49 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/carlos/Developer/forks/go/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cz/k46fpqfd547_mn0ssjq7b5rr0000gn/T/go-build4008414779=/tmp/go-build -gno-record-gcc-switches -fno-common" env

What did you do?

Example reproducible:

package main

import (
	"os"
	"path/filepath"
	"testing"
	"testing/fstest"
)

func TestTestFS(t *testing.T) {
	tmp := t.TempDir()
	tmpfs := os.DirFS(tmp)

	if err := os.WriteFile(filepath.Join(tmp, "hello"), []byte("hello, world\n"), 0644); err != nil {
		t.Fatal(err)
	}

	if err := os.Symlink(filepath.Join(tmp, "hello"), filepath.Join(tmp, "hello.link")); err != nil {
		t.Fatal(err)
	}

	if err := fstest.TestFS(tmpfs, "hello", "hello.link"); err != nil {
		t.Fatal(err)
	}
}

Seems like it compares the stat of the file vs the stat of the link to the file... which fails (as it should).

Maybe I shouldn't use it against anything but MapFS? I'm not sure.

What did you expect to see?

Test should pass.

What did you see instead?

        hello.link: mismatch:
        	entry = hello.link IsDir=false Type=L---------
        	file.Stat() = hello.link IsDir=false Type=----------
        hello.link: mismatch:
        	entry.Info() = hello.link IsDir=false Mode=Lrwxr-xr-x Size=79 ModTime=2021-02-05 00:46:02.12900541 -0300 -03
        	file.Stat() = hello.link IsDir=false Mode=-rw-r--r-- Size=13 ModTime=2021-02-05 00:46:02.128923494 -0300 -03
@robpike
Copy link
Contributor

robpike commented Feb 5, 2021

A symlink is not a file in the sense of the FS interface, so a decision must be made about what a symlink's behavior should be. One can suggest that the symlink should evaluated, and that the resulting object be referenced, but one can also argue that a symlink is an error because it does not honor the simple FS semantics. The second case is what we see here.

I am not sure what the right answer is, but whatever it is, it should be documented. There is no documentation today and that should be addressed once the decision is made.

@caarlos0
Copy link
Contributor Author

caarlos0 commented Feb 5, 2021 •
edited
Loading

Understood.

For context, I got this while looking into replacing afero.Fs for fs.FS in one the packages I maintain.

The main usage of afero.Fs there was to avoid creating thousands of files in testdata to validate our behavior. So "in production" we use the real FS implementation and when running tests a MemFS (although we could use a real FS on a temp dir as well).

Maybe that's not a use case fs.FS was intent for, but then I agree it should be documented.
And would also be a bit sad because it would be really nice to not use 3rd parties for that...

@bcmills
Copy link
Contributor

bcmills commented Feb 5, 2021 •
edited
Loading

one can also argue that a symlink is an error because it does not honor the simple FS semantics.

I don't think it makes sense for os.DirFS to lie about the types of directory entries or filter out symlink entries, especially given that nothing fundamentally prevents io/fs from gaining an LstatFS extension interface in the future.

For now, I think fstest.TestFS should skip the Stat consistency check for any entry that has a type with ModeSymlink set.

@rsc
Copy link
Contributor

rsc commented Feb 5, 2021

I agree about skipping the Stat consistency check if ModeSymlink is set in the directory listing.
Mailed https://golang.org/cl/290009.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/290009 mentions this issue: testing/fstest: avoid symlink-induced failures in tester

@cagedmantis cagedmantis added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 5, 2021
@LukeShu LukeShu mentioned this issue Dec 31, 2021
Open
@golang golang locked and limited conversation to collaborators Feb 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@rsc @cagedmantis @caarlos0 @robpike @bcmills @gopherbot