Bug 143967 – Many test failures in scm_unittest.py

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Michael Catanzaro]

Looks like scm_unittest.py has not been updated in a while. I see many failures:

$ test-webkitpy --all
Suppressing most webkitpy logging while running unit tests.
[47/1550] webkitpy.common.checkout.scm.s..._files_local_plus_working_copy erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1409, in test_changed_files_local_plus_working_copy
      files = self.scm.changed_files('trunk..')
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 223, in changed_files
      return self.run_status_and_extract_filenames(status_command, self._status_regexp("ADM"))
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 88, in run_status_and_extract_filenames
      for line in self.run(status_command, cwd=self.checkout_root).splitlines():
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run
      decode_output=decode_output)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 456, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 374, in default_error_handler
      raise error
  ScriptError: Failed to run "['git', 'diff', '-r', '--name-status', '--no-renames', '--no-ext-diff', '--full-index', 'trunk..', '.']" exit_code: 128 cwd: /tmp/tmpNGh_3sgit_test_checkout
  
[49/1550] webkitpy.common.checkout.scm.s....test_changed_files_not_synced erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1452, in test_changed_files_not_synced
      run_command(['git', 'checkout', '-b', 'my-branch', 'trunk~3'])
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 85, in run_command
      return Executive().run_command(*args, **kwargs)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 456, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 374, in default_error_handler
      raise error
  ScriptError: Failed to run "['git', 'checkout', '-b', 'my-branch', 'trunk~3']" exit_code: 128
  
[56/1550] webkitpy.common.checkout.scm.s...commit_with_message_not_synced erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1273, in test_commit_with_message_not_synced
      run_command(['git', 'checkout', '-b', 'my-branch', 'trunk~3'])
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 85, in run_command
      return Executive().run_command(*args, **kwargs)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 456, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 374, in default_error_handler
      raise error
  ScriptError: Failed to run "['git', 'checkout', '-b', 'my-branch', 'trunk~3']" exit_code: 128
  
[58/1550] webkitpy.common.checkout.scm.s...ssage_not_synced_with_conflict erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1286, in test_commit_with_message_not_synced_with_conflict
      run_command(['git', 'checkout', '-b', 'my-branch', 'trunk~3'])
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 85, in run_command
      return Executive().run_command(*args, **kwargs)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 456, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 374, in default_error_handler
      raise error
  ScriptError: Failed to run "['git', 'checkout', '-b', 'my-branch', 'trunk~3']" exit_code: 128
  
[68/1550] webkitpy.common.checkout.scm.s....test_create_patch_after_merge erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1323, in test_create_patch_after_merge
      run_command(['git', 'checkout', '-b', 'dummy-branch', 'trunk~3'])
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 85, in run_command
      return Executive().run_command(*args, **kwargs)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 456, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 374, in default_error_handler
      raise error
  ScriptError: Failed to run "['git', 'checkout', '-b', 'dummy-branch', 'trunk~3']" exit_code: 128
  
[75/1550] webkitpy.common.checkout.scm.s...t.test_create_patch_not_synced erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1369, in test_create_patch_not_synced
      run_command(['git', 'checkout', '-b', 'my-branch', 'trunk~3'])
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 85, in run_command
      return Executive().run_command(*args, **kwargs)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 456, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 374, in default_error_handler
      raise error
  ScriptError: Failed to run "['git', 'checkout', '-b', 'my-branch', 'trunk~3']" exit_code: 128
  
[93/1550] webkitpy.common.checkout.scm....SVNTest.test_remote_branch_ref failed:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1298, in test_remote_branch_ref
      self.assertEqual(self.scm.remote_branch_ref(), 'refs/remotes/trunk')
  AssertionError: u'refs/remotes/origin/trunk' != 'refs/remotes/trunk'
  
[104/1550] webkitpy.common.checkout.scm....TestWithMock.test_create_patch erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1569, in test_create_patch
      scm = self.make_scm(logging_executive=True)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1563, in make_scm
      scm = Git(cwd=".", executive=MockExecutive(), filesystem=MockFileSystem())
  TypeError: __init__() takes exactly 3 arguments (2 given)
  
