From 0a7f2c202b6b3dc5af7ead002c834ca1eeeca8ad Mon Sep 17 00:00:00 2001 From: Marius Gedminas Date: Thu, 11 Feb 2016 10:09:24 +0200 Subject: [PATCH] Improve --diff output when files lack trailing newlines The behavior now matches GNU diff. Fixes #14094. Example of output before this change: TASK [healthchecks.io : hourly healthchecks.io ping] *************************** changed: [ranka] --- before: /etc/cron.hourly/mg-healthchecks-dot-io +++ after: /tmp/tmpOTvXTw @@ -1,2 +1,2 @@ #!/bin/sh -curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null+curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null after this change: TASK [healthchecks.io : hourly healthchecks.io ping] *************************** changed: [ranka] --- before: /etc/cron.hourly/mg-healthchecks-dot-io +++ after: /tmp/tmpOTvXTw @@ -1,2 +1,2 @@ #!/bin/sh -curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null \ No newline at end of file +curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null The added unit tests contain more examples. This commit also takes care to avoid "no newline at EOF" warnings when no_log is in effect, and also when modules return dicts rather than strings. (It also removes trailing whitespace from using json serialization when diffing dicts, because I hate trailing whitespace in Python source files, even if they're test files.) --- lib/ansible/plugins/action/__init__.py | 2 +- lib/ansible/plugins/callback/__init__.py | 25 ++- test/units/plugins/callback/test_callback.py | 157 ++++++++++++++++++- 3 files changed, 176 insertions(+), 8 deletions(-) diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 2544fd5691..b7ef96ded3 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -864,7 +864,7 @@ class ActionBase(with_metaclass(ABCMeta, object)): if 'before' in diff: diff["before"] = "" if 'after' in diff: - diff["after"] = " [[ Diff output has been hidden because 'no_log: true' was specified for this result ]]" + diff["after"] = " [[ Diff output has been hidden because 'no_log: true' was specified for this result ]]\n" return diff diff --git a/lib/ansible/plugins/callback/__init__.py b/lib/ansible/plugins/callback/__init__.py index 84393e0360..1005309881 100644 --- a/lib/ansible/plugins/callback/__init__.py +++ b/lib/ansible/plugins/callback/__init__.py @@ -19,8 +19,9 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type -import json import difflib +import json +import sys import warnings from copy import deepcopy @@ -124,7 +125,7 @@ class CallbackBase: # format complex structures into 'files' for x in ['before', 'after']: if isinstance(diff[x], dict): - diff[x] = json.dumps(diff[x], sort_keys=True, indent=4) + diff[x] = json.dumps(diff[x], sort_keys=True, indent=4, separators=(',', ': ')) + '\n' if 'before_header' in diff: before_header = "before: %s" % diff['before_header'] else: @@ -133,15 +134,29 @@ class CallbackBase: after_header = "after: %s" % diff['after_header'] else: after_header = 'after' - differ = difflib.unified_diff(to_text(diff['before']).splitlines(True), - to_text(diff['after']).splitlines(True), + before_lines = to_text(diff['before']).splitlines(True) + after_lines = to_text(diff['after']).splitlines(True) + if before_lines and not before_lines[-1].endswith('\n'): + before_lines[-1] += '\n\\ No newline at end of file\n' + if after_lines and not after_lines[-1].endswith('\n'): + after_lines[-1] += '\n\\ No newline at end of file\n' + differ = difflib.unified_diff(before_lines, + after_lines, fromfile=before_header, tofile=after_header, fromfiledate='', tofiledate='', n=C.DIFF_CONTEXT) + difflines = list(differ) + if len(difflines) >= 3 and sys.version_info[:2] == (2, 6): + # difflib in Python 2.6 adds trailing spaces after + # filenames in the -- before/++ after headers. + difflines[0] = difflines[0].replace(' \n', '\n') + difflines[1] = difflines[1].replace(' \n', '\n') + # it also treats empty files differently + difflines[2] = difflines[2].replace('-1,0', '-0,0').replace('+1,0', '+0,0') has_diff = False - for line in differ: + for line in difflines: has_diff = True if line.startswith('+'): line = stringc(line, C.COLOR_DIFF_ADD) diff --git a/test/units/plugins/callback/test_callback.py b/test/units/plugins/callback/test_callback.py index e743762c0b..87cbf3cd52 100644 --- a/test/units/plugins/callback/test_callback.py +++ b/test/units/plugins/callback/test_callback.py @@ -19,6 +19,8 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import re +import textwrap import types from ansible.compat.tests import unittest @@ -134,9 +136,15 @@ class TestCallbackDumpResults(unittest.TestCase): # that try except orig appeared in 61d01f549f2143fd9adfa4ffae42f09d24649c26 # in 2013 so maybe a < py2.6 issue class TestCallbackDiff(unittest.TestCase): + + def setUp(self): + self.cb = CallbackBase() + + def _strip_color(self, s): + return re.sub('\033\\[[^m]*m', '', s) + def test_difflist(self): # TODO: split into smaller tests? - cb = CallbackBase() difflist = [{'before': ['preface\nThe Before String\npostscript'], 'after': ['preface\nThe After String\npostscript'], 'before_header': 'just before', @@ -153,13 +161,158 @@ class TestCallbackDiff(unittest.TestCase): {'before_header': 'just before'}, {'after_header': 'just after'}] - res = cb._get_diff(difflist) + res = self.cb._get_diff(difflist) self.assertIn('Before String', res) self.assertIn('After String', res) self.assertIn('just before', res) self.assertIn('just after', res) + def test_simple_diff(self): + self.assertMultiLineEqual( + self._strip_color(self.cb._get_diff({ + 'before_header': 'somefile.txt', + 'after_header': 'generated from template somefile.j2', + 'before': 'one\ntwo\nthree\n', + 'after': 'one\nthree\nfour\n', + })), + textwrap.dedent('''\ + --- before: somefile.txt + +++ after: generated from template somefile.j2 + @@ -1,3 +1,3 @@ + one + -two + three + +four + + ''')) + + def test_new_file(self): + self.assertMultiLineEqual( + self._strip_color(self.cb._get_diff({ + 'before_header': 'somefile.txt', + 'after_header': 'generated from template somefile.j2', + 'before': '', + 'after': 'one\ntwo\nthree\n', + })), + textwrap.dedent('''\ + --- before: somefile.txt + +++ after: generated from template somefile.j2 + @@ -0,0 +1,3 @@ + +one + +two + +three + + ''')) + + def test_clear_file(self): + self.assertMultiLineEqual( + self._strip_color(self.cb._get_diff({ + 'before_header': 'somefile.txt', + 'after_header': 'generated from template somefile.j2', + 'before': 'one\ntwo\nthree\n', + 'after': '', + })), + textwrap.dedent('''\ + --- before: somefile.txt + +++ after: generated from template somefile.j2 + @@ -1,3 +0,0 @@ + -one + -two + -three + + ''')) + + def test_no_trailing_newline_before(self): + self.assertMultiLineEqual( + self._strip_color(self.cb._get_diff({ + 'before_header': 'somefile.txt', + 'after_header': 'generated from template somefile.j2', + 'before': 'one\ntwo\nthree', + 'after': 'one\ntwo\nthree\n', + })), + textwrap.dedent('''\ + --- before: somefile.txt + +++ after: generated from template somefile.j2 + @@ -1,3 +1,3 @@ + one + two + -three + \\ No newline at end of file + +three + + ''')) + + def test_no_trailing_newline_after(self): + self.assertMultiLineEqual( + self._strip_color(self.cb._get_diff({ + 'before_header': 'somefile.txt', + 'after_header': 'generated from template somefile.j2', + 'before': 'one\ntwo\nthree\n', + 'after': 'one\ntwo\nthree', + })), + textwrap.dedent('''\ + --- before: somefile.txt + +++ after: generated from template somefile.j2 + @@ -1,3 +1,3 @@ + one + two + -three + +three + \\ No newline at end of file + + ''')) + + def test_no_trailing_newline_both(self): + self.assertMultiLineEqual( + self.cb._get_diff({ + 'before_header': 'somefile.txt', + 'after_header': 'generated from template somefile.j2', + 'before': 'one\ntwo\nthree', + 'after': 'one\ntwo\nthree', + }), + '') + + def test_no_trailing_newline_both_with_some_changes(self): + self.assertMultiLineEqual( + self._strip_color(self.cb._get_diff({ + 'before_header': 'somefile.txt', + 'after_header': 'generated from template somefile.j2', + 'before': 'one\ntwo\nthree', + 'after': 'one\nfive\nthree', + })), + textwrap.dedent('''\ + --- before: somefile.txt + +++ after: generated from template somefile.j2 + @@ -1,3 +1,3 @@ + one + -two + +five + three + \\ No newline at end of file + + ''')) + + def test_diff_dicts(self): + self.assertMultiLineEqual( + self._strip_color(self.cb._get_diff({ + 'before': dict(one=1, two=2, three=3), + 'after': dict(one=1, three=3, four=4), + })), + textwrap.dedent('''\ + --- before + +++ after + @@ -1,5 +1,5 @@ + { + + "four": 4, + "one": 1, + - "three": 3, + - "two": 2 + + "three": 3 + } + + ''')) + class TestCallbackOnMethods(unittest.TestCase): def _find_on_methods(self, callback):