Revert of Include isolate.py in data for Android unit tests (patchset #5

id:80001 of https://codereview.chromium.org/1784373002/ ) Reason for revert: PRESUBMIT isn't working for non-android checkouts, as shown by: https://codereview.chromium.org/1840913002/diff/1/build/android/test_runner.pydeps#oldcode138 Original issue's description: > Include isolate.py in data for Android unit tests > > This is required for any test that uses an .isolate to push files to the > device (e.g. base_unittests). > > BUG=589318 > > Committed: https://crrev.com/9e299ea6bbf87415d64d9d84003162d71304ccb9 > Cr-Commit-Position: refs/heads/master@{#383127} TBR=jochen@chromium.org,jbudorick@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=589318 Review URL: https://codereview.chromium.org/1839103002 Cr-Commit-Position: refs/heads/master@{#383733}
FairyWorld · Mar 29, 2016 · e33676c · e33676c
1 parent 52d96b4
commit e33676c
Show file tree

Hide file tree

Showing 8 changed files with 29 additions and 577 deletions.
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
@@ -298,12 +298,6 @@
 )
 
 
-_PYDEPS_FILES = (
-    'build/android/test_runner.pydeps',
-    'build/secondary/tools/swarming_client/isolate.pydeps',
-)
-
-
 def _CheckNoProductionCodeUsingTestOnlyFunctions(input_api, output_api):
   """Attempts to prevent use of functions intended only for testing in
   non-testing code. For now this is just a best-effort implementation
@@ -1504,104 +1498,6 @@ def _CheckAndroidNewMdpiAssetLocation(input_api, output_api):
   return results
 
 
-class PydepsChecker(object):
-  def __init__(self, input_api):
-    self._file_cache = {}
-    self._input_api = input_api
-
-  def _LoadFile(self, path):
-    """Returns the list of paths within a .pydeps file relative to //."""
-    if path not in self._file_cache:
-      with open(path) as f:
-        self._file_cache[path] = f.read()
-    return self._file_cache[path]
-
-  def _ComputeNormalizedPydepsEntries(self, pydeps_path):
-    """Returns an interable of paths within the .pydep, relativized to //."""
-    os_path = self._input_api.os_path
-    pydeps_dir = os_path.dirname(pydeps_path)
-    entries = (l.rstrip() for l in self._LoadFile(pydeps_path).splitlines()
-               if not l.startswith('*'))
-    return (os_path.normpath(os_path.join(pydeps_dir, e)) for e in entries)
-
-  def _CreateFilesToPydepsMap(self):
-    """Returns a map of local_path -> list_of_pydeps."""
-    ret = {}
-    for pydep_local_path in _PYDEPS_FILES:
-      for path in self._ComputeNormalizedPydepsEntries(pydep_local_path):
-        ret.setdefault(path, []).append(pydep_local_path)
-    return ret
-
-  def ComputeAffectedPydeps(self):
-    """Returns an iterable of .pydeps files that might need regenerating."""
-    affected_pydeps = set()
-    file_to_pydeps_map = None
-    for f in self._input_api.AffectedFiles(include_deletes=True):
-      local_path = f.LocalPath()
-      if local_path  == 'DEPS':
-        return _PYDEPS_FILES
-      elif local_path.endswith('.pydeps'):
-        if local_path in _PYDEPS_FILES:
-          affected_pydeps.add(local_path)
-      elif local_path.endswith('.py'):
-        if file_to_pydeps_map is None:
-          file_to_pydeps_map = self._CreateFilesToPydepsMap()
-        affected_pydeps.update(file_to_pydeps_map.get(local_path, ()))
-    return affected_pydeps
-
-  def DetermineIfStale(self, pydeps_path):
-    """Runs print_python_deps.py to see if the files is stale."""
-    old_pydeps_data = self._LoadFile(pydeps_path)
-
-    m = self._input_api.re.search(r'# target: //(.*)', old_pydeps_data)
-    if not m:
-      return ['COULD NOT FIND .pydeps TARGET']
-    target = m.group(1)
-    m = self._input_api.re.search(r'# root: //(.*)', old_pydeps_data)
-    if not m:
-      return ['COULD NOT FIND .pydeps ROOT']
-    root = m.group(1) or '.'
-
-    cmd = ['build/print_python_deps.py', '--root', root, target]
-    new_pydeps_data = self._input_api.subprocess.check_output(cmd)
-    if old_pydeps_data != new_pydeps_data:
-      return cmd[:-1] + ['--output', pydeps_path] + cmd[-1:]
-
-
-def _CheckPydepsNeedsUpdating(input_api, output_api, checker_for_tests=None):
-  """Checks if a .pydeps file needs to be regenerated."""
-  results = []
-  # First, check for new / deleted .pydeps.
-  for f in input_api.AffectedFiles(include_deletes=True):
-    if f.LocalPath().endswith('.pydeps'):
-      if f.Action() == 'D' and f.LocalPath() in _PYDEPS_FILES:
-        results.append(output_api.PresubmitError(
-            'Please update _PYDEPS_FILES within //PRESUBMIT.py to remove %s' %
-            f.LocalPath()))
-      elif f.Action() != 'D' and f.LocalPath() not in _PYDEPS_FILES:
-        results.append(output_api.PresubmitError(
-            'Please update _PYDEPS_FILES within //PRESUBMIT.py to include %s' %
-            f.LocalPath()))
-
-  if results:
-    return results
-
-  checker = checker_for_tests or PydepsChecker(input_api)
-
-  for pydep_path in checker.ComputeAffectedPydeps():
-    try:
-      cmd = checker.DetermineIfStale(pydep_path)
-      if cmd:
-        results.append(output_api.PresubmitError(
-            'File is stale: %s\nTo regenerate, run:\n%s' % (pydep_path,
-                                                            ' '.join(cmd))))
-    except input_api.subprocess.CalledProcessError as error:
-      return [output_api.PresubmitError('Error running ' + ' '.join(error.cmd),
-          long_text=error.output)]
-
-  return results
-
-
 def _CheckForCopyrightedCode(input_api, output_api):
   """Verifies that newly added code doesn't contain copyrighted material
   and is properly licensed under the standard Chromium license.
@@ -1755,7 +1651,6 @@ def _AndroidSpecificOnUploadChecks(input_api, output_api):
   results.extend(_CheckAndroidCrLogUsage(input_api, output_api))
   results.extend(_CheckAndroidNewMdpiAssetLocation(input_api, output_api))
   results.extend(_CheckAndroidToastUsage(input_api, output_api))
-  results.extend(_CheckPydepsNeedsUpdating(input_api, output_api))
   return results
 
 

diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
@@ -3,8 +3,12 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import glob
+import json
+import os
 import re
 import subprocess
+import sys
 import unittest
 
 import PRESUBMIT
@@ -832,102 +836,6 @@ def testUserMetricsActionNotAddedToActions(self):
       output[0].message)
 
 
-class PydepsNeedsUpdatingTest(unittest.TestCase):
-
-  class MockSubprocess(object):
-    CalledProcessError = subprocess.CalledProcessError
-
-  def setUp(self):
-    self.old_PYDEPS_FILES = PRESUBMIT._PYDEPS_FILES
-    PRESUBMIT._PYDEPS_FILES = ['A.pydeps', 'B.pydeps']
-    self.mock_input_api = MockInputApi()
-    self.mock_output_api = MockOutputApi()
-    self.mock_input_api.subprocess = PydepsNeedsUpdatingTest.MockSubprocess()
-    self.checker = PRESUBMIT.PydepsChecker(self.mock_input_api)
-    self.checker._file_cache = {
-        'A.pydeps': '# target: //A.py\n# root: //\nA.py\nC.py\n',
-        'B.pydeps': '# target: //B.py\n# root: //\nB.py\nC.py\n',
-    }
-
-  def tearDown(self):
-    PRESUBMIT._PYDEPS_FILES = self.old_PYDEPS_FILES
-
-  def _RunCheck(self):
-    return PRESUBMIT._CheckPydepsNeedsUpdating(self.mock_input_api,
-                                               self.mock_output_api,
-                                               checker_for_tests=self.checker)
-
-  def testAddedPydep(self):
-    self.mock_input_api.files = [
-      MockAffectedFile('new.pydeps', [], action='A'),
-    ]
-
-    results = self._RunCheck()
-    self.assertEqual(1, len(results))
-    self.assertTrue('PYDEPS_FILES' in str(results[0]))
-
-  def testRemovedPydep(self):
-    self.mock_input_api.files = [
-      MockAffectedFile(PRESUBMIT._PYDEPS_FILES[0], [], action='D'),
-    ]
-
-    results = self._RunCheck()
-    self.assertEqual(1, len(results))
-    self.assertTrue('PYDEPS_FILES' in str(results[0]))
-
-  def testRandomPyIgnored(self):
-    self.mock_input_api.files = [
-      MockAffectedFile('random.py', []),
-    ]
-
-    results = self._RunCheck()
-    self.assertEqual(0, len(results), 'Unexpected results: %r' % results)
-
-  def testRelevantPyNoChange(self):
-    self.mock_input_api.files = [
-      MockAffectedFile('A.py', []),
-    ]
-
-    def mock_check_output(cmd):
-      self.assertEqual('A.py', cmd[3])
-      return self.checker._file_cache['A.pydeps']
-
-    self.mock_input_api.subprocess.check_output = mock_check_output
-
-    results = self._RunCheck()
-    self.assertEqual(0, len(results), 'Unexpected results: %r' % results)
-
-  def testRelevantPyOneChange(self):
-    self.mock_input_api.files = [
-      MockAffectedFile('A.py', []),
-    ]
-
-    def mock_check_output(cmd):
-      self.assertEqual('A.py', cmd[3])
-      return 'changed data'
-
-    self.mock_input_api.subprocess.check_output = mock_check_output
-
-    results = self._RunCheck()
-    self.assertEqual(1, len(results))
-    self.assertTrue('File is stale' in str(results[0]))
-
-  def testRelevantPyTwoChanges(self):
-    self.mock_input_api.files = [
-      MockAffectedFile('C.py', []),
-    ]
-
-    def mock_check_output(cmd):
-      return 'changed data'
-
-    self.mock_input_api.subprocess.check_output = mock_check_output
-
-    results = self._RunCheck()
-    self.assertEqual(2, len(results))
-    self.assertTrue('File is stale' in str(results[0]))
-    self.assertTrue('File is stale' in str(results[1]))
-
-
 class LogUsageTest(unittest.TestCase):
 
   def testCheckAndroidCrLogUsage(self):

diff --git a/PRESUBMIT_test_mocks.py b/PRESUBMIT_test_mocks.py
@@ -26,7 +26,7 @@ def __init__(self):
     self.is_committing = False
     self.change = MockChange([])
 
-  def AffectedFiles(self, file_filter=None, include_deletes=False):
+  def AffectedFiles(self, file_filter=None):
     return self.files
 
   def AffectedSourceFiles(self, file_filter=None):
@@ -92,14 +92,13 @@ class MockFile(object):
   MockInputApi for presubmit unittests.
   """
 
