-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
capture: ensure name of EncodedFile being a string #2557
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.
Awesome, thanks @blueyed!
Could you please also add a new "bugfix" file to the changelog?
_pytest/capture.py
Outdated
@@ -252,6 +252,8 @@ def writelines(self, linelist): | |||
self.write(data) | |||
|
|||
def __getattr__(self, name): | |||
if name == 'name': |
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.
Please add a small comment here explaining why this special case
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.
Will do, but it is not really clear to me if this is the way to go after all, since it seems to be only necessary for logging.StreamHandler ?!
@RonnyPfannschmidt
What API where you referring to that says this to be a string?
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 think the overall file.name
being a string, IIUC.
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.
hmm after revisiting, why not just make name a property?
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.
Ahh good idea. 👍
_pytest/capture.py
Outdated
@@ -252,6 +252,8 @@ def writelines(self, linelist): | |||
self.write(data) | |||
|
|||
def __getattr__(self, name): | |||
if name == 'name': | |||
return '{!r}'.format(self.buffer) |
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 failed in py26 because the field index is mandatory... but I suggest just using repr(self.buffer)
as it is shorter and simpler.
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.
Makes sense, amended.
_pytest/capture.py
Outdated
@@ -252,6 +252,8 @@ def writelines(self, linelist): | |||
self.write(data) | |||
|
|||
def __getattr__(self, name): | |||
if name == 'name': | |||
return repr(self.buffer) | |||
return getattr(object.__getattribute__(self, "buffer"), name) |
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.
while reading, why is get-attribute used there to begin with ?
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 code was already there, I think the idea is that EncodedFile
should act as a proxy to the underlying buffer
.
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 then buffer is directly usable to begin with, from my pov that call can very likely be shortened to self.buffer
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.
OH that! Hmm yeah at first glance you are right, but I suspect this is there for a reason. We would have to hunt the Git history to see if we can find something about it.
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.
Let's see if it is covered by some test(s): #2614.
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.
test_pickling_and_unpickling_enocded_file
fails:
def test_pickling_and_unpickling_enocded_file():
# See https://bitbucket.org/pytest-dev/pytest/pull-request/194
# pickle.loads() raises infinite recursion if
# EncodedFile.__getattr__ is not implemented properly
ef = capture.EncodedFile(None, None)
ef_as_str = pickle.dumps(ef)
> pickle.loads(ef_as_str)
testing/test_capture.py:1151:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.tox/py36/lib/python3.6/site-packages/_pytest/capture.py:258: in __getattr__
return getattr(self.buffer, name)
.tox/py36/lib/python3.6/site-packages/_pytest/capture.py:258: in __getattr__
return getattr(self.buffer, name)
.tox/py36/lib/python3.6/site-packages/_pytest/capture.py:258: in __getattr__
return getattr(self.buffer, name)
E RecursionError: maximum recursion depth exceeded while calling a Python object
!!! Recursion detected (same locals & position)
97bdc6f
to
5c6a4c9
Compare
Hmm the check brakes in
I think changing the assertion to |
5c6a4c9
to
0603d1d
Compare
Thanks @blueyed! |
Fixes #2555.