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

Unrelated testcases fail when total number of tests is increased beyond 367 #4275

Closed
sumezulike opened this issue Mar 14, 2024 · 3 comments · Fixed by #4287
Closed

Unrelated testcases fail when total number of tests is increased beyond 367 #4275

sumezulike opened this issue Mar 14, 2024 · 3 comments · Fixed by #4287
Labels
T: bug Something isn't working

Comments

@sumezulike
Copy link
Contributor

sumezulike commented Mar 14, 2024

Describe the bug

An extremely strange bug concerning development:
If the number of testcases collected by pytest exceeds 367, and pytest is run with the argument --numprocesses with a value of 4, 6, or 8, the test cases test_get_sources_with_stdin_filename_and_exclude and test_get_sources_with_stdin_filename_and_extend_exclude fail.

To Reproduce

  • Clone this repo, as of writing the most recent commit on main is 1abcffc.
  • Follow the guide to install the dependencies.
  • Run tox -e py -- --numprocesses 4 and verify that all tests pass.
  • Note the number of collected test items, currently it is 355.
  • Raise the number to 368, for example by creating files in tests/data/cases, by adding parameters to a parameterized test or by appending the below to a test file, e.g tests/test_black.py
def test_nothing_1():
    pass
def test_nothing_2():
    pass
def test_nothing_3():
    pass
def test_nothing_4():
    pass
def test_nothing_5():
    pass
def test_nothing_6():
    pass
def test_nothing_7():
    pass
def test_nothing_8():
    pass
def test_nothing_9():
    pass
def test_nothing_10():
    pass
def test_nothing_11():
    pass
def test_nothing_12():
    pass
def test_nothing_13():
    pass
  • Run tox -e py -- --numprocesses 4 and expect the following failures:
FAILED tests/test_black.py::TestFileCollection::test_get_sources_with_stdin_filename_and_exclude - AssertionError: assert [] == [PosixPath('_...xclude/a.py')]
FAILED tests/test_black.py::TestFileCollection::test_get_sources_with_stdin_filename_and_extend_exclude - AssertionError: assert [] == [PosixPath('_...xclude/a.py')]
  • Remove one of the test cases.
  • Run tox -e py -- --numprocesses 4 and verify that all tests pass again.

Expected behavior

Well, not that.

Environment

$ black --version
black, 24.2.1.dev11+g1abcffc.d20240314 (compiled: no)
Python (CPython) 3.11.8

Pytest environment output:

platform linux -- Python 3.11.8, pytest-8.1.1, pluggy-1.4.0
cachedir: .tox/py/.pytest_cache
rootdir: /home/samson/PycharmProjects/black
configfile: pyproject.toml
plugins: xdist-3.5.0, cov-4.1.0

OS: EndeavourOS Linux x86_64
Kernel: 6.7.9-arch1-1

Additional context

I've reproduced this on one other machine.
That's not a large sample size, but the consistency of exactly those two tests failing for the same --numprocesses values and the same number of tests makes me think it's a real issue.

@sumezulike sumezulike added the T: bug Something isn't working label Mar 14, 2024
@asottile-sentry
Copy link

because I was bored I decided to throw detect-test-pollution at this:

$ # get the tests from that xdist worker -- exercise left to the reader but I put them in `testlist`
$ detect-test-pollution --testids testlist --failing $(tail -1 testlist)
discovering all tests...
-> pre-discovered 28 tests!
ensuring test passes by itself...
-> OK!
ensuring test fails with test group...
-> OK!
running step 1:
- 27 tests remaining (about 5 steps)
running step 2:
- 14 tests remaining (about 4 steps)
running step 3:
- 7 tests remaining (about 3 steps)
running step 4:
- 4 tests remaining (about 2 steps)
running step 5:
- 2 tests remaining (about 1 steps)
double checking we found it...
-> the polluting test is: tests/test_black.py::TestFileCollection::test_get_sources_with_stdin_symlink_outside_root

and sure enough:

$ pytest tests/test_black.py::TestFileCollection::test_get_sources_with_stdin_symlink_outside_root $(tail -1 testlist)
============================= test session starts ==============================
platform darwin -- Python 3.12.2, pytest-8.1.1, pluggy-1.4.0
rootdir: /private/tmp/black
configfile: pyproject.toml
plugins: cov-4.1.0, xdist-3.5.0
collected 2 items                                                              

tests/test_black.py .F                                                   [100%]

