Code Monkey home page Code Monkey logo

Comments (6)

ericfrederich avatar ericfrederich commented on June 3, 2024

Fyi... this was the code I used to discover the bug. On the pull request I ended up just modifying an existing test.

import click
import pytest
from click.testing import CliRunner


@click.command()
@click.option("--out/--no-out")
@click.option("--err/--no-err")
def dummy_cmd(out: bool, err: bool):
    import sys

    print("this is stdout", file=sys.stdout)
    print("this is stderr", file=sys.stderr)
    if out:
        print("Extra out!", file=sys.stdout)
    if err:
        print("Extra err!", file=sys.stderr)


@pytest.mark.parametrize("out_option", [None, True, False])
@pytest.mark.parametrize("err_option", [None, True, False])
def test_dummy_cli(out_option: str | None, err_option: str | None):
    runner = CliRunner(mix_stderr=False)
    args = []
    expected_out_lines = 1
    expected_err_lines = 1
    if out_option is not None:
        if out_option:
            args.append("--out")
            expected_out_lines += 1
        else:
            args.append("--no-out")
    if err_option is not None:
        if err_option:
            args.append("--err")
            expected_err_lines += 1
        else:
            args.append("--no-err")
    result = runner.invoke(dummy_cmd, [*args])

    if result.exception:
        raise result.exception

    if result.exit_code != 0:
        raise AssertionError(f"Non-zero return code: {result.exit_code!r}\noutput:\n{result.output}")

    assert len(result.stdout.splitlines()) == expected_out_lines
    assert len(result.stderr.splitlines()) == expected_err_lines

from click.

davidism avatar davidism commented on June 3, 2024

Duplicate of #2636. If you're using things that don't flush, like print, then they don't flush. Hiding that by doing it in the test runner just hides the potentially incorrect output behavior you're using. This is one reason why click.echo exists.

from click.

ericfrederich avatar ericfrederich commented on June 3, 2024

This argument doesn't pass the smell test.
If that's really the case why is there a sys.stdout.flush() call here prior to capturing stdout? Interestingly 2 different test fail when I comment out the stdout flush.

All I'm asking is that the code follow the same style when capturing stderr as it does when capturing stdout.
That's all it takes to fix the bug. Just call sys.stderr.flush() prior to stderr = outstreams[1].getvalue() the same way sys.stdout.flush() is called prior to stdout = outstreams[0].getvalue(). Let's be consistent.

Another thought... it may not be your code which is writing to stderr or stdout, but some dependency which you have no control over. Alternatively it may be a dependency you explicitly choose to use which likes to do its own stdout/stderr writing (something like rich)

Click has a lot of things... cli, prompts, progress bars, styled printing, etc. Please don't make this an all-or-nothing library. It's nice to pick and choose. I shouldn't be forced to use click.echo just because I want to use the CLI and testing's CliRunner.

from click.

davidism avatar davidism commented on June 3, 2024

You're welcome to use print to stderr, you just have to remember to flush in that case. Stdout is flushed, stderr isn't. That's true regardless of if you're using click.

If we flush for you during tests, that's hiding the bug, not fixing it.

from click.

davidism avatar davidism commented on June 3, 2024

Another way to put it is: you need to explain why your suggested behavior is correct. It's certainly different than current, but that doesn't mean it's more correct.

What I know is that stdout is flushed, and stderr isn't. I don't know why that's the case, but I do know that this is the behavior I've encountered all over the place, and if I want stderr to show up at a specific time I need to flush manually. This is what click.echo does automatically. If what I know and click's test runner's current behavior is incorrect, then you need to show me sources that explain why. Otherwise, it would just mean we're hiding the actual behavior of the calls you're making.

from click.

ericfrederich avatar ericfrederich commented on June 3, 2024

Thanks, I think understand what you're saying. Are you saying the CliRunner flushed stdout to mimic the behavior you've encountered all over the place?

If click.echo forces a flush every time though, then why does the CliRunner need to flush as well? Why do the tests fail when that flush in the CliRunner is removed?

from click.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.