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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 OS-Windows
Milestone

Comments

@fhs
Copy link
Contributor

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 gopherbot added NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 labels Oct 23, 2019
@gopherbot
Copy link

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

@bradfitz
Copy link
Contributor

It's also failing on Windows:
https://storage.googleapis.com/go-build-log/f58f8d03/windows-amd64-longtest_dc0788fb.log

@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
Copy link

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

@iwdgo
Copy link
Contributor

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
Copy link

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
Copy link

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

@gopherbot

This comment has been minimized.

@dmitshur
Copy link
Contributor

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
Copy link

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.

gopherbot pushed a commit that referenced this issue Mar 31, 2020
…moveAllWithMoreErrorThanReqSize

Previously we injected an error, and the injection points were
(empirically) not realistic on some platforms.

Instead, we now make the directory read-only, which (on most
platforms) suffices to prevent the removal of its files.

Also remove unused test hook, as was done in CL 204060.

For #35117.
For #29921.
Fixes #37895.

Change-Id: Ica4e2818566f8c14df3eed7c3b8de5c0abeb6963
Reviewed-on: https://go-review.googlesource.com/c/go/+/203502
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 06bdd52)
Reviewed-on: https://go-review.googlesource.com/c/go/+/223700
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
@golang golang locked and limited conversation to collaborators Mar 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 OS-Windows
Projects
None yet
Development

No branches or pull requests

6 participants