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

Pytest 8.0.2 may collect tests multiple times under Windows #12039

Closed
mrbean-bremen opened this issue Feb 27, 2024 · 11 comments · Fixed by #12050
Closed

Pytest 8.0.2 may collect tests multiple times under Windows #12039

mrbean-bremen opened this issue Feb 27, 2024 · 11 comments · Fixed by #12050

Comments

@mrbean-bremen
Copy link
Member

I'm not quite sure if this is a bug, or a problem with the test or GitHub Actions, but since version 8.0.2 a test in pytest-order fails in the CI under Windows, which worked fine in 8.0.1 and previous versions. Linux and macOS tests are not affected.

Pytest run environment in the CI:

platform win32 -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0 -- D:\a\pytest-order\pytest-order\.tox\py311-pytest80\Scripts\python.EXE
cachedir: .tox\py311-pytest80\.pytest_cache
rootdir: D:\a\pytest-order\pytest-order
plugins: cov-2.9.0, dependency-0.6.0, mock-3.12.0, xdist-3.5.0

The same happens for Python 3.8 - 3.12.

The actual test should collect 6 items (3 tests per test module, in 2 modules) that are created in the directory provided by the tempdir fixture,
but collects 24 tests in the CI instead. Locally, it works as expected.

Checking the failed tests revealed that the tests have been collected from different locations. The tempdir had been at:
C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-9\\test_xdist_ordering0
(for a specific run), but the same tests have been discovered with a test ID with just the qualified test name, and additionally with the test IDs:
::::Users::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-9:::xxx and
::::Documents and Settings::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-9::xxx,
which are most likely aliases.

I haven't dug into this yet, but I suspect it may have something to do with the fix for the problem with short paths.

@nicoddemus
Copy link
Member

Hi @mrbean-bremen, thanks for the report!

Probably it is related to the fix for short paths... perhaps it is collecting the soft-links that tmpdir creates? 🤔

def _force_symlink(
root: Path, target: Union[str, PurePath], link_to: Union[str, Path]
) -> None:
"""Helper to create the current symlink.
It's full of race conditions that are reasonably OK to ignore
for the context of best effort linking to the latest test run.
The presumption being that in case of much parallelism
the inaccuracy is going to be acceptable.
"""
current_symlink = root.joinpath(target)
try:
current_symlink.unlink()
except OSError:
pass
try:
current_symlink.symlink_to(link_to)
except Exception:
pass

mrbean-bremen added a commit to mrbean-bremen/pytest-order that referenced this issue Feb 28, 2024
@mrbean-bremen
Copy link
Member Author

@nicoddemus - could be... I have unsucessfully tried to reproduce this locally (using junctions), I may have another look later, maybe tomorrow.

mrbean-bremen added a commit to pytest-dev/pytest-order that referenced this issue Feb 29, 2024
@mrbean-bremen
Copy link
Member Author

I can confirm that removing the fix for short paths (e.g. the line that uses os.path.samefile for the match) fixes the problem in the CI. Still no luck to reproduce this locally.

@mrbean-bremen
Copy link
Member Author

@nicoddemus: I think the test using samefile is too broad, as it also finds symlinks and junctions, which we don't wont.
I tested to adapt the check to filter out links and junctions to regular directories, which fixes the problem in the CI:

if sys.platform == "win32" and not is_match:
    # In case the file paths do not match,
    # account for short-paths on Windows (#11895).
    same_file = os.path.samefile(node.path, matchparts[0])
    # we don't want to find links or junctions, so we at least
    # exclude links/junctions to regular directories
    is_match = (
            same_file and
            os.path.islink(node.path) == os.path.islink(matchparts[0]) and
            _is_junction(node.path) == _is_junction(matchparts[0])
    )

(is_junction is basically a copy of the Python 3.12 implementation of os.path.junction).
This does not not exclude links to other links to the same directory, but I would say that these are very unlikely cases.
Directly checking the short paths using ctypes and GetShortPath (which might be more correct) is possible, but ugly...

Please let me know if this makes sense...

@nicoddemus
Copy link
Member

Hmm it does make sense.

Just the islink check is enough to fix your CI? If so, I think we can easily just add this extra check. Junctions are more rare (I believe), so I think we can leave them out of this fix for now, as it requires extra steps (or we can just check for them in 3.12 at least).

@mrbean-bremen
Copy link
Member Author

I wouldn't say that junctions are rare (I use them all the time under Windows, as you need admin rights to create symlinks there), but maybe they are indeed rare for use cases with pytest. I will check tomorrow, if the check for links is sufficient for the CI.

@mrbean-bremen
Copy link
Member Author

Ok, I checked. You were right - the problem is indeed solved with the islink check alone.

@nicoddemus
Copy link
Member

Thanks @mrbean-bremen.

However I'm having trouble reproducing this in a test, this is what I came up with:

import pytest


def test_double_collect_symlinks(tmpdir) -> None:
    test_path = tmpdir.join("test_double_collect_symlinks.py")
    test_path.write("def test(): pass")

    result = pytest.main([str(tmpdir)])
    assert result == 0