[105/1550] webkitpy.common.checkout.scm....ver_with_username_and_password erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1579, in test_push_local_commits_to_server_with_username_and_password
      self.assertEqual(self.make_scm().push_local_commits_to_server(username='[e-mail]', password='blah'), "MOCK output of child process")
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1563, in make_scm
      scm = Git(cwd=".", executive=MockExecutive(), filesystem=MockFileSystem())
  TypeError: __init__() takes exactly 3 arguments (2 given)
  
[106/1550] webkitpy.common.checkout.scm...._username_and_without_password erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1585, in test_push_local_commits_to_server_with_username_and_without_password
      self.assertRaises(AuthenticationError, self.make_scm().push_local_commits_to_server, {'username': '[e-mail]'})
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1563, in make_scm
      scm = Git(cwd=".", executive=MockExecutive(), filesystem=MockFileSystem())
  TypeError: __init__() takes exactly 3 arguments (2 given)
  
[107/1550] webkitpy.common.checkout.scm...._without_username_and_password erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1582, in test_push_local_commits_to_server_without_username_and_password
      self.assertRaises(AuthenticationError, self.make_scm().push_local_commits_to_server)
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1563, in make_scm
      scm = Git(cwd=".", executive=MockExecutive(), filesystem=MockFileSystem())
  TypeError: __init__() takes exactly 3 arguments (2 given)
  
[108/1550] webkitpy.common.checkout.scm....out_username_and_with_password erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1588, in test_push_local_commits_to_server_without_username_and_with_password
      self.assertRaises(AuthenticationError, self.make_scm().push_local_commits_to_server, {'password': 'blah'})
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1563, in make_scm
      scm = Git(cwd=".", executive=MockExecutive(), filesystem=MockFileSystem())
  TypeError: __init__() takes exactly 3 arguments (2 given)
  
[109/1550] webkitpy.common.checkout.scm....ock.test_timestamp_of_revision erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1591, in test_timestamp_of_revision
      scm = self.make_scm()
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1563, in make_scm
      scm = Git(cwd=".", executive=MockExecutive(), filesystem=MockFileSystem())
  TypeError: __init__() takes exactly 3 arguments (2 given)
  
[1017/1550] webkitpy.thirdparty.__init___...t.ThirdpartyTest.test_imports erred:
  Traceback (most recent call last):
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/thirdparty/__init___unittest.py", line 62, in test_imports
      import webkitpy.thirdparty.autoinstalled.coverage
    File "/home/mcatanzaro/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/coverage/__init__.py", line 15, in <module>
      from coverage.control import coverage, process_startup
  ImportError: cannot import name coverage
  
Ran 1550 tests in 13.943s                                                       
FAILED (failures=1, errors=13)

Half of them seem to be that GitTestWithMock.make_scm() passes too few arguments to the Git() constructor. The other half seem to reference a commit named trunk, which has no meaning in git.

This is with git 2.3.5.

[Michael Catanzaro]

Note, there was some work on this done in bug #145511.

[Dean Johnson]

Created attachment 256017 [details]
This is the patch

This patch fixes the broken tests.

[Daniel Bates]

Comment on attachment 256017 [details]
This is the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256017&action=review

I have not finished reviewing this change. The patch is taking a reasonable approach. There are some minor issues I've noticed and I have some questions.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:220
> +        # status_command.extend(self._patch_directories)

We should remove commented out code.

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1274
> +        run_command(['git', 'checkout', 'master~3'])
> +        run_command(['git', 'checkout', '-b', 'my-branch'])

Is this correct? Can we write this using only one call to git checkout?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1288
> +        run_command(['git', 'checkout', 'master~3'])
> +        run_command(['git', 'checkout', '-b', 'my-branch'])

Ditto.

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1300
> +        self.assertEqual(self.scm.remote_branch_ref(), 'refs/remotes/origin/trunk')

