launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/conflict-marker-test-git into lp:launchpad.

Commit message:
Add Git support to NoSpuriousConflictsMarkerTest.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/conflict-marker-test-git/+merge/373153

This test had in fact been somewhat broken for bzr for some time; but since it was apparently added for the reason that badly-resolved conflicts had in fact once made it into trunk (https://bugs.launchpad.net/launchpad/+bug/94798), I thought it better to fix the test than to remove it.

It's true that this should probably be something more like a pre-commit hook than a unit test, but I don't want to try to introduce extra VCS-specific machinery when this is part of preparing for a VCS conversion.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/conflict-marker-test-git into lp:launchpad.

=== modified file 'lib/lp/tests/test_no_conflict_marker.py'
--- lib/lp/tests/test_no_conflict_marker.py	2018-01-02 10:54:31 +0000
+++ lib/lp/tests/test_no_conflict_marker.py	2019-09-24 16:17:52 +0000
@@ -1,7 +1,7 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test that no files in the tree has spurious conflicts marker."""
+"""Test that no files in the tree has spurious conflicts markers."""
 
 __metaclass__ = type
 
@@ -10,39 +10,47 @@
 import unittest
 
 
-class NoSpuriousConlictsMarkerTest(unittest.TestCase):
-    """Check each file in the working tree for spurious conflicts marker."""
+class NoSpuriousConflictsMarkerTest(unittest.TestCase):
+    """Check each file in the working tree for spurious conflicts markers."""
 
     # We do not check for ======= because it might match some
     # old heading style in some doctests.
-    CONFLICT_MARKER_RE = r'^\(<<<<<<< TREE\|>>>>>>> MERGE-SOURCE\)$'
+    CONFLICT_MARKER_RE = r'^\(<<<<<<< \|>>>>>>> \)'
 
     # We could use bzrlib.workingtree for that test, but this cause
-    # problems when the system bzr (so the developers' branch) isn't at
+    # problems when the system bzr (so the developer's branch) isn't at
     # the same level than the bzrlib included in our tree. Anyway, it's
-    # probably faster to use grep anyway.
+    # probably faster to use grep.
     def test_noSpuriousConflictsMarker(self):
-        """Fail if any spurious conflicts marker are found."""
+        """Fail if any spurious conflicts markers are found."""
         root_dir = os.path.join(os.path.dirname(__file__), '../../..')
-        shell_command = "cd '%s' && bzr ls --versioned | xargs grep '%s'" % (
-                root_dir, self.CONFLICT_MARKER_RE)
-
-        # We need to reset PYTHONPATH here otherwise the bzrlib in our
-        # tree will be picked up.
+
+        if os.path.exists(os.path.join(root_dir, '.git')):
+            list_files_command = ['git', 'ls-files']
+        else:
+            list_files_command = ['bzr', 'ls', '-R', '--versioned']
+
+        # We need to reset PYTHONPATH here, otherwise the bzr in our tree
+        # will be picked up.
         new_env = dict(os.environ)
         new_env['PYTHONPATH'] = ''
-        process = subprocess.Popen(
-            shell_command, shell=True, stdin=subprocess.PIPE,
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE,
-            env=new_env)
-        out, err = process.communicate()
+        list_files = subprocess.Popen(
+            list_files_command,
+            stdout=subprocess.PIPE, cwd=root_dir, env=new_env)
+        unique_files = subprocess.Popen(
+            ['sort', '-u'],
+            stdin=list_files.stdout, stdout=subprocess.PIPE)
+        grep = subprocess.Popen(
+            ['xargs', 'grep', '-s', self.CONFLICT_MARKER_RE],
+            stdin=unique_files.stdout, stdout=subprocess.PIPE, cwd=root_dir)
+        out = grep.communicate()[0]
         self.assertFalse(
             len(out), 'Found spurious conflicts marker:\n%s' % out)
 
 
 def test_suite():
     suite = unittest.TestSuite()
-    suite.addTest(unittest.makeSuite(NoSpuriousConlictsMarkerTest))
+    suite.addTest(unittest.makeSuite(NoSpuriousConflictsMarkerTest))
     return suite