From a8b73d2584f8d75ca715eedcd50772cdaab1fbb6 Mon Sep 17 00:00:00 2001 From: Sylvain Defresne Date: Wed, 28 Feb 2018 15:45:54 +0000 Subject: [PATCH] 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 Reviewed-by: Rohit Rao Commit-Queue: Sylvain Defresne Cr-Commit-Position: refs/heads/master@{#539829} --- PRESUBMIT.py | 39 +++++++++++++++++++++++++++++++++++++++ PRESUBMIT_test.py | 24 +++++++++++++++++++++++- PRESUBMIT_test_mocks.py | 25 +++++++++++++++++++++++-- 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 48493fcf789f1c..b5f115dbc99454 100644 --- a/PRESUBMIT.py +++ b/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(): diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index 4b195d8f1f3285..4f33a1d2a3d09b 100755 --- a/PRESUBMIT_test.py +++ b/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' 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() diff --git a/PRESUBMIT_test_mocks.py b/PRESUBMIT_test_mocks.py index e15afcc9101a45..732da0ea38c71a 100644 --- a/PRESUBMIT_test_mocks.py +++ b/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