Is this correct?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1325
> +        # Check out the correct branch to base dummy-branch from.

Please remove this comment since it explains what the code below does. We consider good comments to be comments that explain "why" the codes does a particular action as opposed to "what" the code does.

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1327
> +        # Create dummy branch and add a commit

Ditto.

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1330
> +        run_command(['git', 'merge', 'master'])

Is this correct? Can we write this using only one call to git checkout?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1375
> +        run_command(['git', 'checkout', 'master~3'])
> +        run_command(['git', 'checkout', '-b', 'my-branch'])

Is this correct? Can we write this using only one call to git checkout?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1459
> +        run_command(['git', 'checkout', 'master~3'])
> +        run_command(['git', 'checkout', '-b', 'my-branch'])

Is this correct? Can we write this using only one call to git checkout?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1468
> +        run_command(['git', 'checkout', 'master~3'])
> +        run_command(['git', 'checkout', '-b', 'my-branch'])

Ditto.

[Dean Johnson]

Created attachment 256028 [details]
Fixes scm unit tests and updated based on Daniel's comments.

As per Daniel and I's in person discussion, the referenced `trunk` does not appear in any of the created git repositories used for tests, and such I believe that `master` is the correct branch to be comparing to in the majority of the tests.

[Dean Johnson]

(In reply to comment #3)
> Comment on attachment 256017 [details]
> This is the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256017&action=review
> 
> I have not finished reviewing this change. The patch is taking a reasonable
> approach. There are some minor issues I've noticed and I have some questions.
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:220
> > +        # status_command.extend(self._patch_directories)
> 
> We should remove commented out code.
> 
Removed.

> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1274
> > +        run_command(['git', 'checkout', 'master~3'])
> > +        run_command(['git', 'checkout', '-b', 'my-branch'])
> 
> Is this correct? Can we write this using only one call to git checkout?
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1288
> > +        run_command(['git', 'checkout', 'master~3'])
> > +        run_command(['git', 'checkout', '-b', 'my-branch'])
> 
> Ditto.
> 
Fixed.

> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1300
> > +        self.assertEqual(self.scm.remote_branch_ref(), 'refs/remotes/origin/trunk')
> 
> Is this correct?
> 
In the setup function for this test suite we clone the trunk branch from the svn repo using git-svn and put it in the git checkout path which is just a temporary path made for tests. My assumption is that git-svn or git itself has changed slightly since the code was originally written to include the remote in the branch reference, which is why origin/ had to be added.
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1325
> > +        # Check out the correct branch to base dummy-branch from.
> 
> Please remove this comment since it explains what the code below does. We
> consider good comments to be comments that explain "why" the codes does a
> particular action as opposed to "what" the code does.
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1327
> > +        # Create dummy branch and add a commit
> 
> Ditto.
> 
Removed.
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1330
> > +        run_command(['git', 'merge', 'master'])
> 
> Is this correct? Can we write this using only one call to git checkout?
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1375
> > +        run_command(['git', 'checkout', 'master~3'])
> > +        run_command(['git', 'checkout', '-b', 'my-branch'])
> 
> Is this correct? Can we write this using only one call to git checkout?
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1459
> > +        run_command(['git', 'checkout', 'master~3'])
> > +        run_command(['git', 'checkout', '-b', 'my-branch'])
> 
> Is this correct? Can we write this using only one call to git checkout?
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1468
> > +        run_command(['git', 'checkout', 'master~3'])
> > +        run_command(['git', 'checkout', '-b', 'my-branch'])
> 
> Ditto.
Combined all pairs of `git checkout` commands into a single `git checkout`

[Michael Catanzaro]

Thanks for fixing this. And now I've learned that you can use a single 'git checkout' to switch to a commit and create a new branch with it at the same time; that's new to me too.

[Michael Catanzaro]

Probably also we should run this test by default, or it will just break again.  'test-webkitpy --all' takes 15 seconds on my machine, so I don't see any point in skipping "slow" tests like this when --all is not passed.

[Dean Johnson]

This patch is in conflict with https://bugs.webkit.org/show_bug.cgi?id=137166 and such until we figure out what #!37116 solved exactly I do not think this patch should be landed, even if it's +1'd for code. Removing from commit queue.

[Dean Johnson]

Created attachment 256243 [details]
Added back patch_directories

This adds back the previously affected patch_directories code and fixes the underlying issues that were causing git to complain.

[Michael Catanzaro]

A couple very minor nits from me. Normally we wrap changelogs at 100 characters, so you don't have to scroll way to the right to read the changelog entry. Also, the comment about /private/var/tmp seems to be Mac-specific; I've never heard of /private before, and mkdtemp() normally returns a path under /tmp not /var/tmp on my machine.

[Dean Johnson]

(In reply to comment #10)
> A couple very minor nits from me. Normally we wrap changelogs at 100
> characters, so you don't have to scroll way to the right to read the
> changelog entry.
Good to know, thanks for that info :)

> Also, the comment about /private/var/tmp seems to be
> Mac-specific; I've never heard of /private before, and mkdtemp() normally
> returns a path under /tmp not /var/tmp on my machine.
You're right, I should have noted that this fix is primarily for tests being run on Mac systems.

After some review, I've still got a few more changes and assumptions to verify before it's ready for review again, so I'll update my new patch with your feedback then. Thanks!

[Daniel Bates]

Comment on attachment 256243 [details]
Added back patch_directories

Clearing review and commit queue flags as further investigation is required per comment 11 .

[Csaba Osztrogonác]

Comment on attachment 256243 [details]
Added back patch_directories

View in context: https://bugs.webkit.org/attachment.cgi?id=256243&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1302
> -        self.assertEqual(self.scm.remote_branch_ref(), 'refs/remotes/trunk')
> +        self.assertEqual(self.scm.remote_branch_ref(), 'refs/remotes/origin/trunk')

It would break the test on Linux:

[104/1563] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_remote_branch_ref failed:
  Traceback (most recent call last):
    File "/home/webkit/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1302, in test_remote_branch_ref
      self.assertEqual(self.scm.remote_branch_ref(), 'refs/remotes/origin/trunk')
  AssertionError: u'refs/remotes/trunk' != 'refs/remotes/origin/trunk'

[Csaba Osztrogonác]

(In reply to comment #7)
> Probably also we should run this test by default, or it will just break
> again.  'test-webkitpy --all' takes 15 seconds on my machine, so I don't see
> any point in skipping "slow" tests like this when --all is not passed.

It takes 5 seconds without all and 30 seconds with --all on a quad core machine.
I agree, we can enable it (at least on the bots) once tests pass on Linux, Mac
and Windows too.

[Dean Johnson]

Created attachment 256491 [details]
Added fixes for issues from comments and changed one incorrect test case.

The tests originally written in GitSvnTest.test_changed_files_local_plus_working_copy don't seem to be consistent with other test cases written by the author in the same commit, so they've been edited to be correct and now pass.

[WebKit Commit Bot]

Attachment 256491 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1038:  [GitSVNTest._setup_git_checkout] Instance of 'GitSVNTest' has no 'svn_repo_url' member  [pylint/E1101] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Johnson]

(In reply to comment #16)
> Attachment 256491 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1038: 
> [GitSVNTest._setup_git_checkout] Instance of 'GitSVNTest' has no
> 'svn_repo_url' member  [pylint/E1101] [5]
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Bug filed: https://bugs.webkit.org/show_bug.cgi?id=146797

svn_repo_url is a member of SVNTestRepository, which we use in the GitSVNTest setup.

[Michael Catanzaro]

Your changelog entry is messed up!

[Dean Johnson]

Comment on attachment 256491 [details]
Added fixes for issues from comments and changed one incorrect test case.

Oops. Fixed.

[Dean Johnson]

Comment on attachment 256491 [details]
Added fixes for issues from comments and changed one incorrect test case.

From 588babf59986d51aa0b0c4ea23f17c13725b6ff5 Mon Sep 17 00:00:00 2001
From: Dean Johnson <[e-mail]>
Date: Thu, 9 Jul 2015 14:09:17 -0700
Subject: [PATCH] Fixes scm unit tests.

---
 Tools/ChangeLog                                          | 15 +++++++++++++++
 Tools/Scripts/webkitpy/common/checkout/scm/detection.py  | 10 +++++-----
 Tools/Scripts/webkitpy/common/checkout/scm/git.py        |  2 +-
 .../Scripts/webkitpy/common/checkout/scm/scm_unittest.py | 16 +++++++++++++---
 4 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
index 8a33cac0..2368627 100644
--- a/Tools/ChangeLog
+++ b/Tools/ChangeLog
@@ -1,3 +1,18 @@
[phone]  Dean Johnson  <[e-mail]>
+
+        Fixed scm unit tests.
+        https://bugs.webkit.org/show_bug.cgi?id=143967
+
+        Reviewed by NOBODY (OOPS!).
+
+        * Scripts/webkitpy/common/checkout/scm/detection.py:
+        (SCMDetector.detect_scm_system): Now use realpaths so all paths are consistent when being compared.
+        * Scripts/webkitpy/common/checkout/scm/git.py:
+        (Git.changed_files): Fixed `git` invocation syntactical errors. ('-r' replaced with '--raw' and added '--' to escape patch_directories as arguments)
+        * Scripts/webkitpy/common/checkout/scm/scm_unittest.py: Changed tests and setup in the GitSVNTest class to mroe closely emulate old git..
+        (GitSVNTest._setup_git_checkout): Added --prefix option to `git clone` and renamed `master` branch to `trunk`.
+        (GitSVNTest.test_changed_files_local_plus_working_copy): Changed `trunk` to master and changed how git was referenced so the expected value match the return. Previously the test would fail due to an error of which commits needed to be compared and what was expected.
+
 2015-06-29  Jake Nielsen  <[e-mail]>
 
         Add timeout in run-webkit-tests for launching the simulator
diff --git a/Tools/Scripts/webkitpy/common/checkout/scm/detection.py b/Tools/Scripts/webkitpy/common/checkout/scm/detection.py
index 87a7a0d..8c59005 100644
--- a/Tools/Scripts/webkitpy/common/checkout/scm/detection.py
+++ b/Tools/Scripts/webkitpy/common/checkout/scm/detection.py
@@ -63,16 +63,16 @@ class SCMDetector(object):
         return scm_system
 
     def detect_scm_system(self, path, patch_directories=None):
-        absolute_path = self._filesystem.abspath(path)
+        real_path = self._filesystem.realpath(path)
 
         if patch_directories == []:
             patch_directories = None
 
-        if SVN.in_working_directory(absolute_path, executive=self._executive):
-            return SVN(cwd=absolute_path, patch_directories=patch_directories, filesystem=self._filesystem, executive=self._executive)
+        if SVN.in_working_directory(real_path, executive=self._executive):
+            return SVN(cwd=real_path, patch_directories=patch_directories, filesystem=self._filesystem, executive=self._executive)
 
-        if Git.in_working_directory(absolute_path, executive=self._executive):
-            return Git(cwd=absolute_path, patch_directories=patch_directories, filesystem=self._filesystem, executive=self._executive)
+        if Git.in_working_directory(real_path, executive=self._executive):
+            return Git(cwd=real_path, patch_directories=patch_directories, filesystem=self._filesystem, executive=self._executive)
 
         return None
 
diff --git a/Tools/Scripts/webkitpy/common/checkout/scm/git.py b/Tools/Scripts/webkitpy/common/checkout/scm/git.py
index aad6ead..2be683d 100644
--- a/Tools/Scripts/webkitpy/common/checkout/scm/git.py
+++ b/Tools/Scripts/webkitpy/common/checkout/scm/git.py
@@ -216,7 +216,7 @@ class Git(SCM, SVNRepository):
 
     def changed_files(self, git_commit=None):
         # FIXME: --diff-filter could be used to avoid the "extract_filenames" step.
-        status_command = [self.executable_name, 'diff', '-r', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit)]
+        status_command = [self.executable_name, 'diff', '--raw', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit), '--']
         status_command.extend(self._patch_directories)
         # FIXME: I'm not sure we're returning the same set of files that SVN.changed_files is.
         # Added (A), Copied (C), Deleted (D), Modified (M), Renamed (R)