=================================== FAILURES ===================================
__ TestFileCollection.test_get_sources_with_stdin_filename_and_extend_exclude __

self = <tests.test_black.TestFileCollection object at 0x102f98fb0>

    def test_get_sources_with_stdin_filename_and_extend_exclude(self) -> None:
        # Extend exclude shouldn't exclude stdin_filename since it is mimicking the
        # file being passed directly. This is the same as
        # test_exclude_for_issue_1572
        src = ["-"]
        path = THIS_DIR / "data" / "include_exclude_tests"
        stdin_filename = str(path / "b/exclude/a.py")
        expected = [f"__BLACK_STDIN_FILENAME__{stdin_filename}"]
>       assert_collected_sources(
            src,
            root=THIS_DIR.resolve(),
            expected=expected,
            extend_exclude=r"/exclude/|a\.py",
            stdin_filename=stdin_filename,
        )

tests/test_black.py:2742: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

src = ['-']
expected = ['__BLACK_STDIN_FILENAME__/private/tmp/black/tests/data/include_exclude_tests/b/exclude/a.py']

    def assert_collected_sources(
        src: Sequence[Union[str, Path]],
        expected: Sequence[Union[str, Path]],
        *,
        root: Optional[Path] = None,
        exclude: Optional[str] = None,
        include: Optional[str] = None,
        extend_exclude: Optional[str] = None,
        force_exclude: Optional[str] = None,
        stdin_filename: Optional[str] = None,
    ) -> None:
        gs_src = tuple(str(Path(s)) for s in src)
        gs_expected = [Path(s) for s in expected]
        gs_exclude = None if exclude is None else compile_pattern(exclude)
        gs_include = DEFAULT_INCLUDE if include is None else compile_pattern(include)
        gs_extend_exclude = (
            None if extend_exclude is None else compile_pattern(extend_exclude)
        )
        gs_force_exclude = None if force_exclude is None else compile_pattern(force_exclude)
        collected = black.get_sources(
            root=root or THIS_DIR,
            src=gs_src,
            quiet=False,
            verbose=False,
            include=gs_include,
            exclude=gs_exclude,
            extend_exclude=gs_extend_exclude,
            force_exclude=gs_force_exclude,
            report=black.Report(),
            stdin_filename=stdin_filename,
        )
>       assert sorted(collected) == sorted(gs_expected)
E       AssertionError: assert [] == [PosixPath('_...xclude/a.py')]
E         
E         Right contains one more item: PosixPath('__BLACK_STDIN_FILENAME__/private/tmp/black/tests/data/include_exclude_tests/b/exclude/a.py')
E         Use -v to get more diff

tests/test_black.py:2341: AssertionError
=========================== short test summary info ============================
FAILED tests/test_black.py::TestFileCollection::test_get_sources_with_stdin_filename_and_extend_exclude - AssertionError: assert [] == [PosixPath('_...xclude/a.py')]
========================= 1 failed, 1 passed in 0.10s ==========================

smells to me like a rogue lru_cache -- let's try commenting out the one most related to Path.resolve:

diff --git a/src/black/files.py b/src/black/files.py
index c0cadbf..a92d011 100644
--- a/src/black/files.py
+++ b/src/black/files.py
@@ -48,7 +48,7 @@ def _load_toml(path: Union[Path, str]) -> Dict[str, Any]:
         return tomllib.load(f)
 
 
-@lru_cache
+#@lru_cache
 def _cached_resolve(path: Path) -> Path:
     return path.resolve()
  

bingo!

$ pytest tests/test_black.py::TestFileCollection::test_get_sources_with_stdin_symlink_outside_root $(tail -1 testlist)
============================= test session starts ==============================
platform darwin -- Python 3.12.2, pytest-8.1.1, pluggy-1.4.0
rootdir: /private/tmp/black
configfile: pyproject.toml
plugins: cov-4.1.0, xdist-3.5.0
collected 2 items                                                              

tests/test_black.py ..                                                   [100%]

============================== 2 passed in 0.10s ===============================

classic test pollution :)

@asottile-sentry
Copy link

introduced in 23dfc5b cc @hauntsaninja

hauntsaninja added a commit to hauntsaninja/black that referenced this issue Mar 20, 2024
@hauntsaninja
Copy link
Collaborator

Oh amazing, thank you asottile for figuring it out and sumezulike for the issue report! #4287 should fix

hauntsaninja added a commit that referenced this issue Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants