Changeset 90967 in webkit

Timestamp:

Jul 13, 2011 6:03:02 PM (13 years ago)

Author:

commit-queue@webkit.org

Message:

REGRESSION: GitTestWithMock.test_create_patch fails
​https://bugs.webkit.org/show_bug.cgi?id=62945

Reviewed by Daniel Bates.

I was not able to reproduce the exact failure seen in the bug,
however this test was failing on my machine for other reasons.

I went through and did an audit of our run_command usage, it's
entirely in scm classes after this change. (Not surprising given
that scm.py was the second file ever created in webkit.py.)

The real bug I'm fixing here is that we were setting executive.should_log
when the value had changed to executive._should_log. Now we set the right one
and the test works again.

• Scripts/webkitpy/common/checkout/checkout.py:
• Scripts/webkitpy/common/checkout/scm/git.py:
• Scripts/webkitpy/common/checkout/scm/scm.py:
• Scripts/webkitpy/common/checkout/scm/scm_unittest.py:
• Scripts/webkitpy/common/checkout/scm/svn.py:

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/common/checkout/checkout.py (modified) (1 diff)
• Scripts/webkitpy/common/checkout/scm/git.py (modified) (2 diffs)
• Scripts/webkitpy/common/checkout/scm/scm.py (modified) (1 diff)
• Scripts/webkitpy/common/checkout/scm/scm_unittest.py (modified) (2 diffs)
• Scripts/webkitpy/common/checkout/scm/svn.py (modified) (1 diff)

trunk/Tools/ChangeLog

@@ +1 @@
+2011-07-13  Eric Seidel  <eric@webkit.org>
+
+        REGRESSION: GitTestWithMock.test_create_patch fails
+        https://bugs.webkit.org/show_bug.cgi?id=62945
+
+        Reviewed by Daniel Bates.
+
+        I was not able to reproduce the exact failure seen in the bug,
+        however this test was failing on my machine for other reasons.
+
+        I went through and did an audit of our run_command usage, it's
+        entirely in scm classes after this change.  (Not surprising given
+        that scm.py was the second file ever created in webkit.py.)
+
+        The real bug I'm fixing here is that we were setting executive.should_log
+        when the value had changed to executive._should_log.  Now we set the right one
+        and the test works again.
+
+        * Scripts/webkitpy/common/checkout/checkout.py:
+        * Scripts/webkitpy/common/checkout/scm/git.py:
+        * Scripts/webkitpy/common/checkout/scm/scm.py:
+        * Scripts/webkitpy/common/checkout/scm/scm_unittest.py:
+        * Scripts/webkitpy/common/checkout/scm/svn.py:
+
 2011-07-13  Ilya Sherman  <isherman@chromium.org>
 

trunk/Tools/Scripts/webkitpy/common/checkout/checkout.py

@@ -153 +153 @@
         if force:
             args.append('--force')
-        run_command(args, input=patch.contents())
+        self._scm._executive.run_command(args, input=patch.contents())
 
     def apply_reverse_diff(self, revision):

trunk/Tools/Scripts/webkitpy/common/checkout/scm/git.py

@@ -89 +89 @@
     @classmethod
     def in_working_directory(cls, path):
+        # FIXME: This should use an Executive.
         return run_command(['git', 'rev-parse', '--is-inside-work-tree'], cwd=path, error_handler=Executive.ignore_error).rstrip() == "true"
 
@@ -110 +111 @@
         # Pass --get-all for cases where the config has multiple values
         # Pass the cwd if provided so that we can handle the case of running webkit-patch outside of the working directory.
+        # FIXME: This should use an Executive.
         return run_command(["git", "config", "--get-all", key], error_handler=Executive.ignore_error, cwd=cwd).rstrip('\n')
 

trunk/Tools/Scripts/webkitpy/common/checkout/scm/scm.py

@@ -37 +37 @@
 
 from webkitpy.common.system.deprecated_logging import error, log
-from webkitpy.common.system.executive import Executive, run_command, ScriptError
+from webkitpy.common.system.executive import Executive, ScriptError
 from webkitpy.common.system import ospath
 

trunk/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py

@@ -108 +108 @@
     # We use this wrapper to disable output decoding. diffs should be treated as
     # binary files since they may include text files of multiple differnet encodings.
+    # FIXME: This should use an Executive.
     return run_command([command, "diff"] + list(args), decode_output=False)
 
@@ -1485 +1486 @@
 # This class is the first part of that.
 class GitTestWithMock(unittest.TestCase):
-    def setUp(self):
-        executive = MockExecutive(should_log=False)
+    def make_scm(self, logging_executive=False):
         # We do this should_log dance to avoid logging when Git.__init__ runs sysctl on mac to check for 64-bit support.
-        self.scm = Git(None, executive=executive)
-        executive.should_log = True
+        scm = Git(cwd=None, executive=MockExecutive())
+        scm._executive._should_log = logging_executive
+        return scm
 
     def test_create_patch(self):
+        scm = self.make_scm(logging_executive=True)
         expected_stderr = "MOCK run_command: ['git', 'merge-base', u'refs/remotes/origin/master', 'HEAD']\nMOCK run_command: ['git', 'diff', '--binary', '--no-ext-diff', '--full-index', '-M', 'MOCK output of child process', '--']\nMOCK run_command: ['git', 'log', '-25']\n"
-        OutputCapture().assert_outputs(self, self.scm.create_patch, kwargs={'changed_files': None}, expected_stderr=expected_stderr)
+        OutputCapture().assert_outputs(self, scm.create_patch, expected_stderr=expected_stderr)
 
     def test_push_local_commits_to_server_with_username_and_password(self):
-        self.assertEquals(self.scm.push_local_commits_to_server(username='dbates@webkit.org', password='blah'), "MOCK output of child process")
+        self.assertEquals(self.make_scm().push_local_commits_to_server(username='dbates@webkit.org', password='blah'), "MOCK output of child process")
 
     def test_push_local_commits_to_server_without_username_and_password(self):
-        self.assertRaises(AuthenticationError, self.scm.push_local_commits_to_server)
+        self.assertRaises(AuthenticationError, self.make_scm().push_local_commits_to_server)
 
     def test_push_local_commits_to_server_with_username_and_without_password(self):
-        self.assertRaises(AuthenticationError, self.scm.push_local_commits_to_server, {'username': 'dbates@webkit.org'})
+        self.assertRaises(AuthenticationError, self.make_scm().push_local_commits_to_server, {'username': 'dbates@webkit.org'})
 
     def test_push_local_commits_to_server_without_username_and_with_password(self):
-        self.assertRaises(AuthenticationError, self.scm.push_local_commits_to_server, {'password': 'blah'})
+        self.assertRaises(AuthenticationError, self.make_scm().push_local_commits_to_server, {'password': 'blah'})
 
 if __name__ == '__main__':

trunk/Tools/Scripts/webkitpy/common/checkout/scm/svn.py

@@ -93 +93 @@
     def value_from_svn_info(cls, path, field_name):
         svn_info_args = ['svn', 'info', path]
+        # FIXME: This should use an Executive.
         info_output = run_command(svn_info_args).rstrip()
         match = re.search("^%s: (?P<value>.+)$" % field_name, info_output, re.MULTILINE)