Click: Result object should have .stdout and .stderr in addition to .output

[omribahumi]

I'm writing a click application that sends execution information to stderr. The interesting output is on stdout, which I want to make assertions on.

Right now I have to use a regular expression to extract stdout from the Result.output property.
Is it possible to save them separately in addition to the aggregated value?

[untitaker]

Might be possible (we'd have to write to multiple StringIOs?), but I've yet to see a case where testing whether something goes to stderr instead of stdout caught any actual errors.

[omribahumi]

It's not what I wanna test. The stderr output is irrelevant for the testing. Think about verbose logging where the log output goes to stderr. You don't want that in your output assertions.

[untitaker]

Ahh, but stderr might contain serious (non-fatal) error messages as well. In that case, perhaps you actually want to stay with regex-filtering, or provide and use a switch to turn the logging messages off.

[omribahumi]

Fair enough. I'll add a --quiet flag. Though I still think this might be a nice feature.

[bryder]

I do stderr vs stdout error testing all the time. My tools write python
exceptions to stderr - so I usually have

assert stderr == ""

Sometimes my stdout is supposed to be empty (ie a selection didn't match
anything) - and not checking stderr means I'll miss an exception but the
test will pass because stdout is empty.

So without that separation I can not properly test arguments which are
supposed to produce no result.

I run my cli tests in a wrapper which use pexpect because my tools behave
differently if stdin is /dev/tty or a file/pipe (I warn the user that input
is expected on stdin if they start the tool without naming files - and
stdin is /dev/tty - otherwise people run things and wonder why nothing is
happening). So I have to test the case when stdin is a file or /dev/tty.

This is why for me the prompt functions in click are not useful because I
read data off stdin all the time - at which point prompt is useless. Maybe
I'lll make a pull request but no one seems to need a proper separation of
/dev/tty and stdin and it'll take a bit of work.

On Fri, Jul 3, 2015 at 10:50 PM, Omri Bahumi [email protected]
wrote:

Fair enough. I'll add a --quiet flag. Though I still think this might be
a nice feature.

—
Reply to this email directly or view it on GitHub
https://github.com/mitsuhiko/click/issues/371#issuecomment-118314312.

[geowurster]

@untitaker Another argument for .stdout and .stderr is for ensuring serialized output like JSON is de-serializable. Warnings can be triggered inside an external dependency and may not be filtered with a regex but end up in .output. We can of course add warnings.simplefilter('ignore') at the beginning of tests affected by this scenario, but they can be difficult and time consuming to identify as the correct solution.

The example command below prints some JSON to stdout and optionally issues a warning. The included test can be run with pytest and tells the command to emit a warning, which creates corrupt output. Uncommenting # warnings.simplefilter('ignore') causes the test to pass, as does switching --warn to --no-warn.

import json
import warnings

import click
from click.testing import CliRunner


@click.command()
@click.option('--warn / --no-warn', default=True)
def cmd(warn):

    """
    Dump some JSON (and an optional warning to stdout.
    """

    if warn:
        warnings.warn("""

        Poorly structured warning

        """)
    click.echo(json.dumps({'k': 'v'}))


def test_cmd():

    # warnings.simplefilter('ignore')
    result = CliRunner().invoke(cmd, [
        '--warn'])
    assert result.exit_code == 0, result.output
    assert json.loads(result.output) == {'k': 'v'}


if __name__ == '__main__':
    cmd()

[untitaker]

To clarify my stance, I am not disputing that this feature doesn't have its uses (and am not opposed to the feature)

[adamtheturtle]

I have a rough patch which includes the following passing test:

def test_result():
    @click.command()
    def cli_output():
        click.echo(1)
        click.echo(2, err=True)

    runner = CliRunner()
    result = runner.invoke(cli_output)
    assert result.output == '1\n2\n'

    runner = CliRunner()
    result = runner.invoke(cli_output, combined_output=False)
    stdout, stderr = result.output
    assert stdout == '1\n'
    assert stderr == '2\n'

Would such an API be welcome?
I'm not sure how to get all of .stdout, .stderr and .output.

[hangtwenty]

@adamtheturtle Personally I am voting for this feature, I treat stderr and stdout as separate in testing sometimes... But re: your API in last comment, I'd like it better if .output wasn't overload, instead that was always a combined stream... Then when you do combined_output=False, it will populate result.stderr and result.stdout, and if you ask for result.output it throws ValueError. (conversely in default mode it would throw a ValueError for result.stderr or result.stdout.) So, same gist as yours, but no overloading, and harder/obvious failures when the wrong thing is done.

[adamtheturtle]

@hangtwenty That is a better idea.

Internally, this uses the isolation context manager.

This gives output as follows:

with self.isolation(input=input, env=env, color=color) as output:

In my rough proof of concept, I had that take a combined_output parameter and yield either the combined output or a (stdout, stderr) tuple.

This has a similarly confusing API problem. I'm not sure what to suggest that would be backwards compatible and also friendly. Any ideas @hangtwenty ?

[olgabot]

Glad I found this thread! Separation of stderr and stdout is important to me as well because I'm working on a CLI that writes a CSV to stdout and gives the user a message about the plot that was created using the input data, e.g.:

$ dobby parse_fluorescence test/data/MAA000154.txt --filetype txt --figure-folder test_output > test/data/MAA000154.csv
MAA000154: Wrote fluorescence heatmap to test_output/fluorescence/MAA000154_fluorescence_heatmap.pdf

@hangtwenty's idea is very nice! I like being able to test stderr vs stdout separately. Weirdly, combining click and pytest's capsys.readouterr() doesn't work, because the out and err tuples are empty. It must be something with the way click is running internally so it's scraping out stderr and stdout before pytest has a chance to read them.

(dobby) ➜  dobby git:(modularize) ✗ /Users/olgabot/anaconda3/envs/dobby/bin/pytest --pdb
============================================================================== test session starts ===============================================================================
platform darwin -- Python 3.6.2, pytest-3.2.1, py-1.4.34, pluggy-0.4.0
rootdir: /Users/olgabot/code/dobby, inifile:
collected 2 items                                                                                                                                                                 

test/test_io.py .F
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

filename = '/Users/olgabot/code/dobby/test/data/MAA000154.txt', capsys = <_pytest.capture.CaptureFixture object at 0x117a25550>

    def test_parse_fluorescence_heatmap(filename, capsys):
        from dobby.io import parse_fluorescence, _parse_fluorescence

        heatmap_folder = os.path.join('test_output', 'fluorescence')

        runner = CliRunner()
        result = runner.invoke(parse_fluorescence,
                               [filename, '--figure-folder', heatmap_folder])
        out, err = capsys.readouterr()
        assert result.exit_code == 0

        csv = filename.replace('.txt', '.csv')
        true = _parse_fluorescence(csv, filetype='csv')

        # This test fails because result.output includes stderr but the csv is
        # written to stdout
>       assert result.output == true.to_csv()
E       AssertionError: assert 'MAA000154: W...70187,21950\n' == ',1,2,3,4,5,6,...70187,21950\n'
E         - MAA000154: Wrote fluorescence heatmap to test_output/fluorescence/fluorescence/MAA000154_fluorescence_heatmap.pdf
E           ,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24
E           A,1607623,870255,1473596,822329,2606790,915395,743408,839900,1680346,2601054,1329745,426090,786117,1260567,1559793,668853,742214,773718,758931,659945,1482332,687784,698864,6942884
E           B,1526862,1549455,736932,1381884,724205,739192,2506348,2984756,761838,788951,1406856,778413,778326,676062,1185720,727802,1655809,726753,732393,1628888,602365,1679494,680780,6505976
E           C,802298,717058,2193970,133981...
E         
E         ...Full output truncated (14 lines hidden), use '-vv' to show

test/test_io.py:36: AssertionError
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
*** SyntaxError: invalid syntax
> /Users/olgabot/code/dobby/test/test_io.py(36)test_parse_fluorescence_heatmap()
-> assert result.output == true.to_csv()
(Pdb) out
''
(Pdb) err
''

[hangtwenty]

@olgabot I think you are right about why those are empty - Click is capturing first before pytest

Thinking out loud - For people who don't use py.test, it's great and helpful that Click has its own way. But for myself, you, and @adamtheturtle , maybe our use-case is best met by just relying on py.test capture, and turning off Click capture. But, this would still require a PR -

So if we need a change anyway, we should do the way that seems best. Looking at the source (i.e. usage of isolation https://github.com/pallets/click/blob/master/click/testing.py#L245 , as well as its definition), the options seem to come down to ...

• make self.isolation able to do two 'modes', combined and not ;
  
  
  
  • either like @adamtheturtle was saying - return string vs 2-tuple depending on the mode,
  
  
  • or make it return an instance of some container class, which has .output (unified stderr/stdout) or .stderr/.stdout. so still 1 return thing in both cases, then invoke would handle appropriately
  
  
• make self.isolation able to be just turned off with a flag

2nd is simpler, but maybe dirtier... Thoughts?

[blueyed]

1. I think output should stay the same, but could have .stdout and .stderr properties added.
2. @olgabot with pytest you have to use capfd instead of capsys with click's test runner.

See https://github.com/pallets/click/pull/951 for some WIP.
See also https://github.com/pallets/click/issues/654, which might be simpler, but needs to be extended for stderr then.

[dsully]

Just merged PR #868 - let's follow on with any changes from that. Thanks.