diff --git a/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py b/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py
index d772d79..484c35c 100644
--- a/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py
+++ b/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py
@@ -1035,8 +1035,9 @@ class GitSVNTest(SCMTest):
     def _setup_git_checkout(self):
         self.git_checkout_path = tempfile.mkdtemp(suffix="git_test_checkout")
         # --quiet doesn't make git svn silent, so we use run_silent to redirect output
-        run_silent(['git', 'svn', 'clone', '-T', 'trunk', self.svn_repo_url, self.git_checkout_path])
+        run_silent(['git', 'svn', 'clone', '-T', 'trunk', '--prefix', '', self.svn_repo_url, self.git_checkout_path])
         os.chdir(self.git_checkout_path)
+        run_silent(['git', 'branch', '-m', 'trunk'])
 
     def _tear_down_git_checkout(self):
         # Change back to a valid directory so that later calls to os.getcwd() do not fail.
@@ -1406,12 +1407,21 @@ class GitSVNTest(SCMTest):
         self.assertIn('test_file_commit2', files)
 
         # working copy should *not* be in the list.
-        files = self.scm.changed_files('trunk..')
+        files = self.scm.changed_files('trunk..')  # git diff trunk..HEAD
+        self.assertNotIn('test_file_commit1', files)
+        self.assertNotIn('test_file_commit2', files)
+
+        files = self.scm.changed_files('trunk^..')  # git diff trunk^..HEAD
         self.assertIn('test_file_commit1', files)
         self.assertNotIn('test_file_commit2', files)
 
         # working copy *should* be in the list.
