Merge pull request #108 from nicoddemus/diff-opt

Implement --diff-range-notation option

CHANGELOG

@@ -1,3 +1,16 @@
+(unreleased)
+
+ * New option that controls how the patch is obtained: `--diff-range-notation`, defaulting to `...`.
+
+   Traditionally in git-cover the symmetric difference (three-dot, "A...M") notation has been used: it includes
+   commits reachable from A and M from their merge-base, but not both, taking history in account. This includes cherry-picks
+   between A and M, which are harmless and do not produce changes, but might give inaccurate coverage false-negatives.
+
+   Two-dot range notation ("A..M") compares the tips of both trees and produces a diff. This more
+   accurately describes the actual patch that will be applied by merging A into M, even if commits have been
+   cherry-picked between branches. This will produce a more accurate diff for coverage comparison when complex
+   merges and cherry-picks are involved.
+
 5/10/2019 v2.0.1
 
  * Ensure case is normalized on Windows when comparing paths from XML coverage report Thanks @nicoddemus!

diff_cover/diff_cover_tool.py

@@ -25,6 +25,7 @@
 EXCLUDE_HELP = "Exclude files, more patterns supported"
 SRC_ROOTS_HELP = "List of source directories (only for jacoco coverage reports)"
 COVERAGE_XML_HELP = "XML coverage report"
+DIFF_RANGE_NOTATION_HELP = "Git diff range notation to use when comparing branches, defaults to '...'"
 
 LOGGER = logging.getLogger(__name__)
 
@@ -116,19 +117,29 @@ def parse_coverage_args(argv):
         help=SRC_ROOTS_HELP
     )
 
+    parser.add_argument(
+        '--diff-range-notation',
+        metavar='RANGE_NOTATION',
+        type=str,
+        default='...',
+        choices=['...', '..'],
+        help=DIFF_RANGE_NOTATION_HELP
+    )
+
     return vars(parser.parse_args(argv))
 
 
 def generate_coverage_report(coverage_xml, compare_branch,
                              html_report=None, css_file=None,
                              ignore_staged=False, ignore_unstaged=False,
-                             exclude=None, src_roots=None):
+                             exclude=None, src_roots=None, diff_range_notation=None):
     """
     Generate the diff coverage report, using kwargs from `parse_args()`.
     """
     diff = GitDiffReporter(
-        compare_branch, git_diff=GitDiffTool(), ignore_staged=ignore_staged,
-        ignore_unstaged=ignore_unstaged, exclude=exclude)
+        compare_branch, git_diff=GitDiffTool(diff_range_notation),
+        ignore_staged=ignore_staged, ignore_unstaged=ignore_unstaged,
+        exclude=exclude)
 
     xml_roots = [cElementTree.parse(xml_root) for xml_root in coverage_xml]
     coverage = XmlCoverageReporter(xml_roots, src_roots)
@@ -176,6 +187,7 @@ def main(argv=None, directory=None):
         ignore_unstaged=arg_dict['ignore_unstaged'],
         exclude=arg_dict['exclude'],
         src_roots=arg_dict['src_roots'],
+        diff_range_notation=arg_dict['diff_range_notation']
     )
 
     if percent_covered >= fail_under:

diff_cover/diff_quality_tool.py

@@ -11,8 +11,8 @@
 import six
 
 import diff_cover
-from diff_cover.diff_cover_tool import COMPARE_BRANCH_HELP, FAIL_UNDER_HELP, IGNORE_STAGED_HELP, IGNORE_UNSTAGED_HELP, \
-    EXCLUDE_HELP, HTML_REPORT_HELP, CSS_FILE_HELP
+from diff_cover.diff_cover_tool import COMPARE_BRANCH_HELP, DIFF_RANGE_NOTATION_HELP, FAIL_UNDER_HELP, \
+    IGNORE_STAGED_HELP, IGNORE_UNSTAGED_HELP, EXCLUDE_HELP, HTML_REPORT_HELP, CSS_FILE_HELP
 from diff_cover.diff_reporter import GitDiffReporter
 from diff_cover.git_diff import GitDiffTool
 from diff_cover.git_path import GitPathTool
@@ -143,18 +143,26 @@ def parse_quality_args(argv):
         help=EXCLUDE_HELP
     )
 
+    parser.add_argument(
+        '--diff-range-notation',
+        metavar='RANGE_NOTATION',
+        type=str,
+        default='...',
+        help=DIFF_RANGE_NOTATION_HELP
+    )
+
     return vars(parser.parse_args(argv))
 
 
 def generate_quality_report(tool, compare_branch,
                             html_report=None, css_file=None,
                             ignore_staged=False, ignore_unstaged=False,
-                            exclude=None):
+                            exclude=None, diff_range_notation=None):
     """
     Generate the quality report, using kwargs from `parse_args()`.
     """
     diff = GitDiffReporter(
-        compare_branch, git_diff=GitDiffTool(),
+        compare_branch, git_diff=GitDiffTool(diff_range_notation),
         ignore_staged=ignore_staged, ignore_unstaged=ignore_unstaged,
         supported_extensions=tool.driver.supported_extensions,
         exclude=exclude)
