[Findit] Factoring the gitiles (etc) stuff out into its own directory

appengine/findit/common/chromium_deps.py

Index: appengine/findit/common/chromium_deps.py
diff --git a/appengine/findit/common/chromium_deps.py b/appengine/findit/common/chromium_deps.py
index 39c63472fa5e4b9e3d052d27103ea0f746e9eb9d..5dbd21a1f1e2caa221df74d1c93cbfaeb1fde320 100644
--- a/appengine/findit/common/chromium_deps.py
+++ b/appengine/findit/common/chromium_deps.py
@@ -4,9 +4,17 @@
 
 import re
 
+# N.B., in order for our mocking framework to work correctly, we *must not*
+# import the GitRepository class directly; instead, we must only import
+# the git_repository module and then rely on dynamic dispatch to resolve
+# the module's GitRepository attribute. Otherwise the DEPSDownloader.Load
+# method will hold a reference directly to the real GitRepository class,
+# which means we can't swap it out for the DummyGitRepository class, which
+# in turn will cause some unit tests to fail (since we can't mock the
+# responses of doing SSL stuff).

[stgao, 2016/09/21 23:03:24]

BTW, can we remove this comment? For functional code, we keep comments for the
functionality instead of for testing.

[stgao, 2016/09/21 23:03:24]

This sgtm in general. But it is possible to mock and test in even we import
GitRepository directly here.

What we need to change to the test is:

self.mock(git_repository, 'GitRepository', DummyGitRepository)
      |
self.mock(chromium_deps, 'GitRepository', DummyGitRepository)

[wrengr, 2016/09/28 00:59:49]

I appreciate the desire to remove the comment, but it took a long time to track
down the bug and it's the sort of bug that's liable to show up again if not
noted. Personally I think the mock tests should be done differently so that it's
not so counterintuitive and hard to debug; they should be properly passing
objects around rather than monkey patching classes out from under the functional
code, imo.

[stgao, 2016/10/13 05:47:31]

Then let's keep it. Thanks for your time figuring it out!
I didn't know it cause a big pain on your end.

Passing object sounds a good idea. We will see how the test could be improved.
I'd like to clean up the test with better mocking or other solution too.
A short while ago, I just started to introduce unittest.mock instead of always
depending on the self.mock(module_name, "attribute_name_in_module",
mocked_attribute) provided by the testing framework of the infra repo.

+from gitiles import git_repository

[stgao, 2016/09/21 23:03:24]

nit: import order.

 from common import dependency
 from common import deps_parser
-from common import git_repository
 from common import http_client_appengine
 
 
@@ -30,6 +38,9 @@ def IsChromeVersion(revision):
 class DEPSDownloader(deps_parser.DEPSLoader):
   """Downloads DEPS from remote Git repo."""
 
+  # TODO(wrengr): why do we allocate a new GitRepository every time
+  # we call Load, rather than just doing it once and saving it in the
+  # DEPSDownloader class (via RAII)?

[stgao, 2016/09/21 23:03:24]

Because a GitRepository is defined by the repo_url here.

I'm fine with moving it with RAII and creating a new DEPSDownloader each time
one is needed, although I don't see much benefit in that way.

[wrengr, 2016/09/28 00:59:49]

We could just as well (a) pass repo_url to the DEPSDownloader constructor and
drop the argument to the Load method, or (b) pass the GitRepository object to
the Load method in lieu of passing the string. The choice depends on who
conceptually "owns" the GitRepository object: either the DEPSDownloader itself,
or someone else who wants to share the same object with other classes as well.
The benefit is code clarity. Right now the DEPSDownloader class isn't doing
anything at all; so having it there just makes things more complicated. I don't
know what sort of state HttpClientAppengine is carrying, but if the
GitRepository and its HttpClientAppengine can be reused by all the calls to Load
for that repo_url, then it makes sense to cache those objects when constructing
DEPSDownloader. If such reuse doesn't make sense (due to whatever
HttpClientAppengine is up to), then we should get rid of the DEPSDownloader
class and make Load a standalone function (or perhaps a method on GitRepository
or some subclass of GitRepository).

   def Load(self, repo_url, revision, deps_file):
     http_client = http_client_appengine.HttpClientAppengine()
     repo = git_repository.GitRepository(repo_url, http_client)