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

--import-mode=importlib breaks assertion rewriting #12044

Open
y2kbugger opened this issue Mar 1, 2024 · 12 comments · May be fixed by #12313
Open

--import-mode=importlib breaks assertion rewriting #12044

y2kbugger opened this issue Mar 1, 2024 · 12 comments · May be fixed by #12313
Labels
topic: rewrite related to the assertion rewrite mechanism

Comments

@y2kbugger
Copy link

[tool.pytest.ini_options]
minversion = "8.0"
python_files = "*.test.py"
addopts = [
    "--import-mode=importlib",
    ]

Using --import-mode=importlib is more lenient than the default import-mode, and so it allowed me to use *.test.py rather than a valid module name e.g. *_.test.py.

Everything seemed to work correctly except explanations were missing:

    def test_answer():
>       assert inc(3) == 5
E       AssertionError

insync/db.test.py:34: AssertionError

instead of

    def test_answer():
>       assert inc(3) == 5
E       assert 4 == 5
E        +  where 4 = inc(3)

insync/db_test.py:34: AssertionError
@y2kbugger y2kbugger changed the title Bad python module name breaks assertion rewriting --import-mode=importlib breaks assertion rewriting Mar 12, 2024
@y2kbugger
Copy link
Author

Ok I was missing that after turning importlib back on, the issue returns, so in fact rewriting fails when importlib is turned on.

@Zac-HD Zac-HD added the topic: rewrite related to the assertion rewrite mechanism label Mar 12, 2024
@RonnyPfannschmidt
Copy link
Member

We need to verify what's up here

I suspect we are missing a bit where import lib will correctly fail for invalid module names, thus the assertions rewriter does as well, and then we incorrectly fall back to normal module loading

Most likely we try to find a test module inside a folder after not doing a sanity check on the filename, after that fails the normal import lib machinery takes over finding the module by path and working

I won't be able to verify before next week myself

@estyrke
Copy link

estyrke commented Apr 8, 2024

I am having this issue as well. It seems that it only happens if I have my tests in a package:

root/
  my_module/
    __init__.py
    file.py
  tests/
    __init__.py
    test_main.py

If I remove __init__.py in the tests folder the assert rewriting works even in importlib mode. I should also note that my test files are valid module names, unlike the OP's.

@tommie-lie
Copy link

I've had the same issue, the fix from @estyrke works, the test directories were erroneously packages, removing the __init__.pys fixes assertion rewriting.

mwchase added a commit to mwchase/camel that referenced this issue Apr 12, 2024
@isra17
Copy link

isra17 commented May 10, 2024

Hitting the same problems, we keep out tests in the packages, but we need importlib to work-around issues with namespaced package. Keeping our tests inside the namespaced package, even by removing the __init__.py from the test folder breaks assertion rewriting when we upgraded from 8.0.2 to 8.2.0.

Moving the tests outside the namespace package fix it, but it's an unfortunate limitation.

@isra17
Copy link

isra17 commented May 11, 2024

Actually, it only keep happening with the config:

[pytest]
consider_namespace_packages = true

Without this, removing the __init__.py from a test folder fix it.

@jaraco
Copy link
Contributor

jaraco commented Jun 1, 2024

I'd noticed this issue recently in a number of my projects. I observed the issue most recently in the cssutils project when another user's bug report was missing helpful detail. cssutils's tests are found in cssutils/tests/.

I was able to reproduce the issue simply with:

 draft @ tree
.
└── cssutils
    ├── __init__.py
    └── test_simple.py

2 directories, 2 files
 draft @ cat cssutils/*
class val:
    val = 1


def test_something_simple():
    assert val.val == 0
 draft @ pip-run pytest -- -m pytest --import-mode importlib cssutils/test_simple.py
============================================================== test session starts ===============================================================
platform darwin -- Python 3.12.3, pytest-8.2.1, pluggy-1.5.0
rootdir: /Users/jaraco/draft
collected 1 item                                                                                                                                 

cssutils/test_simple.py F                                                                                                                  [100%]

==================================================================== FAILURES ====================================================================
_____________________________________________________________ test_something_simple ______________________________________________________________

    def test_something_simple():
>       assert val.val == 0
E       AssertionError

cssutils/test_simple.py:6: AssertionError
============================================================ short test summary info =============================================================
FAILED cssutils/test_simple.py::test_something_simple - AssertionError
=============================================================== 1 failed in 0.01s ================================================================

Moving the test to the root, removing the __init__.py, not using --import-mode importlib, or downgrading to pytest<8.1 restores the assert rewrites.

 draft [1] @ pip-run 'pytest<8.1' -- -m pytest --import-mode importlib cssutils/test_simple.py
============================================================== test session starts ===============================================================
platform darwin -- Python 3.12.3, pytest-8.0.2, pluggy-1.5.0
rootdir: /Users/jaraco/draft
collected 1 item                                                                                                                                 

cssutils/test_simple.py F                                                                                                                  [100%]

==================================================================== FAILURES ====================================================================
_____________________________________________________________ test_something_simple ______________________________________________________________

    def test_something_simple():
>       assert val.val == 0
E       assert 1 == 0
E        +  where 1 = val.val

cssutils/test_simple.py:6: AssertionError
============================================================ short test summary info =============================================================
FAILED cssutils/test_simple.py::test_something_simple - assert 1 == 0
=============================================================== 1 failed in 0.01s ================================================================

In my projects, I'm unable to use any of those workarounds, so I'm hoping there's a more permanent fix in the works.

@jmehnle
Copy link

jmehnle commented Jun 18, 2024

I just wasted about 8 hours trying to understand why I was no longer getting assertions rewritten and useful assertion failure output printed. Only after an ugly git bisect, staring at the culprit commit for another hour, and removing __init__.py files below the tests/ directory on a whim, did it start working again.

I wish the importance of not having __init__.py files with --import-mode=importlib was called out in the docs at https://docs.pytest.org/en/stable/explanation/goodpractices.html#test-layout.

@lesteve
Copy link
Contributor

lesteve commented Jun 19, 2024

@jmehnle as a pytest user trying to understand why somehow we lost pytest assertion rewriting in our project, and bumping into this issue (which is probably unrelated by the way but I need to continue my journey to be sure ...), I can definitely relate to the frustration.

As a open-source project maintainer (not a Pytest maintainer just to be 100% clear), I wish people making this kind of comments would understand how draining this is in the long run for maintainers, in particular this part:

I just wasted about 8 hours trying to understand why I was no longer getting assertions rewritten and useful assertion failure output printed.

On this kind of topics, I definitely recommend Settings Expectation for open-source participation https://snarky.ca/setting-expectations-for-open-source-participation/. There are some videos of the talk you can find online as well if you prefer video.

@isra17
Copy link

isra17 commented Jun 19, 2024

By the way, I do have a fix pending for review: #12313 (We internally use a fork of pytest with this fix successfully)

@jnussbaum
Copy link

I'm having the same issue, but #12313 fixes it. Thank you very much for your effort, @isra17

@zbentley
Copy link

zbentley commented Aug 3, 2024

Experienced the same problem; confirmed that #12313 fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: rewrite related to the assertion rewrite mechanism
Projects
None yet
Development

Successfully merging a pull request may close this issue.