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

x/sys/unix: TestGetwd is improperly written #26678

Closed
knz opened this issue Jul 29, 2018 · 8 comments
Closed

x/sys/unix: TestGetwd is improperly written #26678

knz opened this issue Jul 29, 2018 · 8 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@knz
Copy link

knz commented Jul 29, 2018

Please answer these questions before submitting your issue. Thanks!

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

1.11beta2

Does this issue reproduce with the latest release?

unsure

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

freebsd/amd64

What did you do?

ran bash all.bash in the src sub-directory (building from the git repo)

What did you expect to see?

all tests succeeded

What did you see instead?

--- FAIL: TestGetwd (0.00s)
    syscall_unix_test.go:493: Getwd returned "/data/usr/bin" want "/usr/bin"
FAIL
FAIL    cmd/vendor/golang.org/x/sys/unix        0.372s

Extra information: on my machine /usr is a symlink to /data/usr. This is pretty common on unix systems with multiple filesystems. The test should not be sensitive to this.

@meirf meirf changed the title TestGetwd for sys/unix is improperly written x/sys/unix: TestGetwd is improperly written Jul 29, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jul 29, 2018
@robpike
Copy link
Contributor

robpike commented Jul 29, 2018

In general there is no protection against this, as Unix does not keep a canonical path for a file. (Shameless plug: See https://9p.io/sys/doc/lexnames.html.)

The test could perhaps find a safer solution but sysadmin creativity will eventually defeat it. Maybe use /etc? I don't know. Needs thought.

@knz
Copy link
Author

knz commented Jul 29, 2018

A suggestion:

  • make a temp dir.
  • chdir to the temp dir, compute Getwd. Keep for later below.
  • make a sub-dir in the temp dir.
  • make a symlink to the sub-dir.
  • chdir to the symlink
  • compute Getwd again.
  • Check that the new Getwd is the expected sub-dir of the original result of Getwd (after symlink resolution).

@robpike
Copy link
Contributor

robpike commented Jul 30, 2018

Of course one can do that, but that depends on mkdir working, which if you think about it is a far higher bar than checking whether getwd is working. It's like proving the circle's circumference is 2πr using calculus.

And the proposed test only tests that it is consistent, not that it is correct. That's a lot of work for almost no information.

I'm not saying you're wrong, just that it's not a very good test. It may be hard to do better.

@knz
Copy link
Author

knz commented Jul 30, 2018

  1. checking the relative consistency of getwd across directories and sub-directories is, IMHO, what applications care most about (that, together with the relative consistency of getwd and abspath)

  2. There is another directory structure that's already there for you where you control the existence of symlinks: the go source tree itself. The test can check the consistency of getwd there without relying on mkdir.

@ALTree ALTree added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Testing An issue that has been verified to require only test changes, not just a test failure. labels Jul 30, 2018
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 1, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 1, 2018
@virtualsue
Copy link

virtualsue commented Nov 6, 2018

The test is brittle, as discussed, but it can be strengthened by adding more directory test targets, then examining each one in the testing loop and skipping any that don't exist or contain symbolic links. I have revised test code that does this.

@gopherbot
Copy link

Change https://golang.org/cl/147777 mentions this issue: x/sys/unix: fix brittle test that contained dirs which might be symlinks, hence spuriously fail

@virtualsue
Copy link

Second try in the correct repo. I'll check the previous gerrit comments again to see if I addressed the feedback completely. golang/sys#22

@gopherbot
Copy link

Change https://golang.org/cl/148537 mentions this issue: unix: rework TestGetwdto hand test dirs whose names contain symlinks

@golang golang locked and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. 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

6 participants