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

os: TestRemoveAllWithMoreErrorThanReqSize is failing on Plan 9 and Windows #35117

Closed
fhs opened this issue Oct 23, 2019 · 9 comments
Closed

os: TestRemoveAllWithMoreErrorThanReqSize is failing on Plan 9 and Windows #35117

fhs opened this issue Oct 23, 2019 · 9 comments

Comments

@fhs
Copy link
Contributor

@fhs fhs commented Oct 23, 2019

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

cpu% go version
go version devel +master Wed Oct 23 01:24:58 EDT 2019 plan9/386

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

go env Output
$ go env
GO111MODULE='on'
GOARCH='386'
GOBIN=''
GOCACHE='/usr/fhs/lib/cache/go-build'
GOENV='/usr/fhs/lib/go/env'
GOEXE=''
GOFLAGS=''
GOHOSTARCH='386'
GOHOSTOS='plan9'
GONOPROXY=''
GONOSUMDB=''
GOOS='plan9'
GOPATH='/home/fhs/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/big/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLDIR='/home/big/go/pkg/tool/plan9_386'
GCCGO='gccgo'
GO386='sse2'
AR='ar'
CC='8c'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/dev/null'
CGO_CFLAGS='-g -O2'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-g -O2'
CGO_FFLAGS='-g -O2'
CGO_LDFLAGS='-g -O2'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m32 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build107333689=/tmp/go-build -gno-record-gcc-switches'

What did you do?

cpu% cd src/os
cpu% go test

What did you expect to see?

All tests passed.

What did you see instead?

--- FAIL: TestRemoveAllWithMoreErrorThanReqSize (0.30s)
    removeall_test.go:447: Want error from RemoveAllTestHook, got nil
FAIL
exit status: 'os.test 129292: 1'
FAIL	os	2.458s

This test doesn't get run with the -short flag, so it's probably not getting run on the plan9 builders.

On Plan 9, we use the "noat" implementation, which is slightly different from the "at" implementation in that it can Remove the parent directory within the for-loop in addition to the Remove at the end of the function.

This test tries to delete 1025 files. The first Readdirnames returns 1024 entries. In the second Readdirnames call, we get the 1 remaining entry. After deleting that one entry, instead of breaking out of the loop, the code tries to Remove the parent directory. Since the Remove succeeds, we return nil. I believe we need to an add additional hook right after this Remove call, and another hook right after we try to Open the deleted parent directory (only reached if hook is set by the test).

@gopherbot Please add labels OS-Plan9, NeedsFix

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 23, 2019

Change https://golang.org/cl/202757 mentions this issue: os: fix TestRemoveAllWithMoreErrorThanReqSize on Plan 9

@bradfitz bradfitz added the OS-Windows label Oct 24, 2019
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 24, 2019

@fhs fhs changed the title os: TestRemoveAllWithMoreErrorThanReqSize is failing on Plan 9 os: TestRemoveAllWithMoreErrorThanReqSize is failing on Plan 9 and Windows Oct 24, 2019
@bcmills bcmills added this to the Go1.14 milestone Oct 25, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 25, 2019

Change https://golang.org/cl/203502 mentions this issue: os: use an actual RemoveAll failure in TestRemoveAllWithMoreErrorThanReqSize

@iWdGo

This comment has been minimized.

Copy link
Contributor

@iWdGo iWdGo commented Oct 29, 2019

The test still fails on Windows.
Previous error was:

golang\go\src>go test os -v -run=RemoveAllWithMore
=== RUN   TestRemoveAllWithMoreErrorThanReqSize
--- FAIL: TestRemoveAllWithMoreErrorThanReqSize (1.72s)
    removeall_test.go:447: Want error from RemoveAllTestHook, got nil
FAIL

On tip, error is now:

golang\go\src>goissue test os -v -run=RemoveAllWithMore
=== RUN   TestRemoveAllWithMoreErrorThanReqSize
    TestRemoveAllWithMoreErrorThanReqSize: removeall_test.go:452: RemoveAll(<read-only directory>) = nil; want error
--- FAIL: TestRemoveAllWithMoreErrorThanReqSize (1.08s)

Following #26295 (and others) and as documented, RemoveAll is expected to remove everything it can. The proposed commit removes completely the now unused test hook and returns successfully for Windows.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 29, 2019

Change https://golang.org/cl/204060 mentions this issue: os: return successfully TestRemoveAllWithMoreErrorThanReqSize on Windows

gopherbot pushed a commit that referenced this issue Oct 29, 2019
…succeeds on Windows

Also remove unused test hook.

Updates #35117

Change-Id: I6f05ba234fb09e4b44e77c1539c02d1aed49910a
Reviewed-on: https://go-review.googlesource.com/c/go/+/204060
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 16, 2020

Change https://golang.org/cl/223700 mentions this issue: [release-branch.go1.13] os: use an actual RemoveAll failure in TestRemoveAllWithMoreErrorThanReqSize

@gopherbot

This comment was marked as outdated.

Copy link

@gopherbot gopherbot commented Mar 16, 2020

Change https://golang.org/cl/223701 mentions this issue: [release-branch.go1.13] os: return from TestRemoveAllWithMoreErrorThanReqSize when RemoveAll succeeds on Windows

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 17, 2020

@gopherbot, please backport to Go 1.13. This is a test fix. It is needed to resolve a test failure in the os package on windows-amd64-longtest builder, which could be masking other problems (due to #36181) and making releasing Go 1.13.x more difficult.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 17, 2020

Backport issue(s) opened: #37895 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.