os: spurious TestDirFS failures due to directory mtime skew on Windows

[bcmills]

https://storage.googleapis.com/go-build-log/f31194a9/windows-386-2008_061172ea.log:

--- FAIL: TestDirFS (0.35s)
    os_test.go:2690: TestFS found errors:
        testdata/simple: mismatch:
        	entry.Info() = simple IsDir=true Mode=drwxrwxrwx Size=0 ModTime=2020-11-16 02:32:02.0111083 +0000 GMT
        	file.Stat() = simple IsDir=true Mode=drwxrwxrwx Size=0 ModTime=2020-11-16 02:32:02.012085 +0000 GMT
        
FAIL
FAIL	os	2.635s

Marking as release-blocker for Go 1.16 because DirFS is a new API.

CC @rsc @robpike @alexbrainman @networkimprov @zx2c4

[ianlancetaylor]

I don't understand why there is a directory testdata/simple at all. Where is that coming from?

[bcmills]

This failure occurred in a TryBot for CL 270198. I had thought the changes in that CL were unrelated to the failure, but apparently they are somehow actually the cause of it.

[aclements]

Bump. @bcmills , perhaps you could clarify the problem here? IIUC, the CL this failed in added a testdata file, which caused the mod times in the DirFS test to disagree. But is this an issue with the API? With its implementation? With the test?

[bcmills]

Looking at the implementation of the test, it's either a problem with the test itself, with the new testing/fstest package, or both. Right now, I'm leaning toward this being a problem with os.TestDirFS proper.

It appears that fstest.TestFS assumes that no file or directory in the given fs.FS is changed in any way while the test is running, but then os.TestDirFS is running TestFS on the directory containing the os/signal package. That causes the test for the os package to fail if there are any concurrent changes in the os/signal package.

I don't know why the directory's ModTime was being changed at all in the CL at which this failure was observed; that may be a peripherally-related bug in go run.

However, I could quite easily imagine someone running go test os while concurrently editing an otherwise-unrelated source file in os/signal (such as a test function for that package), and in that situation I could easily envision os.TestDirFS spuriously failing.

If os.TestDirFS requires the property that the directory it is traversing does not change at all while the test is running, it should either have its own isolated subdirectory of GOROOT/src/os/testdata, or it should construct its own temporary directory for the duration of the test and run fstest.TestFS on that — not on the signal subdirectory.

[aclements]

It appears that fstest.TestFS assumes that no file or directory in the given fs.FS is changed in any way while the test is running

It seems like it would be hard for it not to assume this. Maybe it's worth documenting?

I'm leaning toward this being a problem with os.TestDirFS proper.

I agree.

This also doesn't seem especially important, though. Would you object to dropping the release-blocker label?

[toothrot]

@bcmills Do you still believe this is a release-blocker? Or is a documentation fix sufficient?