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

mocked stdio are not like the real thing #24

Open
jayvdb opened this issue Jul 27, 2019 · 14 comments
Open

mocked stdio are not like the real thing #24

jayvdb opened this issue Jul 27, 2019 · 14 comments
Labels

Comments

@jayvdb
Copy link

jayvdb commented Jul 27, 2019

All of the sys.std* are of type io.TextIOWrapper which has a .buffer of type io.BufferedReader.

Without the mock implementing TextIOWrapper, it wont work with any the code being tested that needs to access those buffers , or any of the other features of the real stdii handles which are not implemented by StringIO, and also StringIO exposes methods which are not valid on the real stdio handles.

Another important feature of the real stdio handles is the new reconfigure in py37+, which is useful to force a sensible encoding on a stdio stream.

@kvas-it
Copy link
Owner

kvas-it commented Jul 29, 2019

Thanks for the ticket, John. You're right, the std* mocks are not very realistic. The whole extension was rolled together quickly and at that point I decided to just implement the features I needed at the time and worry about the rest later.

It would be great to make the std* mocks more comprehensive (removing the features that are not present on the real handles is also useful but less critical because simply running the tests in subprocess mode would make the invalid usage break). If you would like to submit a pull request with improvements for the mocks, I would welcome that. Otherwise I'd like to see the examples of things that are not working for you -- I probably won't have time right now to make perfect mocks so I'm considering moving step by step first covering the functionality that's actually important for users.

@jayvdb
Copy link
Author

jayvdb commented Jul 29, 2019

I've been building real-ish classes over at https://github.com/bskinn/stdio-mgr/pulls , especially bskinn/stdio-mgr#25 and bskinn/stdio-mgr#26 . Examples of scenarios where the simple mocks dont work are shown at bskinn/stdio-mgr#24

@kvas-it
Copy link
Owner

kvas-it commented Jul 29, 2019

Interesting! stdio-mgr looks like something that it might make sense to use in pytest-console-scripts. It would be an additional dependency, but assuming you implement more realistic mocking there it might make more sense to depend on an existing implementation that works instead of maintaining essentially the same logic in two places.

Another approach could be to use redirect_stdout and redirect_stderr from contextlib (I didn't know about them -- thanks for helping me find them) and then only implement a stdin wrapper. Do you know why this is not done in stdio-mgr btw?

Both of these approaches only work for Python 3, which would be a problem for now, but not in the long term. I guess I could try to implement the whole thing here in a 2/3-compliant way while peeking into your pull requests for stdio-mgr.

What are your thoughts on those options?

@jayvdb
Copy link
Author

jayvdb commented Jul 29, 2019

There are backports of https://pypi.org/search/?q=contextlib , if you went down that path.

@bskinn might be better to answer why he created stdio-mgr instead of using contextlib. My guess is that the stdin part, and linking it with stdout, is where the value is.

Also I think stdio-mgr could be Python 2 compatible without much effort. I would be happy to attempt it, if @bskinn would accept it.

@kvas-it
Copy link
Owner

kvas-it commented Jul 29, 2019

Aha, I just realized that contextlib decorators actually don't help me much because they don't create the mock streams, only replace them in sys -- I should have paid more attention ;) This option is not a real option then.

This makes studio-mgr a more attractive option, especially if it would become Python 2 compatible. Could you ping me when the two MRs mentioned above are merged and I will try to make my mind meanwhile?

@bskinn
Copy link

bskinn commented Jul 29, 2019

The main motivation was definitely the capture of stdin, to enable me to write fully in-context CLI tests that require simulation of user input; e.g., here. I looked around, but didn't find anything that seemed to do what I wanted, so I made it (imperfectly) myself. I've appreciated @jayvdb's help in converting it to a more accurate match to the actual stdio objects in sys.

I'm pretty sure I could have used the stdout/stderr redirect tools from either contextlib or pytest for what I needed, but implementing them both didn't take much extra effort and doing so allowed managing all three stdio objects with a single context, rather than three nested ones, in the vein of:

with mock_stdin() as in_:
    with capture_stdout() as out_:
        with capture_stderr() as err_:
            ...

... and then only implement a stdin wrapper. Do you know why this is not done in stdio-mgr btw?

bskinn/stdio-mgr#4 is badly written (is really a note-to-self), its intent is to enable something like this, where the user could select to mock only one or two of the streams, instead of being forced to mock all three.

