kvas-it / pytest-console-scripts

Pytest plugin for testing console scripts
MIT License
78 stars 14 forks source link

Parity with subprocess.run #63

Closed HexDecimal closed 1 year ago

HexDecimal commented 1 year ago

The readme explains that the console_scripts fixture works similarly to subprocess.run. I'd like to make it even more similar so that it's easier to port tests using tools similar to subprocess.run and also have it be more obvious how the fixture works.

Specifically I'd like to suggest the following changes:

script_runner should keep the defaults of everything being in Unicode. I don't want to complicate it with bytes handling.

def test_subprocess_like_args(script_runner):
    # Examples assuming "inprocess" mode.
    result = script_runner.run(['foobar', '--version'], check=True)  # check=True raises if result.returncode is non-zero
    test_script: Path = ...  # Direct path to script.py
    script_runner.run([test_script], check=True)  # Pass a PathLike as an argument
    script_runner.run(test_script, check=True)  # str or PathLike also works as a single direct argument

    script_runner.run("script --test", shell=True, check=True)  # Could be supported in theory.
kvas-it commented 1 year ago

Yeah, being closer to subprocess.run API would have been a better approach. However, at this point I would prefer to keep backwards compatibility. I have a couple of ideas in mind about how we could do it:

  1. Add another method to ScriptRunner that has the API you propose. Re-implement run() with the existing API on top of the new method and document both in README.
  2. Add another fixture with the API you propose, make it the main one and have script_runner fixture call the new one. This is a bit more work, but we can have the method name run(), which keeps it closer to subprocess.run().

In both cases we could deprecate the existing API and suggest in the doc that the new one should be used for new development. I wouldn't plan removing it at this point since the compatibility wrapper would be lightweight and the maintenance overhead for it small.

How does this sound?

Cheers, Vasily

HexDecimal commented 1 year ago

I've considered this, I think the transition can be handled with a new top-level function:

def _handle_command_args(
    cmd: str | os.PathLike[str] | Sequence[str | os.PathLike[str]],
    *args: str | os.PathLike[str],
) -> str | os.PathLike[str] | Sequence[str | os.PathLike[str]]:
    "Return command arguments in a consistent format."
    if args:
        cmd = [cmd, *args]
        ...  # Warn
    return cmd

Backwards compatibility and warnings are handled by this function which will likely be called from the run_inprocess and run_subprocess methods.

run_inprocess might need more complex logic to get the script argument, but that shouldn't be too difficult.

I'll try to make a PR.

kvas-it commented 1 year ago

So you want to support both ways of calling script_runner.run() with the same function. This seems more difficult, but yeah, should be possible.

Alright, looking forward to the PR!