Piped input now properly honors the echo setting #220

kotfu · 2017-08-24T01:16:20Z

When input is piped into cmd2 from something that isn't a tty, we now honor the *echo setting to determine whether we should print the prompt and the command to the output stream.

Also fixed a bug where we tried to execute stty on the input stream, even when it wasn't a tty.

This closes #218

if we are on a pipe, we have to echo the prompt only after we read and are not at EOF.

When ‘pytest -n8’ parallelizes the test execution, hacking sys.stdin to some other file descriptor yields unpredictable results

codecov · 2017-08-24T01:16:24Z

Codecov Report

Merging #220 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #220      +/-   ##
========================================
+ Coverage   96.81%    97%   +0.19%     
========================================
  Files           1      1              
  Lines        1194   1204      +10     
========================================
+ Hits         1156   1168      +12     
+ Misses         38     36       -2

Impacted Files	Coverage Δ
cmd2.py	`97% <100%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba1319f...9ab2614. Read the comment docs.

kotfu · 2017-08-24T01:32:12Z

code coverage went down, I think it's because I don't currently test the stdin.isatty() logic branches. Lemme see if I can find a way to mock that and force the execution path over those lines.

kotfu · 2017-08-24T01:59:03Z

I don't know how to write a test to cover all the code. There are two parts of cmd2.pseudo_raw_input() that I don't know how to cover. The first part executes if use_raw_input is set:

if sys.stdin.isatty():
    sys.stdout.write(safe_prompt)
    line = sm.input()
else:

I can mock up .isatty() to return true, and I can mock up sm.input() to return the expected value. However, a side effect of sm.input() is that the typed characters are output to sys.stdout. I dunno how to make that happen. The mock(side_effect=func) parameter lets you dynamically change the return value, but I need it to do other stuff too.

It's the same problem with this chunk, also in cmd2.pseudo_raw_input():

if self.stdin.isatty():
    # on a tty, print the prompt first, then read the line
    self.poutput(safe_prompt, end='')
    self.stdout.flush()
    line = self.stdin.readline()
    if len(line) == 0:
        line = 'eof'
else:

except in this chunk it's stdin.readline() that needs to echo the input characters to self.stdout.

Help!

tleonhardt

It looks good to me. If you can get the test coverage back up with a little work great, if not don't kill yourself. Some cases are hard to test.

tleonhardt · 2017-08-24T01:58:48Z

cmd2.py

            # Fix those annoying problems that occur with terminal programs like "less" when you pipe to them
-            proc = subprocess.Popen(shlex.split('stty sane'))
-            proc.communicate()
+            if self.stdin.isatty():


Oh yeah, good call! Trying to "sane" a stdin which isn't a terminal isn't going to work so well ;-) That should explain those stty errors.

tleonhardt · 2017-08-24T02:02:36Z

cmd2.py

            try:
-                line = sm.input(safe_prompt)
+                if sys.stdin.isatty():
+                    sys.stdout.write(safe_prompt)


Just curious ... why not just pass safe_prompt directly to sm.input(safe_prompt) like before? Why call sys.stdout.write(safe_prompt) first and then sm.input() with no prompt?

Actually I did some manual testing, it needs to be the other way, very bad things can happen like the user can delete the prompt by backspacing far enough if you do it this way.

My (apparently lame) thought was that I can patch or mock the sm.input() but since I don't know how to get the side effect of echoing the characters to stdout, I'd just print them out there first. But if it breaks real-life, who cares whether the tests pass or are easy?

tleonhardt · 2017-08-24T02:10:47Z

tests/test_cmd2.py

+# the next helper function and two tests check for piped
+# input when use_rawinput is True.
+#
+# the only way to make this testable is to mock the builtin input()


Yeah, this is a pain to test, mocking it is the only way I know of.

tleonhardt · 2017-08-24T02:14:01Z

cmd2.py

-                line = sm.input(safe_prompt)
+                if sys.stdin.isatty():
+                    line = sm.input(safe_prompt)
+                else:


This else seems to nicely handle the case where stdin is a pipe, both when echo is either True or False

tleonhardt · 2017-08-24T02:15:30Z

cmd2.py

+                        sys.stdout.write('{}{}\n'.format(safe_prompt,line))
            except EOFError:
                line = 'eof'
        else:


This else case is there honestly just because it is there in cmd and because cmd defines the raw_input attribute. It is never used explicitly in cmd2 other than some unit testing "just in case".

tleonhardt · 2017-08-24T02:16:24Z

cmd2.py

            try:
-                line = sm.input(safe_prompt)
+                if sys.stdin.isatty():
+                    line = sm.input(safe_prompt)


I committed a small change to restore this line to the way it was due to issues found during manual testing.

tleonhardt · 2017-08-24T02:27:29Z

You can view visual code coverage analysis in the code coverage tool with covered lines highlighted in green and uncovered lines highlighted in red. But finding the right link to click on can be non-intuitive. For this PR it is here:
https://codecov.io/gh/python-cmd2/cmd2/pull/220/tree

As you suggested, it is the two isatty branches that start with:

if sys.stdin.isatty():

and

if self.stdin.isatty():

that are uncovered.

I don't think stdin is going to be a tty inside of a unit test. So these are a pain to test.

tleonhardt · 2017-08-24T02:36:33Z

Here is one possible unit test approach for the uncovered cases ...

Say we have the block which looks like:

                if sys.stdin.isatty():
                    line = sm.input(safe_prompt)
                else:
                    line = sm.input()
                    if self.echo:
                        sys.stdout.write('{}{}\n'.format(safe_prompt,line))

Sometimes for difficult cases like this I may just shoot for "branch verification" instead of output or side-effect verification.

For example, maybe there could be three unit tests for these cases:

stdin is a tty
- verify that input() is called with an argument
stdin is not a tty and echo is True
- verify that input is called without an argument and that stdout.write is is not called
stdin is not a tty and echo is False
- verify that input is called without an argument and that stdout.write is called with an argument

For all of these cases, the following functions could be mocked so that they don't do anything but we can see how many times they are called and with what argument(s):

sys.stdin.isatty
sm.input
sys.stdout.write

This is a weaker form of unit testing than actually verifying output. But in cases where manipulations are being done to stdin and stdout it is sometimes the best that can be reasonably done.

tleonhardt · 2017-08-24T02:51:35Z

The unit tests I created previously for testing the pseudo_raw_input() function are these:

test_base_cmdloop_without_queue
test_cmdloop_without_rawinput

Though these tests weren't very focused and tested a number of different things at the same time. It looks like your new unit tests you added do a better job of focusing in on testing the pseudo_raw_input() function.

We just might need a couple similar ones which mock isatty() to return true. If we only verify branches taken instead of actual output in that case, do you think it would be a reasonable approach?

kotfu · 2017-08-24T02:53:32Z

Yup. I'm working on it right now. I know I can mock isatty() to be true, and then I can at least check that input() got called with the correct prompt. Stand by.

kotfu · 2017-08-24T03:48:50Z

Uh-oh. This code:

cmd2/tests/test_cmd2.py

Lines 1439 to 1457 in c92ac8c

    
           def piped_rawinput_true(capsys, echo, command): 
        
               m = mock.Mock(name='input', side_effect=[command, 'quit']) 
        
               sm.input = m 
        
               # run the cmdloop, which should pull input from our mocked input 
        
               app = cmd2.Cmd() 
        
               app.use_rawinput = True 
        
               app.echo = echo 
        
               app.abbrev = False 
        
               app._cmdloop() 
        
               out, err = capsys.readouterr() 
        
               return (app, out) 
        
           def test_pseudo_raw_input_piped_rawinput_true_echo_true(capsys): 
        
               command = 'set' 
        
               app, out = piped_rawinput_true(capsys, True, command) 
        
               out = out.splitlines() 
        
               assert out[0] == '{}{}'.format(app.prompt, command) 
        
               assert out[1] == 'abbrev: False'

is not deterministic when pytest runs with xdist. Sometimes is passes, sometimes it fails. I expect because we are mocking sm.input() which could be simultaneously used by other tests that are running.

Mocks of six.moves.input() and sys.stdin.isatty() now use either a context manager or a decorator. These wrappers make sure to put the functions back to their unmocked values when the test is done. This change appears to have solved the undeterministic test results.

tleonhardt

Thank you for another excellent PR. I learned a decent amount about using mocks in pytest from this one. I wasn't aware you could use them as either context managers or decorators and have the original version of the function automatically restored.

tleonhardt · 2017-08-24T11:41:55Z

tests/test_cmd2.py

+def test_pseudo_raw_input_tty_rawinput_true():
+    # use context managers so original functions get put back when we are done
+    # we dont use decorators because we need m_input for the assertion
+    with mock.patch('sys.stdin.isatty',


I wasn't aware you could use mocks as context managers. This is a convenient feature.

Until yesterday, I wasn't either. It's super convenient.

tleonhardt · 2017-08-24T11:42:29Z

tests/test_cmd2.py

+
+# using the decorator puts the original function at six.moves.input
+# back when this method returns
+@mock.patch('six.moves.input',


I wasn't aware you could use mocks as decorators either. This is another great feature.

tleonhardt · 2017-08-24T11:45:56Z

@kotfu I always delete the branch after merging into master to keep things clean. If you ever want me to not delete it, just say something in the PR.

kotfu added 6 commits August 23, 2017 14:33

Don’t try and sane the tty if stdin isn’t a tty

64e1933

Prompt and line now properly echoed on tty and pipe

dde0b69

if we are on a pipe, we have to echo the prompt only after we read and are not at EOF.

Discovered hacking sys.stdin doesn’t always work

ae42c7c

When ‘pytest -n8’ parallelizes the test execution, hacking sys.stdin to some other file descriptor yields unpredictable results

Figured out how to test piped input without hacking stdin

e3930e1

Exit on <control>D when on a tty

41ea474

io.StringIO requires unicode conversion on python2.7

b70c15c

kotfu requested a review from tleonhardt as a code owner August 24, 2017 01:16

kotfu mentioned this pull request Aug 24, 2017

tomcat-manager has unexpected behaviors when input is piped in via stdin tomcatmanager/tomcatmanager#31

Closed

Minor bug fix

b1924fb

tleonhardt previously approved these changes Aug 24, 2017

View reviewed changes

tleonhardt changed the title ~~Piped input not properly honors the echo setting~~ Piped input now properly honors the echo setting Aug 24, 2017

kotfu added 2 commits August 23, 2017 21:28

add tests for pseudo_raw_input when isatty() is true

1a2ee7d

Rename tests that exercise pseudo_raw_input()

c92ac8c

kotfu dismissed tleonhardt’s stale review via c92ac8c August 24, 2017 03:30

kotfu added 3 commits August 23, 2017 22:10

Removed unused capsys fixtures

503ea98

Pick up a few more lines of test coverage

9ab2614

tleonhardt approved these changes Aug 24, 2017

View reviewed changes

tleonhardt merged commit 93a7eb7 into master Aug 24, 2017

tleonhardt deleted the piped_input_improvements branch August 24, 2017 11:44

Piped input now properly honors the echo setting #220

Piped input now properly honors the echo setting #220

Uh oh!

Conversation

kotfu commented Aug 24, 2017 • edited by tleonhardt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kotfu commented Aug 24, 2017

Uh oh!

kotfu commented Aug 24, 2017

Uh oh!

tleonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tleonhardt commented Aug 24, 2017

Uh oh!

tleonhardt commented Aug 24, 2017

Uh oh!

tleonhardt commented Aug 24, 2017

Uh oh!

kotfu commented Aug 24, 2017

Uh oh!

kotfu commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tleonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tleonhardt commented Aug 24, 2017

Uh oh!

Uh oh!

kotfu commented Aug 24, 2017 •

edited by tleonhardt

Loading

codecov bot commented Aug 24, 2017 •

edited

Loading

kotfu commented Aug 24, 2017 •

edited

Loading