@@ -222,6 +230,7 @@ def main(argv=None, directory=None):
                 ignore_staged=arg_dict['ignore_staged'],
                 ignore_unstaged=arg_dict['ignore_unstaged'],
                 exclude=arg_dict['exclude'],
+                diff_range_notation=arg_dict['diff_range_notation'],
             )
             if percent_passing >= fail_under:
                 return 0

diff_cover/git_diff.py

@@ -18,6 +18,24 @@ class GitDiffTool(object):
     Thin wrapper for a subset of the `git diff` command.
     """
 
+    def __init__(self, range_notation):
+        """
+        :param str range_notation:
+            which range notation to use when producing the diff for committed
+            files against another branch.
+
+            Traditionally in git-cover the symmetric difference (three-dot, "A...M") notation has been used: it
+            includes commits reachable from A and M from their merge-base, but not both, taking history in account.
+            This includes cherry-picks between A and M, which are harmless and do not produce changes, but might give
+            inaccurate coverage false-negatives.
+
+            Two-dot range notation ("A..M") compares the tips of both trees and produces a diff. This more accurately
+            describes the actual patch that will be applied by merging A into M, even if commits have been
+            cherry-picked between branches. This will produce a more accurate diff for coverage comparison when
+            complex merges and cherry-picks are involved.
+        """
+        self._range_notation = range_notation
+
     def diff_committed(self, compare_branch='origin/master'):
         """
         Returns the output of `git diff` for committed
@@ -30,7 +48,7 @@ def diff_committed(self, compare_branch='origin/master'):
             'git',
             '-c', 'diff.mnemonicprefix=no',
             '-c', 'diff.noprefix=no',
-            'diff', '{branch}...HEAD'.format(branch=compare_branch),
+            'diff', '{branch}{notation}HEAD'.format(branch=compare_branch, notation=self._range_notation),
             '--no-color',
             '--no-ext-diff'
         ])[0]

diff_cover/tests/test_diff_cover_main.py

@@ -0,0 +1,33 @@
+from __future__ import unicode_literals
+
+import pytest
+
+from diff_cover.diff_cover_tool import parse_coverage_args
+
+
+class TestParseCoverArgsTest:
+
+    def test_parse_coverage_xml(self):
+        argv = ['build/tests/coverage.xml', '--compare-branch=origin/other']
+
+        arg_dict = parse_coverage_args(argv)
+
+        assert arg_dict['coverage_xml'] == ['build/tests/coverage.xml']
+        assert arg_dict['compare_branch'] == 'origin/other'
+        assert arg_dict['diff_range_notation'] == '...'
+
+    def test_parse_range_notation(self, capsys):
+        argv = ['build/tests/coverage.xml', '--diff-range-notation=..']
+
+        arg_dict = parse_coverage_args(argv)
+
+        assert arg_dict['coverage_xml'] == ['build/tests/coverage.xml']
+        assert arg_dict['diff_range_notation'] == '..'
+
+        with pytest.raises(SystemExit) as e:
+            argv = ['build/tests/coverage.xml', '--diff-range-notation=FOO']
+            parse_coverage_args(argv)
+
+        assert e.value.code == 2
+        _, err = capsys.readouterr()
+        assert "invalid choice: 'FOO'" in err

diff_cover/tests/test_diff_quality_main.py

@@ -19,6 +19,7 @@ def test_parse_with_html_report(self):
         self.assertEqual(arg_dict.get('html_report'), 'diff_cover.html')
         self.assertEqual(arg_dict.get('input_reports'), [])
         self.assertEqual(arg_dict.get('ignore_unstaged'), False)
+        self.assertEqual(arg_dict.get('diff_range_notation'), '...')
 
     def test_parse_with_no_html_report(self):
         argv = ['--violations', 'pylint']
@@ -27,6 +28,7 @@ def test_parse_with_no_html_report(self):
         self.assertEqual(arg_dict.get('violations'), 'pylint')
         self.assertEqual(arg_dict.get('input_reports'), [])
         self.assertEqual(arg_dict.get('ignore_unstaged'), False)
+        self.assertEqual(arg_dict.get('diff_range_notation'), '...')
 
     def test_parse_with_one_input_report(self):
         argv = ['--violations', 'pylint', 'pylint_report.txt']
