Skip to content

Disable assertion rewriting external modules #13421

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Disable assertion rewriting external modules #13421

wants to merge 20 commits into from
+130 −1

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 •
edited
Loading

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
RonnyPfannschmidt
RonnyPfannschmidt requested changes May 13, 2025
View reviewed changes
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 added 2 commits May 13, 2025 16:44
@Tusenka
13403: Disable assertion rewriting for external modules - move root p…
0cd03d6 
…ath to AssertState
@Tusenka
Merge remote-tracking branch 'origin/disable_assertion_rewriting_exte…
5d5ad4e 
…rnal_modules' into disable_assertion_rewriting_external_modules

# Conflicts:
#	src/_pytest/assertion/rewrite.py
#	testing/test_assertrewrite.py
@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 •
edited
Loading

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 •
edited
Loading

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 added 2 commits May 18, 2025 06:51
@Tusenka
13403: Disable assertion rewriting for external modules - add tests
ad5317b
@Tusenka
Merge remote-tracking branch 'origin/disable_assertion_rewriting_exte…
5efcb78 
…rnal_modules' into disable_assertion_rewriting_external_modules

# Conflicts:
#	testing/test_assertrewrite.py
@Tusenka
Copy link
Author

Tusenka commented May 18, 2025 •
edited
Loading

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 commented May 19, 2025
View reviewed changes
testing/test_assertrewrite.py
with mock.patch.object(hook, "fnpats", ["*.py"]):
assert hook.find_spec("file") is None

def test_assert_rewrite_correct_for_conftfest(
Copy link
Author

Choose a reason for hiding this comment

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

mamy name it in another way - test_assert_rewrite_for_conftfest

@Tusenka Tusenka force-pushed the disable_assertion_rewriting_external_modules branch from 4e1f8b9 to db7dcd4 Compare May 22, 2025 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable assertion rewriting of external modules for python_files = *.py
3 participants
@Tusenka @nicoddemus @RonnyPfannschmidt