-        files = self.scm.changed_files('trunk....')
+        # .... is a hack for working copy changes to be included
+        files = self.scm.changed_files('trunk....')  # git diff trunk
+        self.assertNotIn('test_file_commit1', files)
+        self.assertIn('test_file_commit2', files)
+
+        files = self.scm.changed_files('trunk^....')  # git diff trunk^
         self.assertIn('test_file_commit1', files)
         self.assertIn('test_file_commit2', files)
 
-- 
2.3.7 (Apple Git-57)

[Dean Johnson]

Created attachment 256518 [details]
Fixed changelog from previous patch.

Sorry about the spam, I'm still learning the system.

[WebKit Commit Bot]

Attachment 256518 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1038:  [GitSVNTest._setup_git_checkout] Instance of 'GitSVNTest' has no 'svn_repo_url' member  [pylint/E1101] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

I don't pretend to understand the changes, but I confirm that all tests pass with this patch. Without this patch I have one failure and six errors.

[Dean Johnson]

If you have any questions I'd be more than happy to answer them. :)

[Daniel Bates]

Comment on attachment 256518 [details]
Fixed changelog from previous patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=256518&action=review

> Tools/ChangeLog:3
> +        Fixed scm unit tests.

This line should be the bug title, "Many test failures in scm_unittest.py" (*).  An optional bug description should be written after the Reviewed By line. It is unwritten convention to omit a description if the bug title is sufficiently descriptive so as to explain the purpose of the patch. With regards to this patch, the bug title seems sufficiently descriptive. Unless you would like to elaborate further, I suggest we omit the description "Fixed scm unit tests."

