Skip to content
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 capfdbinary fixture #2925

Merged
merged 1 commit into from
Nov 17, 2017
Merged

Add capfdbinary fixture #2925

merged 1 commit into from
Nov 17, 2017

Conversation

asottile
Copy link
Member

capfdbinary works like capfd but produces bytes for readouterr().

See #2923

@asottile
Copy link
Member Author

CC @nicoddemus

I approached this only from the capfd front, it's slightly more involved to implement this for capsys (because python2 capture io is not predictably typed) -- I think this is fine, capfd is more useful in this case anyway.

Feedback appreciated, I threw this together quickly and might've missed some documentation / naming / etc.

@nicoddemus
Copy link
Member

Thanks a lot @asottile, that was quick! 👍

it's slightly more involved to implement this for capsys (because python2 capture io is not predictably typed)

This is fine, but it is easy to implement capsys-like for Python 3? IMHO it is fine to add Python 3-only functionality if reasonably easy (doesn't depend on new syntax for example). Not sure how others feel about this, but it is time to start considering Python 3 only features when it is not feasible to support them in Python 2.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

I left some small comments, but overall this is top-notch.

@@ -115,6 +115,10 @@ same interface but allows to also capture output from
libraries or subprocesses that directly write to operating
system level output streams (FD1 and FD2).

If the code under test writes non-textual data, you can capture this using
the ``capfdbinary`` function argument which instead returns ``bytes`` from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use "fixtures" instead of "function argument".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 , while writing this I myself wrote "fixtures" but then matched it with the paragraph above -- I'll update both of them

@@ -115,6 +115,10 @@ same interface but allows to also capture output from
libraries or subprocesses that directly write to operating
system level output streams (FD1 and FD2).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add here, before the next paragraph:

.. versionadded:: 3.3

@asottile
Copy link
Member Author

Yeah capsysbinary is pretty easy to implement in python 3 due to the work I did in #2266 (CaptureIO maintains .buffer), it's only difficult because in python2 CaptureIO is TextIO (backed by io.StringIO) which doesn't actually expose a binary storage.

@nicoddemus
Copy link
Member

Yeah capsysbinary is pretty easy to implement in python 3

I'm OK with making capsysbinary available only in Python 3. We should make it explicit in the docs and using the fixture in Python 2 should raise an error.

But let's see what others think about this first.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 92.694% when pulling 123e4f6 on asottile:capfdbinary into 685387a on pytest-dev:features.

`capfdbinary` works like `capfd` but produces bytes for `readouterr()`.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 92.695% when pulling 8f90812 on asottile:capfdbinary into 685387a on pytest-dev:features.

@RonnyPfannschmidt
Copy link
Member

we can start with python3 only, but we should take a look at exposing a encoded bytesio kinda thing in future

@nicoddemus
Copy link
Member

@asottile so I guess we can go ahead with capsysbinary for py3 only then. 👍

@asottile
Copy link
Member Author

@nicoddemus cool, ok for me to do that in a followup?

@RonnyPfannschmidt
Copy link
Member

@asottile yup

@nicoddemus
Copy link
Member

@RonnyPfannschmidt want some more time to take look at this?

Otherwise I will merge this tomorrow. 👍

@nicoddemus nicoddemus merged commit 6161bcf into pytest-dev:features Nov 17, 2017
@nicoddemus
Copy link
Member

Thanks again @asottile!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants