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: TestRemoveAllDotDot broken at head #31421

Closed
bcmills opened this issue Apr 11, 2019 · 6 comments
Closed

os: TestRemoveAllDotDot broken at head #31421

bcmills opened this issue Apr 11, 2019 · 6 comments
Labels
FrozenDueToAge release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages) Testing An issue that has been verified to require only test changes, not just a test failure.

Comments

@bcmills
Copy link
Contributor

bcmills commented Apr 11, 2019

Observed in https://build.golang.org/log/79c45622d3e12177863a8eafbb8772e26af1a30c.

Broken in https://golang.org/cl/171099 (#29921), and the breakage in the longtest builder was masked by #31263.

@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. release-blocker labels Apr 11, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Apr 11, 2019

And some others too:

~/go/src/os$ go test .
--- FAIL: TestRemoveAllDotDot (0.00s)
    removeall_test.go:252: unlinkat /tmp/TestRemoveAllDotDot-695588954/x/y: error from RemoveAllTestHook
--- FAIL: TestRemoveUnreadableDir (0.00s)
    removeall_test.go:406: openfdat /tmp/TestRemoveAllButReadOnly-192841201/d0/d1/d2: permission denied
--- FAIL: TestRemoveAllButReadOnlyAndPathError (0.00s)
    removeall_test.go:360: got "/tmp/TestRemoveAllButReadOnly-172665500/a/x/1", expected pathErr.path "/tmp/TestRemoveAllButReadOnly-172665500/b/y"
FAIL
FAIL    os      18.322s

@bcmills bcmills added the Soon This needs to be done soon. (regressions, serious bugs, outages) label Apr 11, 2019
@bcmills bcmills self-assigned this Apr 11, 2019
@gopherbot
Copy link

Change https://golang.org/cl/171777 mentions this issue: os: fix aliasing bug in RemoveAllTestHook restoration

@cuonglm
Copy link
Member

cuonglm commented Apr 12, 2019

@bcmills Thanks for the fix 👍

Can you tell my why trybot result and local test passed in my original CL?

@ianlancetaylor
Copy link
Contributor

Because the tests are run in short mode by default, and your test wasn't run in short mode.

@cuonglm
Copy link
Member

cuonglm commented Apr 12, 2019

Because the tests are run in short mode by default, and your test wasn't run in short mode.

Ah right, but how can it pass in my local run? I just did go-tip test -v -count=1.

Is there a way I can make sure that my test will run all tests or mimic the builder so we won't get in this trouble next time?

@bcmills
Copy link
Contributor Author

bcmills commented Apr 13, 2019

The os package resolves to whatever os package is present in the GOROOT of the go command (go-tip in your example), so if you built go-tip in a different GOROOT from the one in which you drafted the CL, then go-tip test os might have actually tested a copy of the os package without your changes.

@golang golang locked and limited conversation to collaborators Apr 12, 2020
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages) Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

4 participants