Ensure autorelease pool is drained between tests

The testing::Test fixture (used by TEST macro) does not drain the
autorelease pool after a test. PlatformTest should be used.

Add a PRESUBMIT check that neither TEST nor testing::Test is used
in iOS Objective-C++ test files. Files are assumed to be iOS if
either their base name match '\bios\b' or one of the component in
the path is 'ios'.

Expand MockInputApi to filter files in mocks of AffectedFiles and
AffectedSourceFiles function, adding missing mocked functions too.
Fix unit tests that were failing after the filtering is correctly
implemented.

Bug: none
Change-Id: I0af99b6658b8e15888dfcfb94345eb879ab9fd37
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/937204
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539829}

PRESUBMIT.py

@@ -179,6 +179,28 @@
     ),
 )
 
+_BANNED_IOS_OBJC_FUNCTIONS = (
+    (
+      r'/\bTEST[(]',
+      (
+        'TEST() macro should not be used in Objective-C++ code as it does not ',
+        'drain the autorelease pool at the end of the test. Use TEST_F() ',
+        'macro instead with a fixture inheriting from PlatformTest (or a ',
+        'typedef).'
+      ),
+      True,
+    ),
+    (
+      r'/\btesting::Test\b',
+      (
+        'testing::Test should not be used in Objective-C++ code as it does ',
+        'not drain the autorelease pool at the end of the test. Use ',
+        'PlatformTest instead.'
+      ),
+      True,
+    ),
+)
+
 
 _BANNED_CPP_FUNCTIONS = (
     # Make sure that gtest's FRIEND_TEST() macro is not used; the
@@ -786,6 +808,18 @@ def IsBlacklisted(affected_file, blacklist):
         return True
     return False
 
+  def IsIosObcjFile(affected_file):
+    local_path = affected_file.LocalPath()
+    if input_api.os_path.splitext(local_path)[-1] not in ('.mm', '.m', '.h'):
+      return False
+    basename = input_api.os_path.basename(local_path)
+    if 'ios' in basename.split('_'):
+      return True
+    for sep in (input_api.os_path.sep, input_api.os_path.altsep):
+      if sep and 'ios' in local_path.split(sep):
+        return True
+    return False
+
   def CheckForMatch(affected_file, line_num, line, func_name, message, error):
     matched = False
     if func_name[0:1] == '/':
@@ -814,6 +848,11 @@ def CheckForMatch(affected_file, line_num, line, func_name, message, error):
       for func_name, message, error in _BANNED_OBJC_FUNCTIONS:
         CheckForMatch(f, line_num, line, func_name, message, error)
 
+  for f in input_api.AffectedFiles(file_filter=IsIosObcjFile):
+    for line_num, line in f.ChangedContents():
+      for func_name, message, error in _BANNED_IOS_OBJC_FUNCTIONS:
+        CheckForMatch(f, line_num, line, func_name, message, error)
+
   file_filter = lambda f: f.LocalPath().endswith(('.cc', '.mm', '.h'))
   for f in input_api.AffectedFiles(file_filter=file_filter):
     for line_num, line in f.ChangedContents():

PRESUBMIT_test.py

@@ -161,7 +161,7 @@ def testSingletonInArbitraryHeader(self):
     warnings = PRESUBMIT._CheckSingletonInHeaders(mock_input_api,
                                                   MockOutputApi())
     self.assertEqual(1, len(warnings))
-    self.assertEqual(2, len(warnings[0].items))
+    self.assertEqual(1, len(warnings[0].items))
     self.assertEqual('error', warnings[0].type)
     self.assertTrue('Found base::Singleton<T>' in warnings[0].message)
 
@@ -1307,5 +1307,27 @@ def testCheckCrbugLinksHaveHttps(self):
     self.assertEqual(3, warnings[0].message.count('\n'));
 
 
+class BannedFunctionCheckTest(unittest.TestCase):
+
+  def testBannedIosObcjFunctions(self):
+    input_api = MockInputApi()
+    input_api.files = [
+      MockFile('some/ios/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction) {',
+                '}']),
+      MockFile('some/mac/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction) {',
+                '}']),
+      MockFile('another/ios_file.mm',
+               ['class SomeTest : public testing::Test {};']),
+    ]
+
+    errors = PRESUBMIT._CheckNoBannedFunctions(input_api, MockOutputApi())
+    self.assertEqual(1, len(errors))
+    self.assertTrue('some/ios/file.mm' in errors[0].message)
+    self.assertTrue('another/ios_file.mm' in errors[0].message)
+    self.assertTrue('some/mac/file.mm' not in errors[0].message)
+
+
 if __name__ == '__main__':
   unittest.main()

PRESUBMIT_test_mocks.py

@@ -58,6 +58,8 @@ class MockInputApi(object):
   attribute as the list of changed files.
   """
 
+  DEFAULT_BLACK_LIST = ()
+
   def __init__(self):
     self.canned_checks = MockCannedChecks()
     self.fnmatch = fnmatch
@@ -74,10 +76,29 @@ def __init__(self):
     self.presubmit_local_path = os.path.dirname(__file__)
 
   def AffectedFiles(self, file_filter=None, include_deletes=False):
-    return self.files
+    for file in self.files:
+      if file_filter and not file_filter(file):
+        continue
+      if not include_deletes and file.Action() == 'D':
+        continue
+      yield file
 
   def AffectedSourceFiles(self, file_filter=None):
-    return self.files
+    return self.AffectedFiles(file_filter=file_filter)
+
+  def FilterSourceFile(self, file, white_list=(), black_list=()):
+    local_path = file.LocalPath()
+    if white_list:
+      for pattern in white_list:
+        compiled_pattern = re.compile(pattern)
+        if compiled_pattern.search(local_path):
+          return True
+    if black_list:
+      for pattern in black_list:
+        compiled_pattern = re.compile(pattern)
+        if compiled_pattern.search(local_path):
+          return False
+    return True
 
   def LocalPaths(self):
     return self.files