Skip to content

tests: re-harden test_cmdline_python_package_symlink #6733

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

Closed
wants to merge 8 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Feb 13, 2020

This was done on purpose in 7268462, and then reverted in a non-merge
"Merge" commit (wrong conflict resolution?):
9646a1cd7#diff-1f1fc4eef6bf4c4da558e0dce9e74e9bR761-R762

This was done on purpose in 7268462, and then reverted in a non-merge
"Merge" commit (wrong conflict resolution?):
pytest-dev@9646a1cd7#diff-1f1fc4eef6bf4c4da558e0dce9e74e9bR761-R762
@nicoddemus
Copy link
Member

wrong conflict resolution?

Probably, thanks!

@blueyed
Copy link
Contributor Author

blueyed commented Feb 14, 2020

The Windows behavior in py38 confused me - is it creating only a non-dir symlink there or something?

@blueyed
Copy link
Contributor Author

blueyed commented Feb 14, 2020

So my guess is that this somehow regressed before, but is/was hidden by un-hardening it in two different places (and the initial mixup of checking py.path's attribute, additionally to skipping it etc).
Needs someone with Windows to investigate I guess.

@nicoddemus
Copy link
Member

Needs someone with Windows to investigate I guess.

I'm assigning it to myself, thanks again.

@nicoddemus nicoddemus self-assigned this Feb 14, 2020
blueyed added a commit to blueyed/pytest that referenced this pull request Feb 27, 2020
Taken out of pytest-dev#6733, returning
for existing symlink (should not skip).
@@ -136,6 +137,24 @@ def testdir(testdir: Testdir) -> Testdir:
return testdir


@pytest.fixture
def symlink_or_skip():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly improved version used in #6733.

@nicoddemus
Copy link
Member

Gave this another go, but no luck so far.

It seems that on Linux nodeids are constructed by resolving symlinks, hence we get this output:

lib/foo/bar/test_bar.py::test_bar <- local/lib/foo/bar/test_bar.py PASSED

That output means (nodeid) <- (fspath), built from here:

if self.verbosity >= 2 and nodeid.split("::")[0] != fspath.replace(
"\\", nodes.SEP
):
res += " <- " + self.startdir.bestrelpath(fspath)

On Windows, nodeid and fspath are the symlink:

local/lib/foo/bar/test_bar.py::test_bar PASSED

I have to find where nodeid turns into a symlink. Will need to investigate more some other time.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 28, 2020

@nicoddemus
The interesting part would be why / if symlinks always (seem) to work on Windows.
I've expected this to fail / be skipped there in general.
Also with regard to what py.path uses to determine if there is symlink support.

@nicoddemus
Copy link
Member

The interesting part would be why / if symlinks always (seem) to work on Windows.
I've expected this to fail / be skipped there in general.

Oh sorry I missed that: Administrators and users with special permissions can create symlinks, normal users cannot. I suspect the user running in CI has admin privileges, that's why it works there.

Also with regard to what py.path uses to determine if there is symlink support.

It does the right thing to check for symlinks (just call os.path.islink).
https://github.com/pytest-dev/py/blob/962ad137fcfc52a218fc8acc3d87708eccf00bbc/py/_path/local.py#L373-L374

But yeah I need to set aside more time to investigate further and figure out what's going on.

blueyed added a commit to blueyed/pytest that referenced this pull request Mar 11, 2020
Taken out of pytest-dev#6733, returning
for existing symlink (should not skip).
blueyed added a commit to blueyed/pytest that referenced this pull request Mar 12, 2020
Taken out of pytest-dev#6733, returning
for existing symlink (should not skip).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants