Skip to content

Conversation

Tusenka
Copy link

@Tusenka Tusenka commented May 13, 2025

Disable assertion rewriting of external modules. Closes 13403.

@Tusenka
Copy link
Author

Tusenka commented May 13, 2025

Need to squash commits - OK to close, I'll reopen a new one with one commit

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label May 13, 2025
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

thank you for getting this started

i believe we need to put the path handling into assertion state so it can correctly pass from the configuration and include the invocation dir/rootdir in a more safe manner than the current heusterics

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 23dcf0c to 54f46d7 Compare May 16, 2025 05:01
@nicoddemus
Copy link
Member

nicoddemus commented May 16, 2025

I skimmed through the issue (I'm short on time so I cannot do a more through research), but looking at the code is not immediately clear to me so thought I would ask:

Note that we want to rewrite asserts for files belonging to a pytest plugin, even if they are not test_*.py files. How does this patch relate to that? This is an important behavior that should be kept.

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 6a9b406 to 1592f85 Compare May 16, 2025 16:10
@Tusenka
Copy link
Author

Tusenka commented May 16, 2025

I skimmed through the issue (I'm short on time so I cannot do a more through research), but looking at the code is not immediately clear to me so thought I would ask:

Note that we want to rewrite asserts for files belonging to a pytest plugin, even if they are not test_*.py files. How does this patch relate to that? This is an important behavior that should be kept.

At present time the fix is applied only for path which applies test_*py. The plugins are processed separately. I'll add some tests against that important part.

@Tusenka
Copy link
Author

Tusenka commented May 18, 2025

I skimmed through the issue (I'm short on time so I cannot do a more through research), but looking at the code is not immediately clear to me so thought I would ask:
Note that we want to rewrite asserts for files belonging to a pytest plugin, even if they are not test_*.py files. How does this patch relate to that? This is an important behavior that should be kept.

At present time the fix is applied only for path which applies test_*py. The plugins are processed separately. I'll add some tests against that important part.

Added some tests for plugin rewriting, it works now

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 16f3fc9 to ba4263d Compare May 19, 2025 04:13
@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 4e1f8b9 to db7dcd4 Compare May 22, 2025 04:12
@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch 2 times, most recently from 3b36041 to 23e0d70 Compare May 27, 2025 15:53
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @Tusenka,

Left some comments, please take a look.

with mock.patch.object(hook, "fnpats", ["*.py"]):
assert hook.find_spec("conftest") is not None

def test_assert_rewrite_correct_for_plugins(
Copy link
Member

Choose a reason for hiding this comment

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

My previous suggestion about using a real-case scenario makes this test unnecessary.

Copy link
Author

Choose a reason for hiding this comment

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

I see the code coverage in that case is less than 100%

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 26b5195 to c4e1bae Compare June 12, 2025 13:04
@Tusenka
Copy link
Author

Tusenka commented Jun 12, 2025

Do we need to rewrite contest plugins? It's the not original behavior

@RonnyPfannschmidt
Copy link
Member

Conftest files that are part of the collection should rewrite as far as I remember

@RonnyPfannschmidt
Copy link
Member

my understanding is that rewrite of conftest plugins was always happening as part of the original assertion hooking

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch 2 times, most recently from 179b723 to edcf484 Compare June 13, 2025 06:44
@Tusenka
Copy link
Author

Tusenka commented Jun 13, 2025

But it still doen't see conftest plugins
image
Result of provided plugins:
['pytest', 'zope.interface', 'idna', 'anyio', 'sniffio', 'filelock', 'cachetools', 'incremental', 'greenlet', 'pluggy', 'hypothesis', 'Automat', 'wheel', 'virtualenv', 'coverage', 'pip', 'distlib', 'mock', 'pre_commit', 'typing_extensions', 'elementpath', 'pytest-twisted', 'PyYAML', 'pyproject-api', 'identify', 'colorama', 'urllib3', 'hyperlink', 'Pygments', 'sortedcontainers', 'constantly', 'nodeenv', 'xmlschema', 'setuptools', 'attrs', 'requests', 'pytest', 'exceptiongroup', 'decorator', 'platformdirs', 'packaging', 'charset-normalizer', 'cfgv', 'Twisted', 'certifi', 'chardet', 'tox', 'argcomplete', 'iniconfig', 'jaraco.collections', 'zipp', 'typeguard', 'wheel', 'importlib_metadata', 'jaraco.text', 'jaraco.context', 'autocommand', 'tomli', 'typing_extensions', 'more-itertools', 'jaraco.functools', 'packaging', 'platformdirs', 'inflect', 'backports.tarfile']

The conftest plugins are loaded after the PEP302/PEP451 import hook considers files to rewrite.
I propose a separate ticket for conftest plugins rewritng.
I'll try to "play" with this.

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch 2 times, most recently from 3d26321 to 2df4e2b Compare August 9, 2025 08:01
@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 2df4e2b to a77ba12 Compare August 24, 2025 16:27
@Tusenka
Copy link
Author

Tusenka commented Aug 25, 2025

I see code coverage is less than 100%(96%), maybe add some unit tests?
Also mypy doesn't like the packeage name, created a question on stackoverflow
nicoddemus[RonnyPfannschmidt], what do you think?

@nicoddemus
Copy link
Member

@Tusenka

testing/example_scripts/rewrite/src/main.py: error: Source file found twice under different module names: "example_scripts.rewrite.src.main" and "testing.example_scripts.rewrite.src.main"
testing/example_scripts/rewrite/src/main.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
testing/example_scripts/rewrite/src/main.py: note: Common resolutions include: a) adding `__init__.py` somewhere, b) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)

I would scratch the explicit files in testing/example_scripts and create the scripts inside the tests that use them, with pytester. This side steps the issue entirely.

@Tusenka
Copy link
Author

Tusenka commented Aug 29, 2025

@Tusenka

testing/example_scripts/rewrite/src/main.py: error: Source file found twice under different module names: "example_scripts.rewrite.src.main" and "testing.example_scripts.rewrite.src.main"
testing/example_scripts/rewrite/src/main.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
testing/example_scripts/rewrite/src/main.py: note: Common resolutions include: a) adding `__init__.py` somewhere, b) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)

I would scratch the explicit files in testing/example_scripts and create the scripts inside the tests that use them, with pytester. This side steps the issue entirely.

Thank you a lot. I would try to invoke the script in the acceptance tests.

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from a77ba12 to 26fe716 Compare August 31, 2025 13:35
@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 26fe716 to cd22caa Compare August 31, 2025 14:18
@Tusenka
Copy link
Author

Tusenka commented Aug 31, 2025

Stuck on relative import - digging in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable assertion rewriting of external modules for python_files = *.py
3 participants