However this executes just 1 test in several pytest versions I tried: 7.4.4, 8.0.1, 8.0.2, main:

λ pytest .tmp\test_double_collect.py -s
================================================= test session starts =================================================
platform win32 -- Python 3.10.10, pytest-8.0.2, pluggy-1.4.0
rootdir: e:\projects\pytest\.tmp
configfile: pytest.ini
plugins: hypothesis-6.84.0, replay-1.4.0
collected 1 item

.tmp\test_double_collect.py ================================================= test session starts =================================================
platform win32 -- Python 3.10.10, pytest-8.0.2, pluggy-1.4.0
rootdir: E:\.tmp\pytest-of-Pichau\pytest-375\test_double_collect_symlinks0
plugins: hypothesis-6.84.0, replay-1.4.0
collected 1 item

..\..\.tmp\pytest-of-Pichau\pytest-375\test_double_collect_symlinks0\test_double_collect_symlinks.py .           [100%]

================================================== 1 passed in 0.01s ==================================================
.

================================================== 1 passed in 0.09s ==================================================

Can you try to reproduce this problem outside pytest-order's test suite?

@mrbean-bremen
Copy link
Member Author

mrbean-bremen commented Mar 2, 2024

Looks like the problem happens only with pytester, e.g. if I run tests using pytester.
EDIT: pytester is not involved, actually.

I have set up a minimal repository to show this.
The test is trivial:

import pytest

def test_collecting_symlinks(tmpdir):
    testname = str(tmpdir.join("first_test.py"))
    with open(testname, "w") as fi:
        fi.write(
"""
def test_1():
    pass
"""
        )

    args = ["-v", str(tmpdir)]
    ret = pytest.main(args, [])
    assert ret == 0

and under Linux the output in the CI is as expected:

collecting ... collected 1 item

../../../../../tmp/pytest-of-runner/pytest-0/test_collecting_symlinks0/first_test.py::test_1 PASSED [100%]

while under Windows I get:

collecting ... collected 8 items

::::Documents and Settings::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-0::test_collecting_symlinks0::first_test.py::test_1 PASSED [ 12%]
::::Documents and Settings::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-0::test_collecting_symlinkscurrent::first_test.py::test_1 PASSED [ 25%]
::::Documents and Settings::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-current::test_collecting_symlinks0::first_test.py::test_1 PASSED [ 37%]
::::Documents and Settings::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-current::test_collecting_symlinkscurrent::first_test.py::test_1 PASSED [ 50%]
first_test.py::test_1 PASSED                                             [ 62%]
::::Users::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-0::test_collecting_symlinkscurrent::first_test.py::test_1 PASSED [ 75%]
::::Users::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-current::test_collecting_symlinks0::first_test.py::test_1 PASSED [ 87%]
::::Users::runneradmin::AppData::Local::Temp::pytest-of-runneradmin::pytest-current::test_collecting_symlinkscurrent::first_test.py::test_1 PASSED [100%]

which is quite a lot of duplication...

@mrbean-bremen
Copy link
Member Author

Actually I wrote a test very similar to yours, and it passed locally, but failed in the CI (though with 2 tests passed, not 8, in this case):

    @pytest.mark.skipif(not sys.platform.startswith("win"), reason="Windows only")
    def test_collect_symlinks_windows(pytester: Pytester, tmpdir) -> None:
        """Regression test for #12039: Tests collected multiple times under Windows."""
        test_file = Path(tmpdir) / "symlink_collection_test.py"
        test_file.write_text("def test(): pass", encoding="UTF-8")
        result = pytester.runpytest(tmpdir)
>       assert result.parseoutcomes() == {"passed": 1}
E       AssertionError: assert {'passed': 2} == {'passed': 1}
E         
E         Differing items:
E         {'passed': 2} != {'passed': 1}
E         Use -v to get more diff

mrbean-bremen added a commit to mrbean-bremen/pytest that referenced this issue Mar 3, 2024
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI.
This checks additionally that one of the files is not a symlink.

Fixes pytest-dev#12039.
mrbean-bremen added a commit to mrbean-bremen/pytest that referenced this issue Mar 3, 2024
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI.
This checks additionally that one of the files is not a symlink.

Fixes pytest-dev#12039.
mrbean-bremen added a commit to mrbean-bremen/pytest that referenced this issue Mar 3, 2024
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI.
This checks additionally that one of the files is not a symlink.

Fixes pytest-dev#12039.
mrbean-bremen added a commit to mrbean-bremen/pytest that referenced this issue Mar 3, 2024
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI.
This checks additionally that one of the files is not a symlink.

Fixes pytest-dev#12039.
@mrbean-bremen
Copy link
Member Author

I made a PR for this.

nicoddemus added a commit that referenced this issue Mar 3, 2024
The check for short paths under Windows via os.path.samefile, introduced in #11936, also found similar tests in symlinked tests in the GH Actions CI.

Fixes #12039.

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
flying-sheep pushed a commit to flying-sheep/pytest that referenced this issue Apr 9, 2024
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI.

Fixes pytest-dev#12039.

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
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 a pull request may close this issue.

2 participants