Also I think stdio-mgr could be Python 2 compatible without much effort. I would be happy to attempt it, if @bskinn would accept it.

Agreed, it would probably not be hard, though I have ~zero experience with 2/3 compat. @jayvdb, if you're willing to continue to pitch in with maintenance (I really need to get CONTRIBUTORS and AUTHORS files put together...), I'm okay with trying to add Python 2 support. I'd ask, though -- how far beyond 1/1/2020 are you planning on keeping Py2 support, since pytest itself has already dropped support in v5? At this point, it seems like the returns would diminish rapidly.

@kvas-it
Copy link
Owner

kvas-it commented Jul 29, 2019

Hi @bskinn, thank you for the background!

I'm okay with trying to add Python 2 support. I'd ask, though -- how far beyond 1/1/2020 are you planning on keeping Py2 support, since pytest itself has already dropped support in v5?

Yeah, very good question. It can't really be very far. I have some code of my own that has to still support Python 2 because of reasons but that's going to go away before 2020 and I would not want to keep Python 2 compatibility around just for that anyway, I'd rather pin pytest-console-scripts at an old version instead. Of course there are other users, some of which might be still dependent on Python 2 but perhaps I could recommend the same approach to them.

Anyway, I don't feel very comfortable asking anyone to work on Python 2 compatibility in their code at this point. Unless it's really easy to make stdio-mgr Python 2/3, it might make more sense to make a major release of pytest-console-scripts with better std* stream mocking that would be Python 3 only.

@bskinn
Copy link

bskinn commented Jul 29, 2019

I poked at 2.7 a bit ... it needs from __future__ import absolute_imports in __init__.py, even to import, and then at that point all of those super() calls make a bunch of noise. It's definitely not Python 2-ready, out-of-the-box.

I suspect there's going to be a lot of pin-and-pray going on with 2.7-locked codebases. Hopefully it doesn't lead to some major security apocalypse down the line. 😞

@jayvdb
Copy link
Author

jayvdb commented Jul 29, 2019

I maintain a few large py2/3 codebases, and pin-and-pray is going to be a notable concept very soon.

I would aim for one good release with py2, then drop it. Users can then pin it.
If there are significant users of py2 beyond 2020, that pinned version can become a branch if necessary.

The benefit is a lot of projects can replace their py2 stdio code with stdio-mgr, without killing py2 explicitly. That concentrates any py2 users on stdio-mgr, distributing the effort required to keep it alive. Or it will die quietly of old age.

@jayvdb
Copy link
Author

jayvdb commented Aug 20, 2019

Hiya @kvas-it , those PRs were merged, and bskinn/stdio-mgr#53 will add extra opportunity to customise the behaviour by allowing subclasses. This would be a good point to get some requirements from you about what features you would want, so we could prioritise them for the next release. There is quite a long list of issues at https://github.com/bskinn/stdio-mgr/issues , mostly feature ideas.

@kvas-it
Copy link
Owner

kvas-it commented Aug 21, 2019

Thank you @jayvdb! I don't have much to add in terms of requirements apart from the fact that for now pytest-console-scripts supports Python 2 (and people might be using it in projects that target Python 2 -- I know I do, although I'm planning to drop Python 2 in those cases). Other than that, stdio-mgr would be pretty easy to integrate as is and it would already make the streams more realistic.

So I guess I just need to make up my mind about Python 2. So the best looking option seems to be a breaking major release that would be Python 3 only and advising people to pin it at the old version for Python 2 support.

@bskinn
Copy link

bskinn commented Aug 21, 2019

To note, per discussion at bskinn/stdio-mgr#34, we're still contemplating an attempt at a 2.7 backport at some point.

@kvas-it
Copy link
Owner

kvas-it commented Aug 22, 2019

Yeah, thank you for that. Don't consider me strongly in favor of it though. I mean, it would make my life easier here but I'm not sure it's a good use of your time, guys ;)

@HexDecimal
Copy link
Collaborator

The project uses contextlib to replace the out/err streams, but that doesn't change much, since it's still StringIO for the stream that the running script sees.

It still doesn't have reconfigure unless it gets switched to TextIOWrapper, and doing that means using BytesIO as the buffer. Everything is currently setup for Unicode only so this very difficult to deal with at the moment. I'm not ready to type-hint a bytes interface, but it could be done with generics and overloads.

On the bright side it's been so long since this PR was opened that everything that's needed from the standard library is now available.

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

No branches or pull requests

4 participants