From 7e60c1f61ee6118f0b943e7897dc3a58ecc043bd Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Tue, 1 Feb 2022 12:10:51 -0500 Subject: [PATCH] Fix bug with truncating newline (#950) Fix bug with truncating newline This was followup promised in #943 Remaining concern Of course I love .rstrip for the simplicity of it, but there may be a case where this method receives multiple empty lines, and it is expected to keep them. Like, it gets foo\n\n\n, and it is expected to write foo\n\n. This whole thing is frustrating and confusing. This scenario is probably unlikely to happen because verbose events only write 1 line at a time anyway. There might still be a case I'm missing that's important, and in that case, this could be adjusted so that only 1 trailing newline is removed from stdout_chunk, instead of all newline characters. I expect that would be less performant, so if this satisfies the current needs I'd keep it. Reviewed-by: Alexander Sowitzki Reviewed-by: David Shrewsbury Reviewed-by: None --- ansible_runner/utils/__init__.py | 2 +- test/unit/test_runner.py | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/ansible_runner/utils/__init__.py b/ansible_runner/utils/__init__.py index a1ece9038..214b0a76a 100644 --- a/ansible_runner/utils/__init__.py +++ b/ansible_runner/utils/__init__.py @@ -410,7 +410,7 @@ def _emit_event(self, buffered_stdout, next_event_data=None): event_data['uuid'] = str(uuid.uuid4()) self._counter += 1 event_data['counter'] = self._counter - event_data['stdout'] = stdout_chunk[:-2] if len(stdout_chunk) > 2 else "" + event_data['stdout'] = stdout_chunk.rstrip('\n\r') n_lines = stdout_chunk.count('\n') event_data['start_line'] = self._start_line event_data['end_line'] = self._start_line + n_lines diff --git a/test/unit/test_runner.py b/test/unit/test_runner.py index f61bd32ad..208c86c4d 100644 --- a/test/unit/test_runner.py +++ b/test/unit/test_runner.py @@ -151,7 +151,7 @@ def test_status_callback_interface(rc, mocker): def test_stdout_file_write(rc, runner_mode): if runner_mode == 'pexpect': pytest.skip('Writing to stdout can be flaky, probably due to some pexpect bug') - rc.command = ['echo', 'hello_world_marker '] # workaround bug in stdout handl wrapper + rc.command = ['echo', 'hello_world_marker'] rc.runner_mode = runner_mode runner = Runner(config=rc) status, exitcode = runner.run() @@ -168,7 +168,7 @@ def test_stdout_file_write(rc, runner_mode): @pytest.mark.parametrize('runner_mode', ['pexpect', 'subprocess']) def test_stdout_file_no_write(rc, runner_mode): - rc.command = ['echo', 'hello_world_marker '] # workaround bug in stdout handl wrapper + rc.command = ['echo', 'hello_world_marker'] rc.runner_mode = runner_mode rc.suppress_output_file = True runner = Runner(config=rc) @@ -177,3 +177,14 @@ def test_stdout_file_no_write(rc, runner_mode): for filename in ('stdout', 'stderr'): stdout_path = Path(rc.artifact_dir) / filename assert not stdout_path.exists() + + +@pytest.mark.parametrize('runner_mode', ['pexpect', 'subprocess']) +def test_multiline_blank_write(rc, runner_mode): + rc.command = ['echo', 'hello_world_marker\n\n\n'] + rc.runner_mode = runner_mode + runner = Runner(config=rc) + status, exitcode = runner.run() + assert status == 'successful' + stdout_path = Path(rc.artifact_dir) / 'stdout' + assert stdout_path.read_text() == 'hello_world_marker\n\n\n\n' # one extra newline okay