One example of a well-formed ChangeLog entry can be seen in changeset r186592, <http://trac.webkit.org/changeset/186592>.

> Tools/ChangeLog:9
> +        (SCMDetector.detect_scm_system): Now use realpaths so all paths are consistent when being compared.

This line does not read well. Moreover, the phrase "consistent when being compared" is vague as it neither explains what we are making the path(s) consistent with nor what the path(s) are being compared against. We should explain why we need to use FileSystem.realpath() instead of FileSystem.abspath() to resolve symbolic links. In particular, we should explain why some of the tests (you may want to consider naming one such test) failed on Mac because we did not resolve symbolic links in the checkout path.

> Tools/ChangeLog:11
> +        (Git.changed_files): Fixed `git` invocation syntactical errors. ('-r' replaced with '--raw' and added '--' to escape patch_directories as arguments)

The first sentence in this remark implies that syntax was incorrect for all versions of Git. Notice that the original syntax (before your proposed change) was valid at least up to Git version 1.4.4.5. Also, -r is not equivalent to --raw. According to (2) of section "Limiting the diff output" of the git diff documentation for Git version 1.4.4.5 (*), the command line argument -r was used to recursively perform a diff when given a sub directory. Looking at the same section in the documentation for git diff in Git versions > 1.4.4.5 it seems to imply that git diff performs a recursive diff by default; => we can remove the -r command line option.

