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
Fix reported location of skip when --runxfail is used #7432
Conversation
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.
Hey @AFirouzi thank you for the PR!
Can you please also include the following?
- A changelog as explained here
- Tests for this change.
Sorry, I am a newbie here. If I got your point correctly, I need to write a test for this specific bug right? which in this case is it should be add to the test_skipping.py |
@AFirouzi yes please. I think test_skipping.py sounds like a great place to put it Also, consider using github keywords to close any linked issues otherwise someone has to manually close them. It also has the benefit of showing in the issue that there is a linked PR, so people know someone has a solution up for review |
Fixes #7392 |
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.
Reading over the issue, it would appear that the issue is the reporting.
Adding --runxfail
shows the error as coming from src/_pytest/skipping.py
, but it should show the test file (e.g.: test_it.py
). Can you update the tests to check for this condition? I don't see the tests checking the file location outputted
@bluetech please correct me if I'm wrong
@gnikonorov is right, the problem is not what the current changelog entry states, but merely that The fix is also not quite right; the actual problem is that the "change the location of the failure to point to the item definition" code is currently dependent on previous conditions (uses |
Now I think I got the issue right. I just needed to simply replace last |
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 for working on this @AFirouzi. I left some comments.
(I edited the PR title to be more descriptive, and added |
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.
Looks good now, thanks @AFirouzi!
Fixes #7392.