From bf1fdca798c9dae0dd395754a6a15663b9d5d048 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Mon, 1 Nov 2010 18:05:36 +0000 Subject: [PATCH] Add rietveld member to ChangeInfo and use this value to contact rietveld. This makes gcl to use a specific rietveld instance per change list. This is useful when moving from one rietveld instance to another, so in-flight reviews are sent to the right instance. Also clear gcl.CODEREVIEW_SETTINGS between gcl_unittest tests to remove tests interference. TEST=updated unit tests BUG=none Review URL: http://codereview.chromium.org/4218006 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@64631 0039d316-1c4b-4281-b951-d872f2087c98 --- gcl.py | 127 +++++++++++++++++++++--------------------- tests/gcl_unittest.py | 58 +++++++++++-------- 2 files changed, 96 insertions(+), 89 deletions(-) diff --git a/gcl.py b/gcl.py index fb355e586..058837a1e 100755 --- a/gcl.py +++ b/gcl.py @@ -274,22 +274,13 @@ class ChangeInfo(object): files: a list of 2 tuple containing (status, filename) of changed files, with paths being relative to the top repository directory. local_root: Local root directory + rietveld: rietveld server for this change """ - + # Kept for unit test support. This is for the old format, it's deprecated. _SEPARATOR = "\n-----\n" - # The info files have the following format: - # issue_id, patchset\n (, patchset is optional) - # _SEPARATOR\n - # filepath1\n - # filepath2\n - # . - # . - # filepathn\n - # _SEPARATOR\n - # description def __init__(self, name, issue, patchset, description, files, local_root, - needs_upload=False): + rietveld, needs_upload=False): self.name = name self.issue = int(issue) self.patchset = int(patchset) @@ -300,6 +291,10 @@ class ChangeInfo(object): self.patch = None self._local_root = local_root self.needs_upload = needs_upload + self.rietveld = rietveld + if not self.rietveld: + # Set the default value. + self.rietveld = GetCodeReviewSetting('CODE_REVIEW_SERVER') def NeedsUpload(self): return self.needs_upload @@ -337,6 +332,7 @@ class ChangeInfo(object): 'needs_upload': self.NeedsUpload(), 'files': self.GetFiles(), 'description': self.description, + 'rietveld': self.rietveld, }, sort_keys=True, indent=2) gclient_utils.FileWrite(GetChangelistInfoFile(self.name), data) @@ -348,13 +344,46 @@ class ChangeInfo(object): """Closes the Rietveld issue for this changelist.""" data = [("description", self.description),] ctype, body = upload.EncodeMultipartFormData(data, []) - SendToRietveld("/%d/close" % self.issue, body, ctype) + self.SendToRietveld('/%d/close' % self.issue, body, ctype) def UpdateRietveldDescription(self): """Sets the description for an issue on Rietveld.""" data = [("description", self.description),] ctype, body = upload.EncodeMultipartFormData(data, []) - SendToRietveld("/%d/description" % self.issue, body, ctype) + self.SendToRietveld('/%d/description' % self.issue, body, ctype) + + def GetIssueDescription(self): + """Returns the issue description from Rietveld.""" + return self.SendToRietveld('/%d/description' % self.issue) + + def PrimeLint(self): + """Do background work on Rietveld to lint the file so that the results are + ready when the issue is viewed.""" + if self.issue and self.patchset: + self.SendToRietveld('/lint/issue%s_%s' % (self.issue, self.patchset), + timeout=1) + + def SendToRietveld(self, request_path, payload=None, + content_type="application/octet-stream", timeout=None): + """Send a POST/GET to Rietveld. Returns the response body.""" + if not self.rietveld: + ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND) + def GetUserCredentials(): + """Prompts the user for a username and password.""" + email = upload.GetEmail('Email (login for uploading to %s)' % + self.rietveld) + password = getpass.getpass('Password for %s: ' % email) + return email, password + rpc_server = upload.HttpRpcServer(self.rietveld, + GetUserCredentials, + save_cookies=True) + try: + return rpc_server.Send(request_path, payload, content_type, timeout) + except urllib2.URLError: + if timeout is None: + ErrorExit('Error accessing url %s' % request_path) + else: + return None def MissingTests(self): """Returns True if the change looks like it needs unit tests but has none. @@ -453,7 +482,7 @@ class ChangeInfo(object): if not os.path.exists(info_file): if fail_on_not_found: ErrorExit("Changelist " + changename + " not found.") - return ChangeInfo(changename, 0, 0, '', None, local_root, + return ChangeInfo(changename, 0, 0, '', None, local_root, rietveld=None, needs_upload=False) content = gclient_utils.FileRead(info_file, 'r') save = False @@ -484,13 +513,24 @@ class ChangeInfo(object): files[files.index(item)] = (status, item[1]) change_info = ChangeInfo(changename, values['issue'], values['patchset'], values['description'], files, - local_root, values['needs_upload']) + local_root, values.get('rietveld'), + values['needs_upload']) if save: change_info.Save() return change_info @staticmethod def _LoadOldFormat(content): + # The info files have the following format: + # issue_id, patchset\n (, patchset is optional) + # _SEPARATOR\n + # filepath1\n + # filepath2\n + # . + # . + # filepathn\n + # _SEPARATOR\n + # description split_data = content.split(ChangeInfo._SEPARATOR, 2) if len(split_data) != 3: raise ValueError('Bad change format') @@ -534,7 +574,7 @@ def LoadChangelistInfoForMultiple(changenames, local_root, fail_on_not_found, """ changes = changenames.split(',') aggregate_change_info = ChangeInfo(changenames, 0, 0, '', None, local_root, - needs_upload=False) + rietveld=None, needs_upload=False) for change in changes: aggregate_change_info._files += ChangeInfo.Load(change, local_root, @@ -615,34 +655,6 @@ def GetFilesNotInCL(): return modified_files[""] -def SendToRietveld(request_path, payload=None, - content_type="application/octet-stream", timeout=None): - """Send a POST/GET to Rietveld. Returns the response body.""" - server = GetCodeReviewSetting("CODE_REVIEW_SERVER") - if not server: - ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND) - def GetUserCredentials(): - """Prompts the user for a username and password.""" - email = upload.GetEmail("Email (login for uploading to %s)" % server) - password = getpass.getpass("Password for %s: " % email) - return email, password - rpc_server = upload.HttpRpcServer(server, - GetUserCredentials, - save_cookies=True) - try: - return rpc_server.Send(request_path, payload, content_type, timeout) - except urllib2.URLError: - if timeout is None: - ErrorExit("Error accessing url %s" % request_path) - else: - return None - - -def GetIssueDescription(issue): - """Returns the issue description from Rietveld.""" - return SendToRietveld("/%d/description" % issue) - - def ListFiles(show_unknown_files): files = GetModifiedFiles() cl_keys = files.keys() @@ -783,10 +795,7 @@ def CMDupload(change_info, args): args.append("--send_mail") upload_arg = ["upload.py", "-y"] - server = GetCodeReviewSetting("CODE_REVIEW_SERVER") - if not server: - ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND) - upload_arg.append("--server=%s" % server) + upload_arg.append("--server=%s" % change_info.rietveld) upload_arg.extend(args) desc_file = "" @@ -837,7 +846,6 @@ def CMDupload(change_info, args): # shows the correct base. previous_cwd = os.getcwd() os.chdir(change_info.GetLocalRoot()) - # If we have a lot of files with long paths, then we won't be able to fit # the command to "svn diff". Instead, we generate the diff manually for # each file and concatenate them before passing it to upload.py. @@ -851,17 +859,9 @@ def CMDupload(change_info, args): if desc_file: os.remove(desc_file) - - # Do background work on Rietveld to lint the file so that the results are - # ready when the issue is viewed. - SendToRietveld("/lint/issue%s_%s" % (issue, patchset), timeout=0.5) - - # Move back before considering try, so GetCodeReviewSettings is - # consistent. + change_info.PrimeLint() os.chdir(previous_cwd) - print "*** Upload does not submit a try; use gcl try to submit a try. ***" - return 0 @@ -933,13 +933,11 @@ def CMDcommit(change_info, args): commit_cmd = ["svn", "commit"] if change_info.issue: # Get the latest description from Rietveld. - change_info.description = GetIssueDescription(change_info.issue) + change_info.description = change_info.GetIssueDescription() commit_message = change_info.description.replace('\r\n', '\n') if change_info.issue: - server = GetCodeReviewSetting("CODE_REVIEW_SERVER") - if not server: - ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND) + server = change_info.rietveld if not server.startswith("http://") and not server.startswith("https://"): server = "http://" + server commit_message += ('\nReview URL: %s/%d' % (server, change_info.issue)) @@ -1006,7 +1004,7 @@ def CMDchange(args): if change_info.issue and not change_info.NeedsUpload(): try: - description = GetIssueDescription(change_info.issue) + description = change_info.GetIssueDescription() except urllib2.HTTPError, err: if err.code == 404: # The user deleted the issue in Rietveld, so forget the old issue id. @@ -1114,7 +1112,6 @@ def CMDlint(change_info, args): # shows the correct base. previous_cwd = os.getcwd() os.chdir(change_info.GetLocalRoot()) - # Process cpplints arguments if any. filenames = cpplint.ParseArguments(args + change_info.GetFileNames()) diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py index 0b62840d9..b0e8c5783 100755 --- a/tests/gcl_unittest.py +++ b/tests/gcl_unittest.py @@ -24,6 +24,11 @@ class GclTestsBase(SuperMoxTestBase): self.mox.StubOutWithMock(gcl.gclient_utils, 'FileRead') self.mox.StubOutWithMock(gcl.gclient_utils, 'FileWrite') gcl.REPOSITORY_ROOT = None + self.old_review_settings = gcl.CODEREVIEW_SETTINGS + self.assertEquals(gcl.CODEREVIEW_SETTINGS, {}) + + def tearDown(self): + gcl.CODEREVIEW_SETTINGS = self.old_review_settings class GclUnittest(GclTestsBase): @@ -46,12 +51,11 @@ class GclUnittest(GclTestsBase): 'GenerateChangeName', 'GenerateDiff', 'GetCLs', 'GetCacheDir', 'GetCachedFile', 'GetChangelistInfoFile', 'GetChangesDir', 'GetCodeReviewSetting', 'GetEditor', 'GetFilesNotInCL', 'GetInfoDir', - 'GetIssueDescription', 'GetModifiedFiles', 'GetRepositoryRoot', - 'ListFiles', + 'GetModifiedFiles', 'GetRepositoryRoot', 'ListFiles', 'LoadChangelistInfoForMultiple', 'MISSING_TEST_MSG', 'OptionallyDoPresubmitChecks', 'REPOSITORY_ROOT', 'RunShell', 'RunShellWithReturnCode', 'SVN', - 'SendToRietveld', 'TryChange', 'UnknownFiles', 'Warn', + 'TryChange', 'UnknownFiles', 'Warn', 'attrs', 'breakpad', 'defer_attributes', 'gclient_utils', 'getpass', 'json', 'main', 'need_change', 'need_change_and_args', 'no_args', 'os', 'random', 're', 'string', 'subprocess', 'sys', 'tempfile', @@ -137,20 +141,23 @@ class ChangeInfoUnittest(GclTestsBase): def testChangeInfoMembers(self): self.mox.ReplayAll() members = [ - 'CloseIssue', 'Delete', 'GetFiles', 'GetFileNames', 'GetLocalRoot', - 'Exists', 'Load', 'MissingTests', 'NeedsUpload', 'Save', - 'UpdateRietveldDescription', 'description', 'issue', 'name', - 'needs_upload', 'patch', 'patchset', + 'CloseIssue', 'Delete', 'Exists', 'GetFiles', 'GetFileNames', + 'GetLocalRoot', 'GetIssueDescription', 'Load', 'MissingTests', + 'NeedsUpload', 'PrimeLint', 'Save', 'SendToRietveld', + 'UpdateRietveldDescription', + 'description', 'issue', 'name', + 'needs_upload', 'patch', 'patchset', 'rietveld', ] # If this test fails, you should add the relevant test. - self.compareMembers(gcl.ChangeInfo('', 0, 0, '', None, self.fake_root_dir), - members) + self.compareMembers( + gcl.ChangeInfo('', 0, 0, '', None, self.fake_root_dir, 'foo'), + members) def testChangeInfoBase(self): files = [('M', 'foo'), ('A', 'bar')] self.mox.ReplayAll() o = gcl.ChangeInfo('name2', '42', '53', 'description2', files, - self.fake_root_dir) + self.fake_root_dir, 'foo') self.assertEquals(o.name, 'name2') self.assertEquals(o.issue, 42) self.assertEquals(o.patchset, 53) @@ -161,11 +168,13 @@ class ChangeInfoUnittest(GclTestsBase): self.assertEquals(o.GetLocalRoot(), self.fake_root_dir) def testLoadWithIssue(self): + self.mox.StubOutWithMock(gcl, 'GetCodeReviewSetting') description = ["This is some description.", "force an extra separator."] gcl.GetChangelistInfoFile('bleh').AndReturn('bleeeh') gcl.os.path.exists('bleeeh').AndReturn(True) gcl.gclient_utils.FileRead('bleeeh', 'r').AndReturn( gcl.ChangeInfo._SEPARATOR.join(["42, 53", "G b.cc"] + description)) + gcl.GetCodeReviewSetting('CODE_REVIEW_SERVER').AndReturn('foo') # Does an upgrade. gcl.GetChangelistInfoFile('bleh').AndReturn('bleeeh') gcl.gclient_utils.FileWrite('bleeeh', mox.IgnoreArg()) @@ -180,10 +189,12 @@ class ChangeInfoUnittest(GclTestsBase): self.assertEquals(change_info.GetFiles(), [('G ', 'b.cc')]) def testLoadEmpty(self): + self.mox.StubOutWithMock(gcl, 'GetCodeReviewSetting') gcl.GetChangelistInfoFile('bleh').AndReturn('bleeeh') gcl.os.path.exists('bleeeh').AndReturn(True) gcl.gclient_utils.FileRead('bleeeh', 'r').AndReturn( gcl.ChangeInfo._SEPARATOR.join(["", "", ""])) + gcl.GetCodeReviewSetting('CODE_REVIEW_SERVER').AndReturn('foo') # Does an upgrade. gcl.GetChangelistInfoFile('bleh').AndReturn('bleeeh') gcl.gclient_utils.FileWrite('bleeeh', mox.IgnoreArg()) @@ -200,25 +211,25 @@ class ChangeInfoUnittest(GclTestsBase): gcl.GetChangelistInfoFile('').AndReturn('foo') values = { 'description': '', 'patchset': 2, 'issue': 1, - 'files': [], 'needs_upload': False} + 'files': [], 'needs_upload': False, 'rietveld': 'foo'} gcl.gclient_utils.FileWrite( 'foo', gcl.json.dumps(values, sort_keys=True, indent=2)) self.mox.ReplayAll() - change_info = gcl.ChangeInfo('', 1, 2, '', None, self.fake_root_dir) + change_info = gcl.ChangeInfo('', 1, 2, '', None, self.fake_root_dir, 'foo') change_info.Save() def testSaveDirty(self): gcl.GetChangelistInfoFile('n').AndReturn('foo') values = { 'description': 'des', 'patchset': 0, 'issue': 0, - 'files': [], 'needs_upload': True} + 'files': [], 'needs_upload': True, 'rietveld': 'foo'} gcl.gclient_utils.FileWrite( 'foo', gcl.json.dumps(values, sort_keys=True, indent=2)) self.mox.ReplayAll() change_info = gcl.ChangeInfo('n', 0, 0, 'des', None, self.fake_root_dir, - needs_upload=True) + 'foo', needs_upload=True) change_info.Save() @@ -230,7 +241,7 @@ class CMDuploadUnittest(GclTestsBase): self.mox.StubOutWithMock(gcl, 'GenerateDiff') self.mox.StubOutWithMock(gcl, 'GetCodeReviewSetting') self.mox.StubOutWithMock(gcl, 'GetRepositoryRoot') - self.mox.StubOutWithMock(gcl, 'SendToRietveld') + self.mox.StubOutWithMock(gcl.ChangeInfo, 'SendToRietveld') self.mox.StubOutWithMock(gcl, 'TryChange') self.mox.StubOutWithMock(gcl.ChangeInfo, 'Load') @@ -242,9 +253,10 @@ class CMDuploadUnittest(GclTestsBase): change_info.description = 'deescription', change_info.files = [('A', 'aa'), ('M', 'bb')] change_info.patch = None + change_info.rietveld = 'my_server' files = [item[1] for item in change_info.files] gcl.DoPresubmitChecks(change_info, False, True).AndReturn(True) - gcl.GetCodeReviewSetting('CODE_REVIEW_SERVER').AndReturn('my_server') + #gcl.GetCodeReviewSetting('CODE_REVIEW_SERVER').AndReturn('my_server') gcl.os.getcwd().AndReturn('somewhere') change_info.GetFiles().AndReturn(change_info.files) change_info.GetLocalRoot().AndReturn('proout') @@ -256,12 +268,12 @@ class CMDuploadUnittest(GclTestsBase): '--message=\'\'', '--issue=1'], change_info.patch).AndReturn(("1", "2")) - gcl.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=0.5) + change_info.Save() + change_info.PrimeLint() gcl.os.chdir('somewhere') gcl.sys.stdout.write("*** Upload does not submit a try; use gcl try to" " submit a try. ***") gcl.sys.stdout.write("\n") - change_info.Save() gcl.GetRepositoryRoot().AndReturn(self.fake_root_dir) gcl.ChangeInfo.Load('naame', self.fake_root_dir, True, True ).AndReturn(change_info) @@ -275,11 +287,10 @@ class CMDuploadUnittest(GclTestsBase): def testServerOverride(self): change_info = gcl.ChangeInfo('naame', 0, 0, 'deescription', [('A', 'aa'), ('M', 'bb')], - self.fake_root_dir) + self.fake_root_dir, 'my_server') self.mox.StubOutWithMock(change_info, 'Save') change_info.Save() gcl.DoPresubmitChecks(change_info, False, True).AndReturn(True) - gcl.GetCodeReviewSetting('CODE_REVIEW_SERVER').AndReturn('my_server') gcl.tempfile.mkstemp(text=True).AndReturn((42, 'descfile')) gcl.os.write(42, change_info.description) gcl.os.close(42) @@ -292,7 +303,7 @@ class CMDuploadUnittest(GclTestsBase): "--description_file=descfile", "--message=deescription"], change_info.patch).AndReturn(("1", "2")) gcl.os.remove('descfile') - gcl.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=0.5) + change_info.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=1) gcl.os.chdir('somewhere') gcl.sys.stdout.write("*** Upload does not submit a try; use gcl try to" " submit a try. ***") @@ -310,11 +321,10 @@ class CMDuploadUnittest(GclTestsBase): def testNormal(self): change_info = gcl.ChangeInfo('naame', 0, 0, 'deescription', [('A', 'aa'), ('M', 'bb')], - self.fake_root_dir) + self.fake_root_dir, 'my_server') self.mox.StubOutWithMock(change_info, 'Save') change_info.Save() gcl.DoPresubmitChecks(change_info, False, True).AndReturn(True) - gcl.GetCodeReviewSetting('CODE_REVIEW_SERVER').AndReturn('my_server') gcl.tempfile.mkstemp(text=True).AndReturn((42, 'descfile')) gcl.os.write(42, change_info.description) gcl.os.close(42) @@ -327,7 +337,7 @@ class CMDuploadUnittest(GclTestsBase): "--description_file=descfile", "--message=deescription"], change_info.patch).AndReturn(("1", "2")) gcl.os.remove('descfile') - gcl.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=0.5) + change_info.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=1) gcl.os.chdir('somewhere') gcl.sys.stdout.write("*** Upload does not submit a try; use gcl try to" " submit a try. ***")