-  def __init__(self, local_path, new_contents, action='A'):
+  def __init__(self, local_path, new_contents):
     self._local_path = local_path
     self._new_contents = new_contents
     self._changed_contents = [(i + 1, l) for i, l in enumerate(new_contents)]
-    self._action = action
 
   def Action(self):
-    return self._action
+    return 'A'  # TODO(dbeam): feel free to change if your test actually uses.
 
   def ChangedContents(self):
     return self._changed_contents

diff --git a/build/android/BUILD.gn b/build/android/BUILD.gn
@@ -84,25 +84,30 @@ action("cpplib_stripped") {
   ]
 }
 
+group("devil_py") {
+  data = [
+    "devil_chromium.json",
+    "devil_chromium.py",
+    "//third_party/android_tools/sdk/build-tools/23.0.1/aapt",
+    "//third_party/android_tools/sdk/build-tools/23.0.1/dexdump",
+    "//third_party/android_tools/sdk/build-tools/23.0.1/lib/libc++.so",
+    "//third_party/android_tools/sdk/build-tools/23.0.1/split-select",
+    "//third_party/android_tools/sdk/platform-tools/adb",
+    "//third_party/catapult/catapult_base/catapult_base/",
+    "//third_party/catapult/dependency_manager/dependency_manager/",
+    "//third_party/catapult/third_party/gsutil/",
+    "//third_party/catapult/devil/devil/",
+  ]
+}
+
 group("test_runner_py") {
-  _py_files = read_file("test_runner.pydeps", "list lines")
-
-  # Filter out comments.
-  set_sources_assignment_filter([ "#*" ])
-  sources = _py_files
-
-  data = sources + [
-           "devil_chromium.json",
-           "//third_party/android_tools/sdk/build-tools/23.0.1/aapt",
-           "//third_party/android_tools/sdk/build-tools/23.0.1/dexdump",
-           "//third_party/android_tools/sdk/build-tools/23.0.1/lib/libc++.so",
-           "//third_party/android_tools/sdk/build-tools/23.0.1/split-select",
-           "//third_party/android_tools/sdk/platform-tools/adb",
-           "//third_party/catapult/third_party/gsutil/",
-           "//third_party/catapult/devil/devil/devil_dependencies.json",
-         ]
+  data = [
+    "test_runner.py",
+    "pylib/",
+    "//build/util/lib/common/",
+  ]
   data_deps = [
-    "//tools/swarming_client:isolate_py",
+    ":devil_py",
   ]
 }