From 5c8c6de7e5cb29982554c8367f85fc949496593f Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Fri, 18 Mar 2011 16:20:18 +0000 Subject: [PATCH] Make sure warnings fail the check when may_prompt=False. Split presubmit_support.Main() in two to make it simpler. Handle better the case with unversioned files and add a unit test. R=dpranke@chromium.org Review URL: http://codereview.chromium.org/6674059 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@78700 0039d316-1c4b-4281-b951-d872f2087c98 --- presubmit_support.py | 75 ++++++++++++++++++------------------- scm.py | 18 +++++++++ tests/presubmit_unittest.py | 47 +++++++++++++++-------- tests/scm_unittest.py | 4 +- 4 files changed, 88 insertions(+), 56 deletions(-) diff --git a/presubmit_support.py b/presubmit_support.py index 25370d672..f1f70d9c4 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -6,7 +6,7 @@ """Enables directory-specific presubmit checks to run at upload and/or commit. """ -__version__ = '1.3.5' +__version__ = '1.4' # TODO(joi) Add caching where appropriate/needed. The API is designed to allow # caching (between all different invocations of presubmit scripts for a given @@ -1083,9 +1083,12 @@ def DoPresubmitChecks(change, if total_time > 1.0: output.write("Presubmit checks took %.1fs to calculate.\n" % total_time) - if not errors and warnings and may_prompt: - output.prompt_yes_no('There were presubmit warnings. ' - 'Are you sure you wish to continue? (y/N): ') + if not errors and warnings: + if may_prompt: + output.prompt_yes_no('There were presubmit warnings. ' + 'Are you sure you wish to continue? (y/N): ') + else: + output.fail() global _ASKED_FOR_FEEDBACK # Ask for feedback one time out of 5. @@ -1120,8 +1123,31 @@ def ParseFiles(args, recursive): return files +def load_files(options, args): + """Tries to determine the SCM.""" + change_scm = scm.determine_scm(options.root) + files = [] + if change_scm == 'svn': + change_class = SvnChange + status_fn = scm.SVN.CaptureStatus + elif change_scm == 'git': + change_class = GitChange + status_fn = scm.GIT.CaptureStatus + else: + logging.info('Doesn\'t seem under source control. Got %d files' % len(args)) + if not args: + return None, None + change_class = Change + if args: + files = ParseFiles(args, options.recursive) + else: + # Grab modified files. + files = status_fn([options.root]) + return change_class, files + + def Main(argv): - parser = optparse.OptionParser(usage="%prog [options]", + parser = optparse.OptionParser(usage="%prog [options] ", version="%prog " + str(__version__)) parser.add_option("-c", "--commit", action="store_true", default=False, help="Use commit instead of upload checks") @@ -1131,7 +1157,6 @@ def Main(argv): help="Act recursively") parser.add_option("-v", "--verbose", action="store_true", default=False, help="Verbose output") - parser.add_option("--files") parser.add_option("--name", default='no name') parser.add_option("--description", default='') parser.add_option("--issue", type='int', default=0) @@ -1146,43 +1171,15 @@ def Main(argv): options, args = parser.parse_args(argv) if options.verbose: logging.basicConfig(level=logging.DEBUG) - if os.path.isdir(os.path.join(options.root, '.svn')): - change_class = SvnChange - if not options.files: - if args: - options.files = ParseFiles(args, options.recursive) - else: - # Grab modified files. - options.files = scm.SVN.CaptureStatus([options.root]) - else: - is_git = os.path.isdir(os.path.join(options.root, '.git')) - if not is_git: - is_git = (0 == subprocess.call( - ['git', 'rev-parse', '--show-cdup'], - stdout=subprocess.PIPE, cwd=options.root)) - if is_git: - # Only look at the subdirectories below cwd. - change_class = GitChange - if not options.files: - if args: - options.files = ParseFiles(args, options.recursive) - else: - # Grab modified files. - options.files = scm.GIT.CaptureStatus([options.root]) - else: - logging.info('Doesn\'t seem under source control.') - change_class = Change + change_class, files = load_files(options, args) + if not change_class: + parser.error('For unversioned directory, is not optional.') if options.verbose: - if not options.files: - print "Found no files." - elif len(options.files) != 1: - print "Found %d files." % len(options.files) - else: - print "Found 1 file." + print "Found %d file(s)." % len(files) results = DoPresubmitChecks(change_class(options.name, options.description, options.root, - options.files, + files, options.issue, options.patchset), options.commit, diff --git a/scm.py b/scm.py index 58f11b3de..2156b293d 100644 --- a/scm.py +++ b/scm.py @@ -65,6 +65,24 @@ def GenFakeDiff(filename): return result +def determine_scm(root): + """Similar to upload.py's version but much simpler. + + Returns 'svn', 'git' or None. + """ + if os.path.isdir(os.path.join(root, '.svn')): + return 'svn' + elif os.path.isdir(os.path.join(root, '.svn')): + return 'git' + else: + if (0 == subprocess.call( + ['git', 'rev-parse', '--show-cdup'], + stdout=subprocess.PIPE, cwd=root)): + return 'git' + else: + return None + + class GIT(object): @staticmethod def Capture(args, **kwargs): diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 3bf7d79e3..b2cb19083 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -119,6 +119,7 @@ def GetPreferredTrySlaves(): presubmit.os.path.abspath = MockAbsPath presubmit.os.getcwd = self.RootDir presubmit.os.chdir = MockChdir + self.mox.StubOutWithMock(presubmit.scm, 'determine_scm') self.mox.StubOutWithMock(presubmit.scm.SVN, 'CaptureInfo') self.mox.StubOutWithMock(presubmit.scm.SVN, 'GetFileProperty') self.mox.StubOutWithMock(presubmit.gclient_utils, 'FileRead') @@ -135,12 +136,12 @@ class PresubmitUnittest(PresubmitTestsBase): self.mox.ReplayAll() members = [ 'AffectedFile', 'Change', 'DoGetTrySlaves', 'DoPresubmitChecks', - 'GetTrySlavesExecuter', 'GitAffectedFile', 'GitChange', - 'InputApi', 'ListRelevantPresubmitFiles', 'Main', + 'GetTrySlavesExecuter', 'GitAffectedFile', + 'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'Main', 'NotImplementedException', 'OutputApi', 'ParseFiles', 'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs', 'SvnAffectedFile', 'SvnChange', 'cPickle', 'cStringIO', - 'exceptions', 'fnmatch', 'gclient_utils', 'glob', 'json', + 'exceptions', 'fnmatch', 'gclient_utils', 'glob', 'json', 'load_files', 'logging', 'marshal', 'normpath', 'optparse', 'os', 'owners', 'pickle', 'presubmit_canned_checks', 'random', 're', 'scm', 'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest', 'urllib2', @@ -662,19 +663,14 @@ def CheckChangeOnCommit(input_api, output_api): self.fake_root_dir, None, False, output)) - def testMain(self): + def testMainUnversioned(self): # OptParser calls presubmit.os.path.exists and is a pain when mocked. self.UnMock(presubmit.os.path, 'exists') self.mox.StubOutWithMock(presubmit, 'DoPresubmitChecks') self.mox.StubOutWithMock(presubmit, 'ParseFiles') - presubmit.os.path.isdir(presubmit.os.path.join(self.fake_root_dir, '.svn') - ).AndReturn(False) - presubmit.os.path.isdir(presubmit.os.path.join(self.fake_root_dir, '.git') - ).AndReturn(False) - presubmit.subprocess.call( - ['git', 'rev-parse', '--show-cdup'], - cwd=self.fake_root_dir, - stdout=presubmit.subprocess.PIPE).AndReturn(1) + presubmit.scm.determine_scm(self.fake_root_dir).AndReturn(None) + presubmit.ParseFiles(['random_file.txt'], None + ).AndReturn(['random_file.txt']) output = self.mox.CreateMock(presubmit.PresubmitOutput) output.should_continue().AndReturn(False) @@ -684,9 +680,30 @@ def CheckChangeOnCommit(input_api, output_api): None, False).AndReturn(output) self.mox.ReplayAll() - self.assertEquals(True, - presubmit.Main(['presubmit', '--root', - self.fake_root_dir])) + self.assertEquals( + True, + presubmit.Main(['--root', self.fake_root_dir, 'random_file.txt'])) + + def testMainUnversionedFail(self): + # OptParser calls presubmit.os.path.exists and is a pain when mocked. + self.UnMock(presubmit.os.path, 'exists') + self.mox.StubOutWithMock(presubmit, 'DoPresubmitChecks') + self.mox.StubOutWithMock(presubmit, 'ParseFiles') + presubmit.scm.determine_scm(self.fake_root_dir).AndReturn(None) + self.mox.StubOutWithMock(presubmit.sys, 'stderr') + presubmit.sys.stderr.write( + 'Usage: presubmit_unittest.py [options] \n') + presubmit.sys.stderr.write('\n') + presubmit.sys.stderr.write( + 'presubmit_unittest.py: error: For unversioned directory, is ' + 'not optional.\n') + self.mox.ReplayAll() + + try: + presubmit.Main(['--root', self.fake_root_dir]) + self.fail() + except SystemExit, e: + self.assertEquals(2, e.code) class InputApiUnittest(PresubmitTestsBase): diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index 783fecd3d..1b157e4e7 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -39,8 +39,8 @@ class RootTestCase(BaseSCMTestCase): self.mox.ReplayAll() members = [ 'GetCasedPath', 'GenFakeDiff', 'GIT', 'SVN', 'ValidateEmail', - 'cStringIO', 'gclient_utils', 'glob', 'logging', 'os', 're', 'shutil', - 'subprocess', 'sys', 'tempfile', 'time', 'xml', + 'cStringIO', 'determine_scm', 'gclient_utils', 'glob', 'logging', 'os', + 're', 'shutil', 'subprocess', 'sys', 'tempfile', 'time', 'xml', ] # If this test fails, you should add the relevant test. self.compareMembers(scm, members)