With regards to the -- delimiter, that was added to git diff in Git version > 1.4.4.5.

(*) <https://git-scm.com/docs/git-diff/1.4.4.5>

> Tools/ChangeLog:12
> +        * Scripts/webkitpy/common/checkout/scm/scm_unittest.py: Changed tests and setup in the GitSVNTest class to mroe closely emulate old git..

mroe => more

> Tools/ChangeLog:13
> +        (GitSVNTest._setup_git_checkout): Added --prefix option to `git clone` and renamed `master` branch to `trunk`.

For your consideration, I suggest we add a remark that explains the versions of Git that require explicitly specifying --prefix to be the empty string. According to <http://git-scm.com/docs/git-svn>, "Before Git v2.0, the default prefix was '' (no prefix)".

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:219
> +        status_command = [self.executable_name, 'diff', '--raw', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit), '--']

As remarked above, -r is not equivalent to --raw. The command line argument -r was used to perform a recursive diff in Git version <= 1.4.4.5. We may be able to omit the -r argument. See my remark above for more details. Alternatively, we could switch to using git diff-tree, which accepts a -r argument and does not require using a path delimiter (--).

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1040
> +        run_silent(['git', 'svn', 'clone', '-T', 'trunk', '--prefix', '', self.svn_repo_url, self.git_checkout_path])
>          os.chdir(self.git_checkout_path)
> +        run_silent(['git', 'branch', '-m', 'trunk'])

This change is OK as-is. I wish we could track down when this behavior changed in Git.

[Dean Johnson]

Created attachment 256742 [details]
Final changes and fixes

Updated the ChangeLog to be much more informative about what and why decisions were made.

[Daniel Bates]

Comment on attachment 256742 [details]
Final changes and fixes

Clearing cq flag as Dean has an updated patch.

[Dean Johnson]

Created attachment 256771 [details]
Patch

[Dean Johnson]

Created attachment 256780 [details]
Patch

[WebKit Commit Bot]

Attachment 256780 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1038:  [GitSVNTest._setup_git_checkout] Instance of 'GitSVNTest' has no 'svn_repo_url' member  [pylint/E1101] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Johnson]

Created attachment 256781 [details]
Patch

[WebKit Commit Bot]

Attachment 256781 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1038:  [GitSVNTest._setup_git_checkout] Instance of 'GitSVNTest' has no 'svn_repo_url' member  [pylint/E1101] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 256781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256781&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:220
> +        status_command = [self.executable_name, 'diff', '-r', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit), '--']
>          status_command.extend(self._patch_directories)

This is OK as-is. As far as I can tell it is acceptable to pass nothing after the '--'. The first mention of the '--' delimiter that I found was in <https://git-scm.com/docs/git-diff/1.5.0>. We may want to consider only emitting a "--" delimiter if self._patch_directories is a non-empty list.

[WebKit Commit Bot]

Comment on attachment 256781 [details]
Patch

Clearing flags on attachment: 256781

Committed r186870: <http://trac.webkit.org/changeset/186870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.