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
Fix shutil.get_terminal_size() to catch AttributeError #70988
Comments
(diff was generated with git, I don't know if it works with Mercurial, but it's a trivial change anyway) |
The patch looks good to me. The function was originally written to be included in the “os” module, hence the NameError. The patch should probably be fine with Mercurial, but it looks like the Reitveld review system doesn’t like it :) What platform do you get the AttributeError with? Perhaps the function is not well covered in the test suite. |
I think Rietveld doesn't like me because I made it a .diff file, and not a .patch file, but who knows. It's a bit of a shot in the dark though, because I can't reproduce an environment where I think that if |
I guess Tests should fail if
I think this is because the patch doesn't contain a revision number. On Windows |
get_terminal_size.diff LGTM. Would you like to try to write an unit test? Maybe using unittest.mock.patch? |
test_shutil always call shutil.get_terminal_size() and I didn't see any failure on the unit suite on our buildbots, even less common platforms like OpenBSD, AIX, OpenIndiana, etc. So yes, os.get_terminal_size() looks to be available on all supported platforms. |
On posix-based OSes, New patch file with tests included. |
Hmm, if Should I write a unit test for that too? Even though this one theorically covers it as well, I wouldn't want people in the future to think they can safely remove |
It would be nice to have an unit test too for this case. You can use something like: with unittest.mock.patch('shutil.sys') as mock_sys:
del mock_sys.__stdout__
print(shutil.get_terminal_size((3, 3))) (and mock also os.envion, as shown in the review.) |
Honest, I don't think that we need such complex test for the case that isn't occurred in wild. If delete os.get_terminal_size, all TermsizeTests tests fail, so we will know if encounter a platform without os.get_terminal_size. Instead I suggest to add ValueError in exceptions list and add tests for changed sys.__stdout__: None, StringIO(), open(TESTFN, "w"). Some of these tests fail without AttributeError in the exceptions list. |
Right. I'm also fine if you test manually this corner case. The Lib/shutil.py change LGTM in get_term_size_with_test2.patch (I ignored the unit test). |
To be fair, I don't think we actually need a unit test to check if |
New changeset e3763b5964b6 by Victor Stinner in branch '3.5': New changeset 75f40345d784 by Victor Stinner in branch 'default': |
Well, since Serhiy, Emmanuel and me agree that unit tests are overkill, I pushed the obivous and trivial fix. Thank you Emmanual for your contribution! I added your name to Misc/ACKS ;-) |
Here is a patch that adds ValueError in the exceptions list and adds tests. |
I doubt it is worth spending much effort supporting sys.__stdout__ being overwritten with StringIO or deleted. That seems an abuse of the “sys” module. Idle doesn’t even seem to alter this attribute. But if you call stdout.close() or detach(), I think that is a more valid way to trigger ValueError, so Serhiy’s patch is worthwhile for that case. |
Left testing only the most common cases: sys.__stdout__ is None or is non a terminal. |
Serhiy’s patch looks worthwhile to me, though I still think a comment would help. There are lots of different cases being handled by those few lines: try: |
Could you suggest concrete wording? |
How about: try: |
Martin's comment is helpful and LGTM. |
New changeset df8652452d25 by Serhiy Storchaka in branch '3.5': New changeset d6e6dcef674f by Serhiy Storchaka in branch 'default': |
Thank you Martin, your comment is helpful. |
New changeset 678424183b38 by INADA Naoki in branch '3.6': New changeset f8815001a390 by INADA Naoki in branch 'default': |
This patch introduced multiple refleaks in test_asyncgen. |
Ah, never mind, the commit message has a wrong issue number:
Closing this one, will re-open bpo-26081. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: