From a3c90dd0bcb4aecfc64a4a584e52aec77ee61158 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Tue, 26 Jul 2022 11:43:48 -0700 Subject: [PATCH] ansible-test - Fix TTY and output handling. (#78350) --- .../ansible-test-tty-output-handling.yml | 7 +++ .../targets/ansible-test-sanity-lint/aliases | 4 ++ .../ansible-test-sanity-lint/expected.txt | 1 + .../targets/ansible-test-sanity-lint/runme.sh | 47 ++++++++++++++++++ .../targets/ansible-test-shell/aliases | 4 ++ .../ansible_collections/ns/col/.keep | 0 .../ansible-test-shell/expected-stderr.txt | 1 + .../ansible-test-shell/expected-stdout.txt | 1 + .../targets/ansible-test-shell/runme.sh | 30 ++++++++++++ .../coverage/analyze/targets/__init__.py | 4 -- .../_internal/commands/shell/__init__.py | 6 ++- test/lib/ansible_test/_internal/config.py | 2 +- .../lib/ansible_test/_internal/connections.py | 15 +++--- test/lib/ansible_test/_internal/delegation.py | 8 ++- .../lib/ansible_test/_internal/docker_util.py | 9 ++-- test/lib/ansible_test/_internal/util.py | 49 +++++++++++++------ .../lib/ansible_test/_internal/util_common.py | 5 +- 17 files changed, 159 insertions(+), 34 deletions(-) create mode 100644 changelogs/fragments/ansible-test-tty-output-handling.yml create mode 100644 test/integration/targets/ansible-test-sanity-lint/aliases create mode 100644 test/integration/targets/ansible-test-sanity-lint/expected.txt create mode 100755 test/integration/targets/ansible-test-sanity-lint/runme.sh create mode 100644 test/integration/targets/ansible-test-shell/aliases create mode 100644 test/integration/targets/ansible-test-shell/ansible_collections/ns/col/.keep create mode 100644 test/integration/targets/ansible-test-shell/expected-stderr.txt create mode 100644 test/integration/targets/ansible-test-shell/expected-stdout.txt create mode 100755 test/integration/targets/ansible-test-shell/runme.sh diff --git a/changelogs/fragments/ansible-test-tty-output-handling.yml b/changelogs/fragments/ansible-test-tty-output-handling.yml new file mode 100644 index 00000000000..58031dcd50a --- /dev/null +++ b/changelogs/fragments/ansible-test-tty-output-handling.yml @@ -0,0 +1,7 @@ +bugfixes: + - ansible-test - The ``shell`` command no longer requests a TTY when using delegation unless an interactive shell is being used. + An interactive shell is the default behavior when no command is given to pass to the shell. + - ansible-test - The ``shell`` command no longer redirects all output to stdout when running a provided command. + Any command output written to stderr will be mixed with the stderr output from ansible-test. + - ansible-test - Delegation for commands which generate output for programmatic consumption no longer redirect all output to stdout. + The affected commands and options are ``shell``, ``sanity --lint``, ``sanity --list-tests``, ``integration --list-targets``, ``coverage analyze`` diff --git a/test/integration/targets/ansible-test-sanity-lint/aliases b/test/integration/targets/ansible-test-sanity-lint/aliases new file mode 100644 index 00000000000..193276cc9e5 --- /dev/null +++ b/test/integration/targets/ansible-test-sanity-lint/aliases @@ -0,0 +1,4 @@ +shippable/posix/group1 # runs in the distro test containers +shippable/generic/group1 # runs in the default test container +context/controller +needs/target/collection diff --git a/test/integration/targets/ansible-test-sanity-lint/expected.txt b/test/integration/targets/ansible-test-sanity-lint/expected.txt new file mode 100644 index 00000000000..94238c8a879 --- /dev/null +++ b/test/integration/targets/ansible-test-sanity-lint/expected.txt @@ -0,0 +1 @@ +plugins/modules/python-wrong-shebang.py:1:1: expected module shebang "b'#!/usr/bin/python'" but found: b'#!invalid' diff --git a/test/integration/targets/ansible-test-sanity-lint/runme.sh b/test/integration/targets/ansible-test-sanity-lint/runme.sh new file mode 100755 index 00000000000..3e73cb4a615 --- /dev/null +++ b/test/integration/targets/ansible-test-sanity-lint/runme.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# Make sure that `ansible-test sanity --lint` outputs the correct format to stdout, even when delegation is used. + +set -eu + +# Create test scenarios at runtime that do not pass sanity tests. +# This avoids the need to create ignore entries for the tests. + +mkdir -p ansible_collections/ns/col/plugins/modules + +( + cd ansible_collections/ns/col/plugins/modules + + echo '#!invalid' > python-wrong-shebang.py # expected module shebang "b'#!/usr/bin/python'" but found: b'#!invalid' +) + +source ../collection/setup.sh + +set -x + +### +### Run the sanity test with the `--lint` option. +### + +# Use the `--venv` option to verify that delegation preserves the output streams. +ansible-test sanity --test shebang --color --failure-ok --lint --venv "${@}" 1> actual-stdout.txt 2> actual-stderr.txt +diff -u "${TEST_DIR}/expected.txt" actual-stdout.txt +grep -f "${TEST_DIR}/expected.txt" actual-stderr.txt + +# Run without delegation to verify direct output uses the correct streams. +ansible-test sanity --test shebang --color --failure-ok --lint "${@}" 1> actual-stdout.txt 2> actual-stderr.txt +diff -u "${TEST_DIR}/expected.txt" actual-stdout.txt +grep -f "${TEST_DIR}/expected.txt" actual-stderr.txt + +### +### Run the sanity test without the `--lint` option. +### + +# Use the `--venv` option to verify that delegation preserves the output streams. +ansible-test sanity --test shebang --color --failure-ok --venv "${@}" 1> actual-stdout.txt 2> actual-stderr.txt +grep -f "${TEST_DIR}/expected.txt" actual-stdout.txt +[ ! -s actual-stderr.txt ] + +# Run without delegation to verify direct output uses the correct streams. +ansible-test sanity --test shebang --color --failure-ok "${@}" 1> actual-stdout.txt 2> actual-stderr.txt +grep -f "${TEST_DIR}/expected.txt" actual-stdout.txt +[ ! -s actual-stderr.txt ] diff --git a/test/integration/targets/ansible-test-shell/aliases b/test/integration/targets/ansible-test-shell/aliases new file mode 100644 index 00000000000..193276cc9e5 --- /dev/null +++ b/test/integration/targets/ansible-test-shell/aliases @@ -0,0 +1,4 @@ +shippable/posix/group1 # runs in the distro test containers +shippable/generic/group1 # runs in the default test container +context/controller +needs/target/collection diff --git a/test/integration/targets/ansible-test-shell/ansible_collections/ns/col/.keep b/test/integration/targets/ansible-test-shell/ansible_collections/ns/col/.keep new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/integration/targets/ansible-test-shell/expected-stderr.txt b/test/integration/targets/ansible-test-shell/expected-stderr.txt new file mode 100644 index 00000000000..af6415db3c7 --- /dev/null +++ b/test/integration/targets/ansible-test-shell/expected-stderr.txt @@ -0,0 +1 @@ +stderr diff --git a/test/integration/targets/ansible-test-shell/expected-stdout.txt b/test/integration/targets/ansible-test-shell/expected-stdout.txt new file mode 100644 index 00000000000..faa3a15c184 --- /dev/null +++ b/test/integration/targets/ansible-test-shell/expected-stdout.txt @@ -0,0 +1 @@ +stdout diff --git a/test/integration/targets/ansible-test-shell/runme.sh b/test/integration/targets/ansible-test-shell/runme.sh new file mode 100755 index 00000000000..0e0d18ae30e --- /dev/null +++ b/test/integration/targets/ansible-test-shell/runme.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash +# Make sure that `ansible-test shell` outputs to the correct stream. + +set -eu + +source ../collection/setup.sh + +set -x + +# Try `shell` with delegation. + +ansible-test shell --venv -- \ + python -c 'import sys; print("stdout"); print("stderr", file=sys.stderr)' 1> actual-stdout.txt 2> actual-stderr.txt + +cat actual-stdout.txt +cat actual-stderr.txt + +diff -u "${TEST_DIR}/expected-stdout.txt" actual-stdout.txt +grep -f "${TEST_DIR}/expected-stderr.txt" actual-stderr.txt + +# Try `shell` without delegation. + +ansible-test shell -- \ + python -c 'import sys; print("stdout"); print("stderr", file=sys.stderr)' 1> actual-stdout.txt 2> actual-stderr.txt + +cat actual-stdout.txt +cat actual-stderr.txt + +diff -u "${TEST_DIR}/expected-stdout.txt" actual-stdout.txt +grep -f "${TEST_DIR}/expected-stderr.txt" actual-stderr.txt diff --git a/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py b/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py index 412414050f3..267969886ea 100644 --- a/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py +++ b/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py @@ -29,10 +29,6 @@ TargetSetIndexes = t.Dict[t.FrozenSet[int], int] class CoverageAnalyzeTargetsConfig(CoverageAnalyzeConfig): """Configuration for the `coverage analyze targets` command.""" - def __init__(self, args): # type: (t.Any) -> None - super().__init__(args) - - self.display_stderr = True def make_report(target_indexes, arcs, lines): # type: (TargetIndexes, Arcs, Lines) -> t.Dict[str, t.Any] diff --git a/test/lib/ansible_test/_internal/commands/shell/__init__.py b/test/lib/ansible_test/_internal/commands/shell/__init__.py index 4bd3fde02a7..e62437ead7e 100644 --- a/test/lib/ansible_test/_internal/commands/shell/__init__.py +++ b/test/lib/ansible_test/_internal/commands/shell/__init__.py @@ -7,6 +7,7 @@ import typing as t from ...util import ( ApplicationError, + OutputStream, display, ) @@ -82,7 +83,10 @@ def command_shell(args): # type: (ShellConfig) -> None return if args.cmd: - con.run(args.cmd, capture=False, interactive=False, force_stdout=True) + # Running a command is assumed to be non-interactive. Only a shell (no command) is interactive. + # If we want to support interactive commands in the future, we'll need an `--interactive` command line option. + # Command stderr output is allowed to mix with our own output, which is all sent to stderr. + con.run(args.cmd, capture=False, interactive=False, output_stream=OutputStream.ORIGINAL) return if isinstance(con, SshConnection) and args.raw: diff --git a/test/lib/ansible_test/_internal/config.py b/test/lib/ansible_test/_internal/config.py index 1e08cfb4782..27395678c7f 100644 --- a/test/lib/ansible_test/_internal/config.py +++ b/test/lib/ansible_test/_internal/config.py @@ -217,7 +217,7 @@ class ShellConfig(EnvironmentConfig): self.cmd = args.cmd # type: t.List[str] self.raw = args.raw # type: bool self.check_layout = self.delegate # allow shell to be used without a valid layout as long as no delegation is required - self.interactive = True + self.interactive = sys.stdin.isatty() and not args.cmd # delegation should only be interactive when stdin is a TTY and no command was given self.export = args.export # type: t.Optional[str] self.display_stderr = True diff --git a/test/lib/ansible_test/_internal/connections.py b/test/lib/ansible_test/_internal/connections.py index 05b53ac3040..0bcdb346aeb 100644 --- a/test/lib/ansible_test/_internal/connections.py +++ b/test/lib/ansible_test/_internal/connections.py @@ -16,6 +16,7 @@ from .config import ( from .util import ( Display, + OutputStream, SubprocessError, retry, ) @@ -50,7 +51,7 @@ class Connection(metaclass=abc.ABCMeta): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" @@ -98,7 +99,7 @@ class LocalConnection(Connection): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" return run_command( @@ -109,7 +110,7 @@ class LocalConnection(Connection): stdin=stdin, stdout=stdout, interactive=interactive, - force_stdout=force_stdout, + output_stream=output_stream, ) @@ -140,7 +141,7 @@ class SshConnection(Connection): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" options = list(self.options) @@ -174,7 +175,7 @@ class SshConnection(Connection): stdin=stdin, stdout=stdout, interactive=interactive, - force_stdout=force_stdout, + output_stream=output_stream, error_callback=error_callback, ) @@ -222,7 +223,7 @@ class DockerConnection(Connection): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" options = [] @@ -243,7 +244,7 @@ class DockerConnection(Connection): stdin=stdin, stdout=stdout, interactive=interactive, - force_stdout=force_stdout, + output_stream=output_stream, ) def inspect(self): # type: () -> DockerInspect diff --git a/test/lib/ansible_test/_internal/delegation.py b/test/lib/ansible_test/_internal/delegation.py index 79d2dc7cbc8..975b6fc7e53 100644 --- a/test/lib/ansible_test/_internal/delegation.py +++ b/test/lib/ansible_test/_internal/delegation.py @@ -27,6 +27,7 @@ from .util import ( ANSIBLE_BIN_PATH, ANSIBLE_LIB_ROOT, ANSIBLE_TEST_ROOT, + OutputStream, ) from .util_common import ( @@ -191,7 +192,12 @@ def delegate_command(args, host_state, exclude, require): # type: (EnvironmentC success = False try: - con.run(insert_options(command, options), capture=False, interactive=args.interactive) + # When delegating, preserve the original separate stdout/stderr streams, but only when the following conditions are met: + # 1) Display output is being sent to stderr. This indicates the output on stdout must be kept separate from stderr. + # 2) The delegation is non-interactive. Interactive mode, which generally uses a TTY, is not compatible with intercepting stdout/stderr. + # The downside to having separate streams is that individual lines of output from each are more likely to appear out-of-order. + output_stream = OutputStream.ORIGINAL if args.display_stderr and not args.interactive else None + con.run(insert_options(command, options), capture=False, interactive=args.interactive, output_stream=output_stream) success = True finally: if host_delegation: diff --git a/test/lib/ansible_test/_internal/docker_util.py b/test/lib/ansible_test/_internal/docker_util.py index f906d394c4a..6de2547938b 100644 --- a/test/lib/ansible_test/_internal/docker_util.py +++ b/test/lib/ansible_test/_internal/docker_util.py @@ -20,6 +20,7 @@ from .util import ( find_executable, SubprocessError, cache, + OutputStream, ) from .util_common import ( @@ -516,7 +517,7 @@ def docker_exec( stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] interactive=False, # type: bool - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] data=None, # type: t.Optional[str] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Execute the given command in the specified container.""" @@ -527,7 +528,7 @@ def docker_exec( options.append('-i') return docker_command(args, ['exec'] + options + [container_id] + cmd, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, - force_stdout=force_stdout, data=data) + output_stream=output_stream, data=data) def docker_info(args): # type: (CommonConfig) -> t.Dict[str, t.Any] @@ -549,7 +550,7 @@ def docker_command( stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] interactive=False, # type: bool - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] always=False, # type: bool data=None, # type: t.Optional[str] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] @@ -561,7 +562,7 @@ def docker_command( command.append('--remote') return run_command(args, command + cmd, env=env, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, always=always, - force_stdout=force_stdout, data=data) + output_stream=output_stream, data=data) def docker_environment(): # type: () -> t.Dict[str, str] diff --git a/test/lib/ansible_test/_internal/util.py b/test/lib/ansible_test/_internal/util.py index 3ea0f57e39f..17a88adb98f 100644 --- a/test/lib/ansible_test/_internal/util.py +++ b/test/lib/ansible_test/_internal/util.py @@ -3,6 +3,7 @@ from __future__ import annotations import abc import errno +import enum import fcntl import importlib.util import inspect @@ -100,6 +101,24 @@ MODE_DIRECTORY = MODE_READ | stat.S_IWUSR | stat.S_IXUSR | stat.S_IXGRP | stat.S MODE_DIRECTORY_WRITE = MODE_DIRECTORY | stat.S_IWGRP | stat.S_IWOTH +class OutputStream(enum.Enum): + """The output stream to use when running a subprocess and redirecting/capturing stdout or stderr.""" + + ORIGINAL = enum.auto() + AUTO = enum.auto() + + def get_buffer(self, original: t.BinaryIO) -> t.BinaryIO: + """Return the correct output buffer to use, taking into account the given original buffer.""" + + if self == OutputStream.ORIGINAL: + return original + + if self == OutputStream.AUTO: + return display.fd.buffer + + raise NotImplementedError(str(self)) + + class Architecture: """ Normalized architecture names. @@ -330,12 +349,14 @@ def raw_command( stdin=None, # type: t.Optional[t.Union[t.IO[bytes], int]] stdout=None, # type: t.Optional[t.Union[t.IO[bytes], int]] interactive=False, # type: bool - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] cmd_verbosity=1, # type: int str_errors='strict', # type: str error_callback=None, # type: t.Optional[t.Callable[[SubprocessError], None]] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return stdout and stderr as a tuple.""" + output_stream = output_stream or OutputStream.AUTO + if capture and interactive: raise InternalError('Cannot combine capture=True with interactive=True.') @@ -354,11 +375,11 @@ def raw_command( if stdout and not capture: raise InternalError('Redirection of stdout requires capture=True to avoid redirection of stderr to stdout.') - if force_stdout and capture: - raise InternalError('Cannot combine force_stdout=True with capture=True.') + if output_stream != OutputStream.AUTO and capture: + raise InternalError(f'Cannot combine {output_stream=} with capture=True.') - if force_stdout and interactive: - raise InternalError('Cannot combine force_stdout=True with interactive=True.') + if output_stream != OutputStream.AUTO and interactive: + raise InternalError(f'Cannot combine {output_stream=} with interactive=True.') if not cwd: cwd = os.getcwd() @@ -425,9 +446,9 @@ def raw_command( # This prevents subprocesses from sharing stdout/stderr with the current process or each other. # Doing so allows subprocesses to safely make changes to their file handles, such as making them non-blocking (ssh does this). # This also maintains consistency between local testing and CI systems, which typically do not provide a TTY. - # To maintain output ordering, a single pipe is used for both stdout/stderr when not capturing output. + # To maintain output ordering, a single pipe is used for both stdout/stderr when not capturing output unless the output stream is ORIGINAL. stdout = stdout or subprocess.PIPE - stderr = subprocess.PIPE if capture else subprocess.STDOUT + stderr = subprocess.PIPE if capture or output_stream == OutputStream.ORIGINAL else subprocess.STDOUT communicate = True else: stderr = None @@ -448,7 +469,7 @@ def raw_command( if communicate: data_bytes = to_optional_bytes(data) stdout_bytes, stderr_bytes = communicate_with_process(process, data_bytes, stdout == subprocess.PIPE, stderr == subprocess.PIPE, capture=capture, - force_stdout=force_stdout) + output_stream=output_stream) stdout_text = to_optional_text(stdout_bytes, str_errors) or '' stderr_text = to_optional_text(stderr_bytes, str_errors) or '' else: @@ -477,7 +498,7 @@ def communicate_with_process( stdout: bool, stderr: bool, capture: bool, - force_stdout: bool + output_stream: OutputStream, ) -> t.Tuple[bytes, bytes]: """Communicate with the specified process, handling stdin/stdout/stderr as requested.""" threads: t.List[WrappedThread] = [] @@ -492,13 +513,13 @@ def communicate_with_process( threads.append(WriterThread(process.stdin, stdin)) if stdout: - stdout_reader = reader(process.stdout, force_stdout) + stdout_reader = reader(process.stdout, output_stream.get_buffer(sys.stdout.buffer)) threads.append(stdout_reader) else: stdout_reader = None if stderr: - stderr_reader = reader(process.stderr, force_stdout) + stderr_reader = reader(process.stderr, output_stream.get_buffer(sys.stderr.buffer)) threads.append(stderr_reader) else: stderr_reader = None @@ -546,11 +567,11 @@ class WriterThread(WrappedThread): class ReaderThread(WrappedThread, metaclass=abc.ABCMeta): """Thread to read stdout from a subprocess.""" - def __init__(self, handle: t.IO[bytes], force_stdout: bool) -> None: + def __init__(self, handle: t.IO[bytes], buffer: t.BinaryIO) -> None: super().__init__(self._run) self.handle = handle - self.force_stdout = force_stdout + self.buffer = buffer self.lines = [] # type: t.List[bytes] @abc.abstractmethod @@ -577,7 +598,7 @@ class OutputThread(ReaderThread): def _run(self) -> None: """Workload to run on a thread.""" src = self.handle - dst = sys.stdout.buffer if self.force_stdout else display.fd.buffer + dst = self.buffer try: for line in src: diff --git a/test/lib/ansible_test/_internal/util_common.py b/test/lib/ansible_test/_internal/util_common.py index 12c96fc4f0e..f7b8bfc59b5 100644 --- a/test/lib/ansible_test/_internal/util_common.py +++ b/test/lib/ansible_test/_internal/util_common.py @@ -27,6 +27,7 @@ from .util import ( MODE_DIRECTORY, MODE_FILE_EXECUTE, MODE_FILE, + OutputStream, PYTHON_PATHS, raw_command, ANSIBLE_TEST_DATA_ROOT, @@ -409,7 +410,7 @@ def run_command( stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] interactive=False, # type: bool - force_stdout=False, # type: bool + output_stream=None, # type: t.Optional[OutputStream] cmd_verbosity=1, # type: int str_errors='strict', # type: str error_callback=None, # type: t.Optional[t.Callable[[SubprocessError], None]] @@ -417,7 +418,7 @@ def run_command( """Run the specified command and return stdout and stderr as a tuple.""" explain = args.explain and not always return raw_command(cmd, capture=capture, env=env, data=data, cwd=cwd, explain=explain, stdin=stdin, stdout=stdout, interactive=interactive, - force_stdout=force_stdout, cmd_verbosity=cmd_verbosity, str_errors=str_errors, error_callback=error_callback) + output_stream=output_stream, cmd_verbosity=cmd_verbosity, str_errors=str_errors, error_callback=error_callback) def yamlcheck(python):