-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add support for xfail #34
Add support for xfail #34
Conversation
8e92c09
to
62cb9f8
Compare
62cb9f8
to
c09429f
Compare
The CI passes. It just shows up as red because of the unrelated flake8 configuration problem which is going to be fixed by #35. |
@RonnyPfannschmidt I remember that you don't want to maintain this. I seem to recall somebody like @nicoddemus mentioning this plugin on Twitter a while back so maybe he'd be interested in doing the release. If not, I'm happy to help with getting the release out of the door. Including upgrading the test matrix (the last pytest and pythons are not there yet), the CI (could migrate it to GHA) and also polishing some minor bits in the repo. |
I'm happy to do handover as well as some minimal support for merges /releases in near future, however I'm Curr in the middle of a move and won't give focus to pytest-forked until mid next week |
@RonnyPfannschmidt cool! FWIW these two PRs are ready for merge right now. They are pretty trivial. |
"reason: {xfail_reason}; " | ||
"pytest-forked reason: {crash_info}". | ||
format( | ||
xfail_reason=xfail_marker.kwargs['reason'], |
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.
This does not consider conditions for xfail, which may trigger wrong xfail
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.
Right... Looks like xfail handling logic is leaking outside of pytest. I'll try to figure out how to do this.
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.
But I consider this to be a good initial implementation because it at least covers some basic cases. I need to come up with better tests to see other behaviors.
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.
its possibly sensible to start this feature as also issuing a warning that it currently may issue incorrect xfails and then follow up later on with a pytest integrated solution for the xfail checking
the problem is that mark evaluation is currently inflexible and internal
i would consider it a valid solution to have a warning about potential false xfails that refers a followup issue and then sorting the details out later
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.
warning as in warnings.warn(UserWarning, 'this is not yet ideal')
?
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.
warning added
c09429f
to
4d84741
Compare
@RonnyPfannschmidt so how do we proceed now? |
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 merge after sorting out the nitpick
38890af
to
eea6536
Compare
@RonnyPfannschmidt still green |
@RonnyPfannschmidt what's the next step? |
A release and followup issues here and in pytest for better mark evaluation |
How can I help? |
@webknjaz it seems that there is a setup for automated pypi uploads, so after change-log is updated for a feature release, i can tag for the pytest side there is only a need for a issue to expose mark evaluation so plugins can use it for own markers |
are you talking about pytest-dev/pytest#7327 ? |
I've started filling it out, checked some related files in the repo and realized that a few more changes could be added so I'm preparing several PRs with changelog and metadata updates |
So here's what I'd like to squeeze in: |
@RonnyPfannschmidt I saw you merged things. Please cut the tag ( |
@webknjaz thanks for the ping, just did that |
Great, so it didn't explode on publish: https://travis-ci.org/github/pytest-dev/pytest-forked/jobs/702022414#L434. |
Resolves #33.