-
Notifications
You must be signed in to change notification settings - Fork 232
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
Check that we can serialize the warning detail attr #379
Check that we can serialize the warning detail attr #379
Conversation
Thanks a lot @aadamson! sigh oh boy do I regret having the idea of transferring warning instances between master and workers. |
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.
LGTM, but can you please provide a regression test for this?
See
pytest-xdist/testing/acceptance_test.py
Line 759 in f206187
def test_unserializable_arguments(self, testdir, n): |
@nicoddemus My pleasure! I think it's a really cool feature :) I'll add that test now |
Awesome, thanks! |
Do you guys have a guide on running the tests locally? |
Take a look at pytest's guide it is the same for pytest-xdist (we use Please let us know if you run into trouble. 👍 |
dac4f14
to
1245bd8
Compare
@nicoddemus picking this back up - I can't repro the issue (or rather, I can't figure out how to construct a test that will replicate the failure, so I can't check if the failure is handled properly). Would you be okay if I only address testing the feature in Python 3 in this PR? |
Go ahead, we can try to help out to reproduce on Python 2. 👍 |
9b516b0
to
66d6e49
Compare
I'm seeing TestWarnings.test_warnings fail in CI with
but I can't find where the |
Oh we need to update the test suite for |
@aadamson you can merge/rebase on |
66d6e49
to
ac2bf10
Compare
Done, thank you! |
Awesome, thank you! |
Thanks for your help! How do I merge? |
Only the maintainers can merge. 😁 I didn't merge yet in case @RonnyPfannschmidt wants to take a look first. |
Thanks for submitting a PR, your contribution is really appreciated!
Here's a quick checklist that should be present in PRs:
Make sure to include reasonable tests for your change if necessary
We use towncrier for changelog management, so please add a news file into the
changelog
folder following these guidelines:Name it
$issue_id.$type
for example588.bugfix
;If you don't have an issue_id change it to the PR id after creating it
Ensure type is one of
removal
,feature
,bugfix
,vendor
,doc
ortrivial
Make sure to use full sentences with correct case and punctuation, for example:
I was running into the same issue addressed in #349, except instead of failing to serialize an arg of the warning, I was unable to serialize a warning detail of the warning. To reproduce the error, you can cause a ResourceWarning due to an unclosed socket. One of the warning details will be the socket that wasn't closed, which can't be serialized.
This PR addresses the issue by performing the same check as #349.