Add hook to update io.input.stream to sys.stdin if it is None.

[chadac]

Pull Request Check List

Resolves: #5633

• Added tests for changed code.
• Updated documentation for changed code.

[abn]

Is this something we can add a meaningful test for?

[chadac]

@abn

I could create a new test_command.py with these tests or add them to an existing file:

def test_command_with_no_io_stream(mocker: MockerFixture):
    command = Command()
    mocker.patch.object(
        BaseCommand,
        "run",
        return_value=[1]
    )
    io = IO(input=Input(), output=None, error_output=None)
    io.input.set_stream(None)
    command.run(io)
    assert io.input.stream == sys.stdin


def test_command_with_existing_io_stream(mocker: MockerFixture):
    command = Command()
    mocker.patch.object(
        BaseCommand,
        "run",
        return_value=[1]
    )
    io = IO(input=Input(), output=None, error_output=None)
    io.input.set_stream(sys.stdout)
    command.run(io)
    assert io.input.stream == sys.stdout

What would y'all recommend?

[abn]

I was wondering if we could create a more functional test. Similar to one of the other interactive command test cases.

[chadac]

Sure, lemme take a crack at that. There currently aren't any tests for cache clear at the moment, I can see if I can get it fully covered in the integration tests.

[chadac]

I've gone ahead and added some tests for the cache clear command, but incidentally it actually doesn't capture the issue this PR fixes. The main issue is that the CommandTester in cleo don't actually trigger the problematic code in cleo.application.Application:L425. It's possible that this could be caught by migrating to using ApplicationTester instead, although I don't know what y'all think about that. It would be a divergence from what most of the other commands do.

[neersighted]

I've gone ahead and added some tests for the cache clear command, but incidentally it actually doesn't capture the issue this PR fixes. The main issue is that the CommandTester in cleo don't actually trigger the problematic code in cleo.application.Application:L425. It's possible that this could be caught by migrating to using ApplicationTester instead, although I don't know what y'all think about that. It would be a divergence from what most of the other commands do.

I would rather have a good regression test for this, even if it's a bit divergent from the other tests.

[chadac]

Sounds good, I've gone ahead and swapped it over. I I did have to add one attribute to the Application class in order to ensure that the "original" input object is sufficiently preserved, so that the command code can property specify either sys.stdin or the proper test StringIO stream. I'm open to other ideas, though. It's technically less brittle now as the hardcoded sys.stdin is gone, but technically it should be unneeded when cleo merges in this PR and has the same tests.

[branchvincent]

@chadac we plan to have a new cleo release soon with your merged fix. Could we update this pr to temporarily depend on cleo's master branch to verify your tests also pass here? Then we can update again once it's released

[neersighted]

The Cleo release is now in master -- this can just be converted to a regression test only.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.