From 9949ab7a4b1cb3c342b38129b4e0bfcfb2ef5749 Mon Sep 17 00:00:00 2001 From: Saagar Sanghavi Date: Mon, 20 Jul 2020 20:56:40 +0000 Subject: [PATCH] Preliminary changes to git cl and presubmit_support Added an rdb wrapper and use it to time CheckChangeOn{Commit, Upload} from every PRESUBMIT.py file, then reports the results to ResultSink. Bug: http://crbug.com/1107588 Change-Id: I9bc8a18ba615a76f3e0611eae5b2c4adc5924276 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2242097 Commit-Queue: Saagar Sanghavi Reviewed-by: Edward Lesmes Reviewed-by: Garrett Beaty --- .vpython | 7 +++ .vpython3 | 7 +++ git_cl.py | 28 +++++++++--- presubmit_support.py | 35 ++++++++++----- rdb_wrapper.py | 65 +++++++++++++++++++++++++++ tests/git_cl_test.py | 65 ++++++++++++++++++++++----- tests/presubmit_unittest.py | 15 +++++++ tests/rdb_wrapper_test.py | 87 +++++++++++++++++++++++++++++++++++++ 8 files changed, 281 insertions(+), 28 deletions(-) create mode 100644 rdb_wrapper.py create mode 100644 tests/rdb_wrapper_test.py diff --git a/.vpython b/.vpython index ace42d55ce..d0ca325f45 100644 --- a/.vpython +++ b/.vpython @@ -11,6 +11,13 @@ wheel: < version: "version:0.10.3" > +# Used by: +# presubmit_support.py +wheel: < + name: "infra/python/wheels/requests-py2_py3" + version: "version:2.13.0" +> + # Used by: # my_activity.py wheel: < diff --git a/.vpython3 b/.vpython3 index a04639e6b1..3ab3f07d4f 100644 --- a/.vpython3 +++ b/.vpython3 @@ -11,6 +11,13 @@ wheel: < version: "version:0.13.1" > +# Used by: +# presubmit_support.py +wheel: < + name: "infra/python/wheels/requests-py2_py3" + version: "version:2.13.0" +> + # Used by: # my_activity.py wheel: < diff --git a/git_cl.py b/git_cl.py index 4e2ab41ae6..da2b4bc30f 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1273,9 +1273,8 @@ class Changelist(object): return args - def RunHook( - self, committing, may_prompt, verbose, parallel, upstream, description, - all_files): + def RunHook(self, committing, may_prompt, verbose, parallel, upstream, + description, all_files, resultdb=False): """Calls sys.exit() if the hook fails; returns a HookResults otherwise.""" args = self._GetCommonPresubmitArgs(verbose, upstream) args.append('--commit' if committing else '--upload') @@ -1294,8 +1293,14 @@ class Changelist(object): args.extend(['--description_file', description_file]) start = time_time() - p = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args) + + cmd = ['vpython', PRESUBMIT_SUPPORT] + args + if resultdb: + cmd = ['rdb', 'stream', '-new'] + cmd + + p = subprocess2.Popen(cmd) exit_code = p.wait() + metrics.collector.add_repeated('sub_commands', { 'command': 'presubmit', 'execution_time': time_time() - start, @@ -1408,7 +1413,8 @@ class Changelist(object): parallel=options.parallel, upstream=base_branch, description=change_desc.description, - all_files=False) + all_files=False, + resultdb=options.resultdb) self.ExtendCC(hook_results['more_cc']) print_stats(git_diff_args) @@ -1899,7 +1905,8 @@ class Changelist(object): parallel=parallel, upstream=upstream, description=description, - all_files=False) + all_files=False, + resultdb=False) self.SubmitIssue(wait_for_merge=True) print('Issue %s has been submitted.' % self.GetIssueURL()) @@ -3820,6 +3827,9 @@ def CMDpresubmit(parser, args): parser.add_option('--parallel', action='store_true', help='Run all tests specified by input_api.RunTests in all ' 'PRESUBMIT files in parallel.') + parser.add_option('--resultdb', action='store_true', + help='Run presubmit checks in the ResultSink environment ' + 'and send results to the ResultDB database.') options, args = parser.parse_args(args) if not options.force and git_common.is_dirty_git_tree('presubmit'): @@ -3845,7 +3855,8 @@ def CMDpresubmit(parser, args): parallel=options.parallel, upstream=base_branch, description=description, - all_files=options.all) + all_files=options.all, + resultdb=options.resultdb) return 0 @@ -4052,6 +4063,9 @@ def CMDupload(parser, args): 'or a new commit is created.') parser.add_option('--git-completion-helper', action="store_true", help=optparse.SUPPRESS_HELP) + parser.add_option('--resultdb', action='store_true', + help='Run presubmit checks in the ResultSink environment ' + 'and send results to the ResultDB database.') orig_args = args (options, args) = parser.parse_args(args) diff --git a/presubmit_support.py b/presubmit_support.py index 7baa711c79..b64a7bc9a1 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -46,6 +46,7 @@ import gerrit_util import owners import owners_finder import presubmit_canned_checks +import rdb_wrapper import scm import subprocess2 as subprocess # Exposed through the API. @@ -63,7 +64,6 @@ else: # Ask for feedback only once in program lifetime. _ASKED_FOR_FEEDBACK = False - def time_time(): # Use this so that it can be mocked in tests without interfering with python # system machinery. @@ -1557,20 +1557,23 @@ class PresubmitExecuter(object): try: context['__args'] = (input_api, output_api) logging.debug('Running %s in %s', function_name, presubmit_path) - result = eval(function_name + '(*__args)', context) + + # TODO (crbug.com/1106943): Dive into each of the individual checks + + rel_path = os.path.relpath(os.getcwd(), main_path) + # Always use forward slashes, so that path is same in *nix and Windows + rel_path = rel_path.replace(os.path.sep, '/') + + with rdb_wrapper.setup_rdb(function_name, rel_path) as my_status: + result = eval(function_name + '(*__args)', context) + self._check_result_type(result) + if any(res.fatal for res in result): + my_status.status = rdb_wrapper.STATUS_FAIL logging.debug('Running %s done.', function_name) self.more_cc.extend(output_api.more_cc) finally: for f in input_api._named_temporary_files: os.remove(f) - if not isinstance(result, (tuple, list)): - raise PresubmitFailure( - 'Presubmit functions must return a tuple or list') - for item in result: - if not isinstance(item, OutputApi.PresubmitResult): - raise PresubmitFailure( - 'All presubmit results must be of types derived from ' - 'output_api.PresubmitResult') else: result = () # no error since the script doesn't care about current event. @@ -1578,6 +1581,17 @@ class PresubmitExecuter(object): os.chdir(main_path) return result + def _check_result_type(self, result): + """Helper function which ensures result is a list, and all elements are + instances of OutputApi.PresubmitResult""" + if not isinstance(result, (tuple, list)): + raise PresubmitFailure('Presubmit functions must return a tuple or list') + if not all(isinstance(res, OutputApi.PresubmitResult) for res in result): + raise PresubmitFailure( + 'All presubmit results must be of types derived from ' + 'output_api.PresubmitResult') + + def DoPresubmitChecks(change, committing, verbose, @@ -1642,7 +1656,6 @@ def DoPresubmitChecks(change, # Accept CRLF presubmit script. presubmit_script = gclient_utils.FileRead(filename, 'rU') results += executer.ExecPresubmitScript(presubmit_script, filename) - results += thread_pool.RunAsync() messages = {} diff --git a/rdb_wrapper.py b/rdb_wrapper.py new file mode 100644 index 0000000000..302ec5e01d --- /dev/null +++ b/rdb_wrapper.py @@ -0,0 +1,65 @@ +#!/usr/bin/env vpython +# Copyright (c) 2020 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import contextlib +import json +import os +import requests +import time + +# Constants describing TestStatus for ResultDB +STATUS_PASS = 'PASS' +STATUS_FAIL = 'FAIL' +STATUS_CRASH = 'CRASH' +STATUS_ABORT = 'ABORT' +STATUS_SKIP = 'SKIP' + +class ResultSinkStatus(object): + def __init__(self): + self.status = STATUS_PASS + +@contextlib.contextmanager +def setup_rdb(function_name, rel_path): + """Context Manager function for ResultDB reporting. + + Args: + function_name (str): The name of the function we are about to run. + rel_path (str): The relative path from the root of the repository to the + directory defining the check being executed. + """ + sink = None + if 'LUCI_CONTEXT' in os.environ: + with open(os.environ['LUCI_CONTEXT']) as f: + j = json.load(f) + if 'result_sink' in j: + sink = j['result_sink'] + + my_status = ResultSinkStatus() + start_time = time.time() + try: + yield my_status + except Exception: + my_status.status = STATUS_FAIL + raise + finally: + end_time = time.time() + elapsed_time = end_time - start_time + if sink != None: + tr = { + 'testId': '{0}/:{1}'.format(rel_path, function_name), + 'status': my_status.status, + 'expected': (my_status.status == STATUS_PASS), + 'duration': '{:.9f}s'.format(elapsed_time) + } + requests.post( + url='http://{0}/prpc/luci.resultsink.v1.Sink/ReportTestResults' + .format(sink['address']), + headers={ + 'Content-Type': 'application/json', + 'Accept': 'application/json', + 'Authorization': 'ResultSink {0}'.format(sink['auth_token']) + }, + data=json.dumps({'testResults': [tr]}) + ) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 29c88d5d69..fd7cd686b5 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1004,7 +1004,7 @@ class TestGitCl(unittest.TestCase): mock.patch('git_cl.gclient_utils.RunEditor', lambda *_, **__: self._mocked_call(['RunEditor'])).start() mock.patch('git_cl.DownloadGerritHook', lambda force: self._mocked_call( - 'DownloadGerritHook', force)).start() + 'DownloadGerritHook', force)).start() mock.patch('git_cl.gclient_utils.FileRead', lambda path: self._mocked_call(['FileRead', path])).start() mock.patch('git_cl.gclient_utils.FileWrite', @@ -2691,7 +2691,8 @@ class ChangelistTest(unittest.TestCase): parallel=True, upstream='upstream', description='description', - all_files=True) + all_files=True, + resultdb=False) self.assertEqual(expected_results, results) subprocess2.Popen.assert_called_once_with([ @@ -2742,7 +2743,8 @@ class ChangelistTest(unittest.TestCase): parallel=False, upstream='upstream', description='description', - all_files=False) + all_files=False, + resultdb=False) self.assertEqual(expected_results, results) subprocess2.Popen.assert_called_once_with([ @@ -2761,6 +2763,44 @@ class ChangelistTest(unittest.TestCase): 'exit_code': 0, }) + def testRunHook_FewerOptionsResultDB(self): + expected_results = { + 'more_cc': ['more@example.com', 'cc@example.com'], + 'should_continue': True, + } + gclient_utils.FileRead.return_value = json.dumps(expected_results) + git_cl.time_time.side_effect = [100, 200] + mockProcess = mock.Mock() + mockProcess.wait.return_value = 0 + subprocess2.Popen.return_value = mockProcess + + git_cl.Changelist.GetAuthor.return_value = None + git_cl.Changelist.GetIssue.return_value = None + git_cl.Changelist.GetPatchset.return_value = None + git_cl.Changelist.GetCodereviewServer.return_value = None + + cl = git_cl.Changelist() + results = cl.RunHook( + committing=False, + may_prompt=False, + verbose=0, + parallel=False, + upstream='upstream', + description='description', + all_files=False, + resultdb=True) + + self.assertEqual(expected_results, results) + subprocess2.Popen.assert_called_once_with([ + 'rdb', 'stream', '-new', + 'vpython', 'PRESUBMIT_SUPPORT', + '--root', 'root', + '--upstream', 'upstream', + '--upload', + '--json_output', '/tmp/fake-temp2', + '--description_file', '/tmp/fake-temp1', + ]) + @mock.patch('sys.exit', side_effect=SystemExitMock) def testRunHook_Failure(self, _mock): git_cl.time_time.side_effect = [100, 200] @@ -2777,7 +2817,8 @@ class ChangelistTest(unittest.TestCase): parallel=True, upstream='upstream', description='description', - all_files=True) + all_files=True, + resultdb=False) sys.exit.assert_called_once_with(2) @@ -2889,7 +2930,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase): parallel=None, upstream='upstream', description='fetch description', - all_files=None) + all_files=None, + resultdb=None) def testNoIssue(self): git_cl.Changelist.GetIssue.return_value = None @@ -2901,7 +2943,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase): parallel=None, upstream='upstream', description='get description', - all_files=None) + all_files=None, + resultdb=None) def testCustomBranch(self): self.assertEqual(0, git_cl.main(['presubmit', 'custom_branch'])) @@ -2912,11 +2955,13 @@ class CMDPresubmitTestCase(CMDTestCaseBase): parallel=None, upstream='custom_branch', description='fetch description', - all_files=None) + all_files=None, + resultdb=None) def testOptions(self): self.assertEqual( - 0, git_cl.main(['presubmit', '-v', '-v', '--all', '--parallel', '-u'])) + 0, git_cl.main(['presubmit', '-v', '-v', '--all', '--parallel', '-u', + '--resultdb'])) git_cl.Changelist.RunHook.assert_called_once_with( committing=False, may_prompt=False, @@ -2924,8 +2969,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase): parallel=True, upstream='upstream', description='fetch description', - all_files=True) - + all_files=True, + resultdb=True) class CMDTryResultsTestCase(CMDTestCaseBase): _DEFAULT_REQUEST = { diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index cfb9536183..9b00784b6a 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -179,6 +179,7 @@ index fe3de7b..54ae6e1 100755 mock.patch('os.path.isfile').start() mock.patch('os.remove').start() mock.patch('presubmit_support._parse_files').start() + mock.patch('presubmit_support.rdb_wrapper.setup_rdb').start() mock.patch('presubmit_support.sigint_handler').start() mock.patch('presubmit_support.time_time', return_value=0).start() mock.patch('presubmit_support.warn').start() @@ -524,6 +525,20 @@ class PresubmitUnittest(PresubmitTestsBase): ' return "foo"', fake_presubmit) + self.assertFalse(executer.ExecPresubmitScript( + 'def CheckChangeOnCommit(input_api, output_api):\n' + ' results = []\n' + ' results.extend(input_api.canned_checks.CheckChangeHasBugField(\n' + ' input_api, output_api))\n' + ' results.extend(input_api.canned_checks.CheckChangeHasNoUnwantedTags(\n' + ' input_api, output_api))\n' + ' results.extend(input_api.canned_checks.CheckChangeHasDescription(\n' + ' input_api, output_api))\n' + ' return results\n', + fake_presubmit)) + + presubmit.rdb_wrapper.setup_rdb.assert_called() + self.assertRaises(presubmit.PresubmitFailure, executer.ExecPresubmitScript, 'def CheckChangeOnCommit(input_api, output_api):\n' diff --git a/tests/rdb_wrapper_test.py b/tests/rdb_wrapper_test.py new file mode 100644 index 0000000000..968fafa91a --- /dev/null +++ b/tests/rdb_wrapper_test.py @@ -0,0 +1,87 @@ +#!/usr/bin/env vpython3 +# Copyright (c) 2020 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +"""Unit tests for rdb_wrapper.py""" + +from __future__ import print_function + +import json +import logging +import os +import requests +import sys +import time +import unittest + +if sys.version_info.major == 2: + import mock + BUILTIN_OPEN = '__builtin__.open' +else: + from unittest import mock + BUILTIN_OPEN = 'builtins.open' + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +import rdb_wrapper + +class TestSetupRDB(unittest.TestCase): + + def setUp(self): + super(TestSetupRDB, self).setUp() + mock.patch(BUILTIN_OPEN, mock.mock_open(read_data = + '''{"result_sink":{"address": "fakeAddr","auth_token" : "p@$$w0rD"}}''') + ).start() + mock.patch('os.environ', {'LUCI_CONTEXT': 'dummy_file.txt'}).start() + mock.patch('requests.post').start() + mock.patch('time.time', side_effect=[1.0, 2.0, 3.0, 4.0, 5.0]).start() + + def test_setup_rdb(self): + with rdb_wrapper.setup_rdb("_foobar", './my/folder') as my_status_obj: + self.assertEqual(my_status_obj.status, rdb_wrapper.STATUS_PASS) + my_status_obj.status = rdb_wrapper.STATUS_FAIL + + expectedTr = { + 'testId' : './my/folder/:_foobar', + 'status' : rdb_wrapper.STATUS_FAIL, + 'expected': False, + 'duration': '1.000000000s' + } + + requests.post.assert_called_once_with( + url='http://fakeAddr/prpc/luci.resultsink.v1.Sink/ReportTestResults', + headers={ + 'Content-Type': 'application/json', + 'Accept': 'application/json', + 'Authorization': 'ResultSink p@$$w0rD' + }, + data=json.dumps({'testResults': [expectedTr]}) + ) + + def test_setup_rdb_exception(self): + with self.assertRaises(Exception): + with rdb_wrapper.setup_rdb("_foobar", './my/folder'): + raise Exception("Generic Error") + + expectedTr = { + 'testId': './my/folder/:_foobar', + 'status': rdb_wrapper.STATUS_FAIL, + 'expected': False, + 'duration': '1.000000000s' + } + + requests.post.assert_called_once_with( + url='http://fakeAddr/prpc/luci.resultsink.v1.Sink/ReportTestResults', + headers={ + 'Content-Type': 'application/json', + 'Accept': 'application/json', + 'Authorization': 'ResultSink p@$$w0rD' + }, + data=json.dumps({'testResults': [expectedTr]}) + ) + +if __name__ == '__main__': + logging.basicConfig( + level=logging.DEBUG if '-v' in sys.argv else logging.ERROR) + unittest.main()