-
- Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
base: main
Are you sure you want to change the base?
Disable assertion rewriting external modules #13421
Conversation
Need to squash commits - OK to close, I'll reopen a new one with one commit |
There was a problem hiding this 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
23dcf0c
to 54f46d7
Compare 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 |
6a9b406
to 1592f85
Compare
At present time the fix is applied only for path which applies |
Added some tests for plugin rewriting, it works now |
16f3fc9
to ba4263d
Compare 4e1f8b9
to db7dcd4
Compare 3b36041
to 23e0d70
Compare There was a problem hiding this 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.
testing/test_assertrewrite.py Outdated
with mock.patch.object(hook, "fnpats", ["*.py"]): | ||
assert hook.find_spec("conftest") is not None | ||
| ||
def test_assert_rewrite_correct_for_plugins( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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%
26b5195
to c4e1bae
Compare Do we need to rewrite contest plugins? It's the not original behavior |
Conftest files that are part of the collection should rewrite as far as I remember |
my understanding is that rewrite of conftest plugins was always happening as part of the original assertion hooking |
179b723
to edcf484
Compare 3d26321
to 2df4e2b
Compare 2df4e2b
to a77ba12
Compare I see code coverage is less than 100%(96%), maybe add some unit tests? |
I would scratch the explicit files in |
Thank you a lot. I would try to invoke the script in the acceptance tests. |
26fe716
to cd22caa
Compare Stuck on relative import - digging in |
cd22caa
to dc11901
Compare
Disable assertion rewriting of external modules. Closes 13403.