@@ -89,6 +91,18 @@ def test_parse_with_exclude(self):
         self.assertEqual(arg_dict.get('exclude'),
                          ['noneed/*.py', 'other/**/*.py'])
 
+    def test_parse_diff_range_notation(self):
+        argv = ['--violations', 'pep8',
+                '--diff-range-notation=..']
+
+        arg_dict = parse_quality_args(argv)
+
+        self.assertEqual(arg_dict.get('violations'), 'pep8')
+        self.assertEqual(arg_dict.get('html_report'), None)
+        self.assertEqual(arg_dict.get('input_reports'), [])
+        self.assertEqual(arg_dict.get('ignore_unstaged'), False)
+        self.assertEqual(arg_dict.get('diff_range_notation'), '..')
+
 
 class MainTest(unittest.TestCase):
     """Tests for the main() function in tool.py"""

diff_cover/tests/test_git_diff.py

@@ -15,11 +15,12 @@ def setUp(self):
         self.process.returncode = 0
         self.subprocess = mock.patch('diff_cover.command_runner.subprocess').start()
         self.subprocess.Popen.return_value = self.process
+        self.addCleanup(mock.patch.stopall)
         # Create the git diff tool
-        self.tool = GitDiffTool()
-
-    def test_diff_committed(self):
+        self.tool = GitDiffTool('...')
 
+    def check_diff_committed(self, diff_range_notation):
+        self.tool = GitDiffTool(diff_range_notation)
         self._set_git_diff_output('test output', '')
         output = self.tool.diff_committed()
 
@@ -28,12 +29,16 @@ def test_diff_committed(self):
 
         # Expect that the correct command was executed
         expected = ['git', '-c', 'diff.mnemonicprefix=no', '-c',
-                    'diff.noprefix=no', 'diff', 'origin/master...HEAD',
+                    'diff.noprefix=no', 'diff', 'origin/master{}HEAD'.format(diff_range_notation),
                     '--no-color', '--no-ext-diff']
         self.subprocess.Popen.assert_called_with(
             expected, stdout=self.subprocess.PIPE, stderr=self.subprocess.PIPE
         )
 
+    def test_diff_commited(self):
+        self.check_diff_committed('...')
+        self.check_diff_committed('..')
+
     def test_diff_unstaged(self):
         self._set_git_diff_output('test output', '')
         output = self.tool.diff_unstaged()

diff_cover/tests/test_git_path.py

@@ -12,9 +12,9 @@ def setUp(self):
         self.process.returncode = 0
         self.subprocess = mock.patch('diff_cover.command_runner.subprocess').start()
         self.subprocess.Popen.return_value = self.process
+        self.addCleanup(mock.patch.stopall)
 
     def tearDown(self):
-        mock.patch.stopall()
         # Reset static class members
         GitPathTool._root = None
         GitPathTool._cwd = None

diff_cover/tests/test_java_violations_reporter.py

@@ -1,8 +1,6 @@
 # -*- coding: utf-8 -*-
 from __future__ import unicode_literals
 
-import os
-import xml.etree.cElementTree as etree
 from textwrap import dedent
 import unittest
 import mock
@@ -38,10 +36,7 @@ class CheckstyleQualityReporterTest(unittest.TestCase):
     def setUp(self):
         """Set up required files."""
         _patch_so_all_files_exist()
-
-    def tearDown(self):
-        """Undo all patches."""
-        patch.stopall()
+        self.addCleanup(patch.stopall)
 
     def test_no_such_file(self):
         """Expect that we get no results."""

diff_cover/tests/test_snippets.py

@@ -119,18 +119,13 @@ def setUp(self):
         Create a temporary source file.
         """
         _, self._src_path = tempfile.mkstemp()
+        self.addCleanup(os.remove, self._src_path)
 
         # Path tool should not be aware of testing command
         path_mock = mock.patch('diff_cover.violationsreporters.violations_reporter.GitPathTool').start()
         path_mock.absolute_path = lambda path: path
         path_mock.relative_path = lambda path: path
-
-    def tearDown(self):
-        """
-        Delete the temporary source file.
-        """
-        os.remove(self._src_path)
-        mock.patch.stopall()
+        self.addCleanup(mock.patch.stopall)
 
     def test_one_snippet(self):
         self._init_src_file(10)

diff_cover/tests/test_violations_reporter.py

@@ -54,9 +54,7 @@ def setUp(self):
         _git_path_mock = patch('diff_cover.violationsreporters.violations_reporter.GitPathTool').start()
         _git_path_mock.relative_path = lambda path: path
         _git_path_mock.absolute_path = lambda path: path
-
-    def tearDown(self):
-        patch.stopall()
+        self.addCleanup(patch.stopall)
 
     def test_violations(self):