diff options
Diffstat (limited to 'WebKitTools/Scripts/webkitpy')
48 files changed, 696 insertions, 454 deletions
diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/api.py b/WebKitTools/Scripts/webkitpy/common/checkout/api.py index a5ac939..ca28e32 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/api.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/api.py @@ -83,16 +83,16 @@ class Checkout(object): def bug_id_for_revision(self, revision): return self.commit_info_for_revision(revision).bug_id() - def modified_changelogs(self, git_commit, squash): + def modified_changelogs(self, git_commit): # SCM returns paths relative to scm.checkout_root # Callers (especially those using the ChangeLog class) may # expect absolute paths, so this method returns absolute paths. - changed_files = self._scm.changed_files(git_commit, squash) + changed_files = self._scm.changed_files(git_commit) absolute_paths = [os.path.join(self._scm.checkout_root, path) for path in changed_files] return [path for path in absolute_paths if self._is_path_to_changelog(path)] - def commit_message_for_this_commit(self, git_commit, squash): - changelog_paths = self.modified_changelogs(git_commit, squash) + def commit_message_for_this_commit(self, git_commit): + changelog_paths = self.modified_changelogs(git_commit) if not len(changelog_paths): raise ScriptError(message="Found no modified ChangeLogs, cannot create a commit message.\n" "All changes require a ChangeLog. See:\n" @@ -109,9 +109,9 @@ class Checkout(object): # FIXME: We should sort and label the ChangeLog messages like commit-log-editor does. return CommitMessage("".join(changelog_messages).splitlines()) - def bug_id_for_this_commit(self, git_commit, squash): + def bug_id_for_this_commit(self, git_commit): try: - return parse_bug_id(self.commit_message_for_this_commit(git_commit, squash).message()) + return parse_bug_id(self.commit_message_for_this_commit(git_commit).message()) except ScriptError, e: pass # We might not have ChangeLogs. @@ -131,7 +131,7 @@ class Checkout(object): # We revert the ChangeLogs because removing lines from a ChangeLog # doesn't make sense. ChangeLogs are append only. - changelog_paths = self.modified_changelogs(git_commit=None, squash=False) + changelog_paths = self.modified_changelogs(git_commit=None) if len(changelog_paths): self._scm.revert_files(changelog_paths) diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py b/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py index 1436379..fdfd879 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py @@ -114,11 +114,11 @@ class CommitMessageForThisCommitTest(unittest.TestCase): # ChangeLog is difficult to mock at current. def test_commit_message_for_this_commit(self): checkout = Checkout(None) - checkout.modified_changelogs = lambda git_commit, squash: ["ChangeLog1", "ChangeLog2"] + checkout.modified_changelogs = lambda git_commit: ["ChangeLog1", "ChangeLog2"] output = OutputCapture() expected_stderr = "Parsing ChangeLog: ChangeLog1\nParsing ChangeLog: ChangeLog2\n" commit_message = output.assert_outputs(self, checkout.commit_message_for_this_commit, - kwargs={"git_commit": None, "squash": False}, expected_stderr=expected_stderr) + kwargs={"git_commit": None}, expected_stderr=expected_stderr) self.assertEqual(commit_message.message(), self.expected_commit_message) @@ -163,13 +163,13 @@ class CheckoutTest(unittest.TestCase): def test_bug_id_for_this_commit(self): scm = Mock() checkout = Checkout(scm) - checkout.commit_message_for_this_commit = lambda git_commit, squash: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines()) - self.assertEqual(checkout.bug_id_for_this_commit(git_commit=None, squash=False), 36629) + checkout.commit_message_for_this_commit = lambda git_commit: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines()) + self.assertEqual(checkout.bug_id_for_this_commit(git_commit=None), 36629) def test_modified_changelogs(self): scm = Mock() scm.checkout_root = "/foo/bar" - scm.changed_files = lambda git_commit, squash: ["file1", "ChangeLog", "relative/path/ChangeLog"] + scm.changed_files = lambda git_commit: ["file1", "ChangeLog", "relative/path/ChangeLog"] checkout = Checkout(scm) expected_changlogs = ["/foo/bar/ChangeLog", "/foo/bar/relative/path/ChangeLog"] - self.assertEqual(checkout.modified_changelogs(git_commit=None, squash=False), expected_changlogs) + self.assertEqual(checkout.modified_changelogs(git_commit=None), expected_changlogs) diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/scm.py b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py index d7c621c..569558a 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/scm.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py @@ -35,7 +35,6 @@ import sys import shutil from webkitpy.common.system.executive import Executive, run_command, ScriptError -from webkitpy.common.system.user import User from webkitpy.common.system.deprecated_logging import error, log @@ -94,6 +93,17 @@ def commit_error_handler(error): Executive.default_error_handler(error) +class AuthenticationError(Exception): + def __init__(self, server_host): + self.server_host = server_host + + +class AmbiguousCommitError(Exception): + def __init__(self, num_local_commits, working_directory_is_clean): + self.num_local_commits = num_local_commits + self.working_directory_is_clean = working_directory_is_clean + + # SCM methods are expected to return paths relative to self.checkout_root. class SCM: def __init__(self, cwd): @@ -198,7 +208,7 @@ class SCM: def delete(self, path): self._subclass_must_implement() - def changed_files(self, git_commit=None, squash=None): + def changed_files(self, git_commit=None): self._subclass_must_implement() def changed_files_for_revision(self): @@ -213,7 +223,7 @@ class SCM: def display_name(self): self._subclass_must_implement() - def create_patch(self, git_commit=None, squash=None): + def create_patch(self, git_commit=None): self._subclass_must_implement() def committer_email_for_revision(self, revision): @@ -237,10 +247,7 @@ class SCM: def revert_files(self, file_paths): self._subclass_must_implement() - def should_squash(self, squash): - self._subclass_must_implement() - - def commit_with_message(self, message, username=None, git_commit=None, squash=None): + def commit_with_message(self, message, username=None, git_commit=None, force_squash=False): self._subclass_must_implement() def svn_commit_log(self, svn_revision): @@ -377,7 +384,7 @@ class SVN(SCM): parent, base = os.path.split(os.path.abspath(path)) return self.run(["svn", "delete", "--force", base], cwd=parent) - def changed_files(self, git_commit=None, squash=None): + def changed_files(self, git_commit=None): return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("ACDMR")) def changed_files_for_revision(self, revision): @@ -403,7 +410,7 @@ class SVN(SCM): return "svn" # FIXME: This method should be on Checkout. - def create_patch(self, git_commit=None, squash=None): + def create_patch(self, git_commit=None): """Returns a byte array (str()) representing the patch file. Patch files are effectively binary since they may contain files of multiple different encodings.""" @@ -477,22 +484,19 @@ class SVN(SCM): # FIXME: This should probably use cwd=self.checkout_root. self.run(['svn', 'revert'] + file_paths) - def should_squash(self, squash): - # SVN doesn't support the concept of squashing. - return False - - def commit_with_message(self, message, username=None, git_commit=None, squash=None): - # squash and git-commit are not used by SVN. + def commit_with_message(self, message, username=None, git_commit=None, force_squash=False): + # git-commit and force are not used by SVN. if self.dryrun: # Return a string which looks like a commit so that things which parse this output will succeed. return "Dry run, no commit.\nCommitted revision 0." + svn_commit_args = ["svn", "commit"] + if not username and not self.has_authorization_for_realm(): - username = User.prompt("%s login: " % self.svn_server_host, repeat=5) - if not username: - raise Exception("You need to specify the username on %s to perform the commit as." % self.svn_server_host) + raise AuthenticationError(self.svn_server_host) if username: svn_commit_args.extend(["--username", username]) + svn_commit_args.extend(["-m", message]) # FIXME: Should this use cwd=self.checkout_root? return self.run(svn_commit_args, error_handler=commit_error_handler) @@ -584,24 +588,25 @@ class Git(SCM): def delete(self, path): return self.run(["git", "rm", "-f", path]) - def _merge_base(self, git_commit, squash): + def _assert_synced(self): + if len(run_command(['git', 'rev-list', '--max-count=1', self.remote_branch_ref(), '^HEAD'])): + raise ScriptError(message="Not fully merged/rebased to %s. This branch needs to be synced first." % self.remote_branch_ref()) + + def merge_base(self, git_commit): if git_commit: - # FIXME: Calling code should turn commit ranges into a list of commit IDs - # and then treat each commit separately. + # Special-case HEAD.. to mean working-copy changes only. + if git_commit.upper() == 'HEAD..': + return 'HEAD' + if '..' not in git_commit: git_commit = git_commit + "^.." + git_commit return git_commit - if self.should_squash(squash): - return self.remote_merge_base() - - # FIXME: Non-squash behavior should match commit_with_message. It raises an error - # if there are working copy changes and --squash or --no-squash wasn't passed in. - # If --no-squash, then it should proceed with each local commit as a separate patch. - return 'HEAD' + self._assert_synced() + return self.remote_merge_base() - def changed_files(self, git_commit=None, squash=None): - status_command = ['git', 'diff', '-r', '--name-status', '-C', '-M', "--no-ext-diff", "--full-index", self._merge_base(git_commit, squash)] + def changed_files(self, git_commit=None): + status_command = ['git', 'diff', '-r', '--name-status', '-C', '-M', "--no-ext-diff", "--full-index", self.merge_base(git_commit)] return self.run_status_and_extract_filenames(status_command, self._status_regexp("ADM")) def _changes_files_for_commit(self, git_commit): @@ -633,12 +638,12 @@ class Git(SCM): def display_name(self): return "git" - def create_patch(self, git_commit=None, squash=None): + def create_patch(self, git_commit=None): """Returns a byte array (str()) representing the patch file. Patch files are effectively binary since they may contain files of multiple different encodings.""" # FIXME: This should probably use cwd=self.checkout_root - return self.run(['git', 'diff', '--binary', "--no-ext-diff", "--full-index", "-M", self._merge_base(git_commit, squash)], decode_output=False) + return self.run(['git', 'diff', '--binary', "--no-ext-diff", "--full-index", "-M", self.merge_base(git_commit)], decode_output=False) @classmethod def git_commit_from_svn_revision(cls, revision): @@ -679,63 +684,41 @@ class Git(SCM): def revert_files(self, file_paths): self.run(['git', 'checkout', 'HEAD'] + file_paths) - def _get_squash_error_message(self, num_local_commits): - working_directory_message = "" if self.working_directory_is_clean() else " and working copy changes" - return ("""There are %s local commits%s. Do one of the following: - 1) Use --squash or --no-squash - 2) git config webkit-patch.squash true/false - """ % (num_local_commits, working_directory_message)) - - def should_squash(self, squash): - if squash is None: - config_squash = Git.read_git_config('webkit-patch.squash') - if (config_squash and config_squash is not ""): - squash = config_squash.lower() == "true" - else: - # Only raise an error if there are actually multiple commits to squash. - num_local_commits = len(self.local_commits()) - if num_local_commits > 1 or (num_local_commits > 0 and not self.working_directory_is_clean()): - raise ScriptError(message=self._get_squash_error_message(num_local_commits)) - - if squash and self._remote_branch_has_extra_commits(): - raise ScriptError(message="Cannot use --squash when HEAD is not fully merged/rebased to %s. " - "This branch needs to be synced first." % self.remote_branch_ref()) + def _assert_can_squash(self, working_directory_is_clean): + squash = Git.read_git_config('webkit-patch.commit_should_always_squash') + should_squash = squash and squash.lower() == "true" - return squash + if not should_squash: + # Only warn if there are actually multiple commits to squash. + num_local_commits = len(self.local_commits()) + if num_local_commits > 1 or (num_local_commits > 0 and not working_directory_is_clean): + raise AmbiguousCommitError(num_local_commits, working_directory_is_clean) - def _remote_branch_has_extra_commits(self): - return len(run_command(['git', 'rev-list', '--max-count=1', self.remote_branch_ref(), '^HEAD'])) - - def commit_with_message(self, message, username=None, git_commit=None, squash=None): + def commit_with_message(self, message, username=None, git_commit=None, force_squash=False): # Username is ignored during Git commits. + working_directory_is_clean = self.working_directory_is_clean() + if git_commit: + # Special-case HEAD.. to mean working-copy changes only. + if git_commit.upper() == 'HEAD..': + if working_directory_is_clean: + raise ScriptError(message="The working copy is not modified. --git-commit=HEAD.. only commits working copy changes.") + self.commit_locally_with_message(message) + return self._commit_on_branch(message, 'HEAD') + # Need working directory changes to be committed so we can checkout the merge branch. - if not self.working_directory_is_clean(): + if not working_directory_is_clean: # FIXME: webkit-patch land will modify the ChangeLogs to correct the reviewer. # That will modify the working-copy and cause us to hit this error. - # The ChangeLog modification could be made to modify the existing local commit? + # The ChangeLog modification could be made to modify the existing local commit. raise ScriptError(message="Working copy is modified. Cannot commit individual git_commits.") return self._commit_on_branch(message, git_commit) - squash = self.should_squash(squash) - if squash: - self.run(['git', 'reset', '--soft', self.remote_branch_ref()]) - self.commit_locally_with_message(message) - elif not self.working_directory_is_clean(): - if not len(self.local_commits()): - # There are only working copy changes. Assume they should be committed. - self.commit_locally_with_message(message) - elif squash is None: - # The user didn't explicitly say to squash or not squash. There are local commits - # and working copy changes. Not clear what the user wants. - raise ScriptError(message="""There are local commits and working copy changes. Do one of the following: -1) Commit/revert working copy changes. -2) Use --squash or --no-squash -3) git config webkit-patch.squash true/false -""") - - # FIXME: This will commit all local commits, each with it's own message. We should restructure - # so that each local commit has the appropriate commit message based off it's ChangeLogs. + if not force_squash: + self._assert_can_squash(working_directory_is_clean) + self._assert_synced() + self.run(['git', 'reset', '--soft', self.remote_branch_ref()]) + self.commit_locally_with_message(message) return self.push_local_commits_to_server() def _commit_on_branch(self, message, git_commit): diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py b/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py index eaa3b46..852f838 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py @@ -44,7 +44,7 @@ import shutil from datetime import date from webkitpy.common.checkout.api import Checkout -from webkitpy.common.checkout.scm import detect_scm_system, SCM, SVN, CheckoutNeedsUpdate, commit_error_handler +from webkitpy.common.checkout.scm import detect_scm_system, SCM, SVN, CheckoutNeedsUpdate, commit_error_handler, AuthenticationError, AmbiguousCommitError from webkitpy.common.config.committers import Committer # FIXME: This should not be needed from webkitpy.common.net.bugzilla import Attachment # FIXME: This should not be needed from webkitpy.common.system.executive import Executive, run_command, ScriptError @@ -597,6 +597,10 @@ Q1dTBx0AAAB42itg4GlgYJjGwMDDyODMxMDw34GBgQEAJPQDJA== def test_commit_with_username(self): self._shared_test_commit_with_message("dbates@webkit.org") + def test_commit_without_authorization(self): + self.scm.has_authorization_for_realm = lambda: False + self.assertRaises(AuthenticationError, self._shared_test_commit_with_message) + def test_has_authorization_for_realm(self): scm = detect_scm_system(self.svn_checkout_path) fake_home_dir = tempfile.mkdtemp(suffix="fake_home_dir") @@ -867,24 +871,14 @@ class GitSVNTest(SCMTest): def test_commit_text_parsing(self): write_into_file_at_path('test_file', 'more test content') - self.scm.commit_locally_with_message("another test commit") commit_text = self.scm.commit_with_message("another test commit") self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '6') self.scm.dryrun = True write_into_file_at_path('test_file', 'still more test content') - self.scm.commit_locally_with_message("yet another test commit") commit_text = self.scm.commit_with_message("yet another test commit") self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '0') - def _one_local_commit_plus_working_copy_changes(self): - write_into_file_at_path('test_file_commit1', 'more test content') - run_command(['git', 'add', 'test_file_commit1']) - self.scm.commit_locally_with_message("another test commit") - - write_into_file_at_path('test_file_commit2', 'still more test content') - run_command(['git', 'add', 'test_file_commit2']) - def test_commit_with_message_working_copy_only(self): write_into_file_at_path('test_file_commit1', 'more test content') run_command(['git', 'add', 'test_file_commit1']) @@ -895,21 +889,18 @@ class GitSVNTest(SCMTest): svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) self.assertTrue(re.search(r'test_file_commit1', svn_log)) - def test_commit_with_message_squashed(self): - self._one_local_commit_plus_working_copy_changes() - scm = detect_scm_system(self.git_checkout_path) - commit_text = scm.commit_with_message("yet another test commit", squash=True) - - self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') - svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) - self.assertTrue(re.search(r'test_file_commit2', svn_log)) - self.assertTrue(re.search(r'test_file_commit1', svn_log)) - - def _two_local_commits(self): + def _one_local_commit(self): write_into_file_at_path('test_file_commit1', 'more test content') run_command(['git', 'add', 'test_file_commit1']) self.scm.commit_locally_with_message("another test commit") + def _one_local_commit_plus_working_copy_changes(self): + self._one_local_commit() + write_into_file_at_path('test_file_commit2', 'still more test content') + run_command(['git', 'add', 'test_file_commit2']) + + def _two_local_commits(self): + self._one_local_commit() write_into_file_at_path('test_file_commit2', 'still more test content') run_command(['git', 'add', 'test_file_commit2']) self.scm.commit_locally_with_message("yet another test commit") @@ -920,6 +911,17 @@ class GitSVNTest(SCMTest): self.scm.commit_locally_with_message("another test commit") self._two_local_commits() + def test_commit_with_message(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(AmbiguousCommitError, scm.commit_with_message, "yet another test commit") + commit_text = scm.commit_with_message("yet another test commit", force_squash=True) + + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit2', svn_log)) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) + def test_commit_with_message_git_commit(self): self._two_local_commits() @@ -943,51 +945,68 @@ class GitSVNTest(SCMTest): self.assertTrue(re.search(r'test_file_commit1', svn_log)) self.assertTrue(re.search(r'test_file_commit2', svn_log)) - def test_commit_with_message_multiple_local_commits(self): - self._two_local_commits() + def test_changed_files_working_copy_only(self): + self._one_local_commit_plus_working_copy_changes() scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.commit_with_message, ["another test commit"]) + commit_text = scm.commit_with_message("another test commit", git_commit="HEAD..") + self.assertFalse(re.search(r'test_file_commit1', svn_log)) + self.assertTrue(re.search(r'test_file_commit2', svn_log)) + + def test_commit_with_message_only_local_commit(self): + self._one_local_commit() + scm = detect_scm_system(self.git_checkout_path) + commit_text = scm.commit_with_message("another test commit") + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) def test_commit_with_message_multiple_local_commits_and_working_copy(self): self._two_local_commits() write_into_file_at_path('test_file_commit1', 'working copy change') scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.commit_with_message, ["another test commit"]) + + self.assertRaises(AmbiguousCommitError, scm.commit_with_message, "another test commit") + commit_text = scm.commit_with_message("another test commit", force_squash=True) + + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit2', svn_log)) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) def test_commit_with_message_git_commit_and_working_copy(self): self._two_local_commits() write_into_file_at_path('test_file_commit1', 'working copy change') scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.commit_with_message, ["another test commit", 'git_commit="HEAD^"']) + self.assertRaises(ScriptError, scm.commit_with_message, "another test commit", git_commit="HEAD^") - def test_commit_with_message_multiple_local_commits_no_squash(self): + def test_commit_with_message_multiple_local_commits_always_squash(self): self._two_local_commits() scm = detect_scm_system(self.git_checkout_path) - commit_text = scm.commit_with_message("yet another test commit", squash=False) + scm._assert_can_squash = lambda working_directory_is_clean: True + commit_text = scm.commit_with_message("yet another test commit") self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) self.assertTrue(re.search(r'test_file_commit2', svn_log)) - self.assertFalse(re.search(r'test_file_commit1', svn_log)) - - svn_log = run_command(['git', 'svn', 'log', '--limit=2', '--verbose']) self.assertTrue(re.search(r'test_file_commit1', svn_log)) - def test_commit_with_message_multiple_local_commits_squash(self): + def test_commit_with_message_multiple_local_commits(self): self._two_local_commits() scm = detect_scm_system(self.git_checkout_path) - commit_text = scm.commit_with_message("yet another test commit", squash=True) + self.assertRaises(AmbiguousCommitError, scm.commit_with_message, "yet another test commit") + commit_text = scm.commit_with_message("yet another test commit", force_squash=True) + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) self.assertTrue(re.search(r'test_file_commit2', svn_log)) self.assertTrue(re.search(r'test_file_commit1', svn_log)) - def test_commit_with_message_not_synced_squash(self): + def test_commit_with_message_not_synced(self): run_command(['git', 'checkout', '-b', 'my-branch', 'trunk~3']) self._two_local_commits() scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.commit_with_message, "another test commit", squash=True) + self.assertRaises(AmbiguousCommitError, scm.commit_with_message, "another test commit") + self.assertRaises(ScriptError, scm.commit_with_message, "another test commit", force_squash=True) def test_remote_branch_ref(self): self.assertEqual(self.scm.remote_branch_ref(), 'refs/remotes/trunk') @@ -1004,26 +1023,16 @@ class GitSVNTest(SCMTest): def test_create_patch_local_plus_working_copy(self): self._one_local_commit_plus_working_copy_changes() scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.create_patch) - - def test_create_patch_multiple_local_commits(self): - self._two_local_commits() - scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.create_patch) - - def test_create_patch_squashed(self): - self._one_local_commit_plus_working_copy_changes() - scm = detect_scm_system(self.git_checkout_path) - patch = scm.create_patch(squash=True) - self.assertTrue(re.search(r'test_file_commit2', patch)) + patch = scm.create_patch() self.assertTrue(re.search(r'test_file_commit1', patch)) + self.assertTrue(re.search(r'test_file_commit2', patch)) - def test_create_patch_not_squashed(self): + def test_create_patch(self): self._one_local_commit_plus_working_copy_changes() scm = detect_scm_system(self.git_checkout_path) - patch = scm.create_patch(squash=False) + patch = scm.create_patch() self.assertTrue(re.search(r'test_file_commit2', patch)) - self.assertFalse(re.search(r'test_file_commit1', patch)) + self.assertTrue(re.search(r'test_file_commit1', patch)) def test_create_patch_git_commit(self): self._two_local_commits() @@ -1040,26 +1049,25 @@ class GitSVNTest(SCMTest): self.assertTrue(re.search(r'test_file_commit2', patch)) self.assertTrue(re.search(r'test_file_commit1', patch)) - def test_create_patch_multiple_local_commits_no_squash(self): - self._two_local_commits() + def test_create_patch_working_copy_only(self): + self._one_local_commit_plus_working_copy_changes() scm = detect_scm_system(self.git_checkout_path) - patch = scm.create_patch(squash=False) - # FIXME: It's weird that with squash=False, create_patch/changed_files ignores local commits, - # but commit_with_message commits them. - self.assertTrue(patch == "") + patch = scm.create_patch(git_commit="HEAD..") + self.assertFalse(re.search(r'test_file_commit1', patch)) + self.assertTrue(re.search(r'test_file_commit2', patch)) - def test_create_patch_multiple_local_commits_squash(self): + def test_create_patch_multiple_local_commits(self): self._two_local_commits() scm = detect_scm_system(self.git_checkout_path) - patch = scm.create_patch(squash=True) + patch = scm.create_patch() self.assertTrue(re.search(r'test_file_commit2', patch)) self.assertTrue(re.search(r'test_file_commit1', patch)) - def test_create_patch_not_synced_squash(self): + def test_create_patch_not_synced(self): run_command(['git', 'checkout', '-b', 'my-branch', 'trunk~3']) self._two_local_commits() scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.create_patch, squash=True) + self.assertRaises(ScriptError, scm.create_patch) def test_create_binary_patch(self): # Create a git binary patch and check the contents. @@ -1090,26 +1098,9 @@ class GitSVNTest(SCMTest): def test_changed_files_local_plus_working_copy(self): self._one_local_commit_plus_working_copy_changes() scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.changed_files) - - def test_changed_files_multiple_local_commits(self): - self._two_local_commits() - scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.changed_files) - - def test_changed_files_squashed(self): - self._one_local_commit_plus_working_copy_changes() - scm = detect_scm_system(self.git_checkout_path) - files = scm.changed_files(squash=True) - self.assertTrue('test_file_commit2' in files) + files = scm.changed_files() self.assertTrue('test_file_commit1' in files) - - def test_changed_files_not_squashed(self): - self._one_local_commit_plus_working_copy_changes() - scm = detect_scm_system(self.git_checkout_path) - files = scm.changed_files(squash=False) self.assertTrue('test_file_commit2' in files) - self.assertFalse('test_file_commit1' in files) def test_changed_files_git_commit(self): self._two_local_commits() @@ -1126,26 +1117,25 @@ class GitSVNTest(SCMTest): self.assertTrue('test_file_commit1' in files) self.assertTrue('test_file_commit2' in files) - def test_changed_files_multiple_local_commits_no_squash(self): - self._two_local_commits() + def test_changed_files_working_copy_only(self): + self._one_local_commit_plus_working_copy_changes() scm = detect_scm_system(self.git_checkout_path) - files = scm.changed_files(squash=False) - # FIXME: It's weird that with squash=False, create_patch/changed_files ignores local commits, - # but commit_with_message commits them. - self.assertTrue(len(files) == 0) + files = scm.changed_files(git_commit="HEAD..") + self.assertFalse('test_file_commit1' in files) + self.assertTrue('test_file_commit2' in files) - def test_changed_files_multiple_local_commits_squash(self): + def test_changed_files_multiple_local_commits(self): self._two_local_commits() scm = detect_scm_system(self.git_checkout_path) - files = scm.changed_files(squash=True) + files = scm.changed_files() self.assertTrue('test_file_commit2' in files) self.assertTrue('test_file_commit1' in files) - def test_changed_files_not_synced_squash(self): + def test_changed_files_not_synced(self): run_command(['git', 'checkout', '-b', 'my-branch', 'trunk~3']) self._two_local_commits() scm = detect_scm_system(self.git_checkout_path) - self.assertRaises(ScriptError, scm.changed_files, squash=True) + self.assertRaises(ScriptError, scm.changed_files) def test_changed_files(self): self._shared_test_changed_files() diff --git a/WebKitTools/Scripts/webkitpy/common/config/committers.py b/WebKitTools/Scripts/webkitpy/common/config/committers.py index 7b69815..e5f0a50 100644 --- a/WebKitTools/Scripts/webkitpy/common/config/committers.py +++ b/WebKitTools/Scripts/webkitpy/common/config/committers.py @@ -64,6 +64,7 @@ class Reviewer(Committer): committers_unable_to_review = [ Committer("Aaron Boodman", "aa@chromium.org", "aboodman"), + Committer("Abhishek Arya", "inferno@chromium.org", "inferno-sec"), Committer("Adam Langley", "agl@chromium.org", "agl"), Committer("Albert J. Wong", "ajwong@chromium.org"), Committer("Alejandro G. Castro", ["alex@igalia.com", "alex@webkit.org"]), @@ -76,7 +77,6 @@ committers_unable_to_review = [ Committer("Andy Estes", "aestes@apple.com", "estes"), Committer("Anthony Ricaud", "rik@webkit.org", "rik"), Committer("Anton Muhin", "antonm@chromium.org", "antonm"), - Committer("Antonio Gomes", "tonikitoo@webkit.org", "tonikitoo"), Committer("Ben Murdoch", "benm@google.com", "benm"), Committer("Benjamin C Meyer", ["ben@meyerhome.net", "ben@webkit.org"], "icefox"), Committer("Benjamin Otte", ["otte@gnome.org", "otte@webkit.org"], "otte"), @@ -115,6 +115,7 @@ committers_unable_to_review = [ Committer("Jakub Wieczorek", "jwieczorek@webkit.org", "fawek"), Committer("James Hawkins", ["jhawkins@chromium.org", "jhawkins@google.com"], "jhawkins"), Committer("James Robinson", ["jamesr@chromium.org", "jamesr@google.com"], "jamesr"), + Committer("Jay Civelli", "jcivelli@chromium.org", "jcivelli"), Committer("Jens Alfke", ["snej@chromium.org", "jens@apple.com"]), Committer("Jer Noble", "jer.noble@apple.com", "jernoble"), Committer("Jeremy Moskovich", ["playmobil@google.com", "jeremy@chromium.org"], "jeremymos"), @@ -149,6 +150,7 @@ committers_unable_to_review = [ Committer("Mike Thole", ["mthole@mikethole.com", "mthole@apple.com"]), Committer("Mikhail Naganov", "mnaganov@chromium.org"), Committer("MORITA Hajime", "morrita@google.com", "morrita"), + Committer("Nico Weber", ["thakis@chromium.org", "thakis@google.com"], "thakis"), Committer("Pam Greene", "pam@chromium.org", "pamg"), Committer("Peter Kasting", ["pkasting@google.com", "pkasting@chromium.org"], "pkasting"), Committer("Philippe Normand", ["pnormand@igalia.com", "philn@webkit.org"], "philn-tp"), @@ -193,6 +195,7 @@ reviewers_list = [ Reviewer("Alice Liu", "alice.liu@apple.com", "aliu"), Reviewer("Alp Toker", ["alp@nuanti.com", "alp@atoker.com", "alp@webkit.org"], "alp"), Reviewer("Anders Carlsson", ["andersca@apple.com", "acarlsson@apple.com"], "andersca"), + Reviewer("Antonio Gomes", "tonikitoo@webkit.org", "tonikitoo"), Reviewer("Antti Koivisto", ["koivisto@iki.fi", "antti@apple.com"], "anttik"), Reviewer("Ariya Hidayat", ["ariya.hidayat@gmail.com", "ariya@webkit.org"], "ariya"), Reviewer("Beth Dakin", "bdakin@apple.com", "dethbakin"), @@ -254,7 +257,7 @@ reviewers_list = [ Reviewer("Steve Falkenburg", "sfalken@apple.com", "sfalken"), Reviewer("Tim Omernick", "timo@apple.com"), Reviewer("Timothy Hatcher", ["timothy@hatcher.name", "timothy@apple.com"], "xenon"), - Reviewer(u"Tor Arne Vestb\u00f8", "vestbo@webkit.org", "torarne"), + Reviewer(u"Tor Arne Vestb\u00f8", ["vestbo@webkit.org", "tor.arne.vestbo@nokia.com"], "torarne"), Reviewer("Vicki Murley", "vicki@apple.com"), Reviewer("Xan Lopez", ["xan.lopez@gmail.com", "xan@gnome.org", "xan@webkit.org"], "xan"), Reviewer("Yury Semikhatsky", "yurys@chromium.org", "yurys"), diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py index eccda3a..0c6a313 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py +++ b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py @@ -73,6 +73,6 @@ class Rietveld(object): # Use RealMain instead of calling upload from the commandline so that # we can pass in the diff ourselves. Otherwise, upload will just use - # git diff for git checkouts, which doesn't respect --squash and --git-commit. + # git diff for git checkouts, which doesn't respect --git-commit. issue, patchset = upload.RealMain(args, data=diff) return issue diff --git a/WebKitTools/Scripts/webkitpy/common/system/executive.py b/WebKitTools/Scripts/webkitpy/common/system/executive.py index 9c5889b..6088680 100644 --- a/WebKitTools/Scripts/webkitpy/common/system/executive.py +++ b/WebKitTools/Scripts/webkitpy/common/system/executive.py @@ -183,7 +183,7 @@ class Executive(object): # According to http://docs.python.org/library/os.html # os.kill isn't available on Windows. python 2.5.5 os.kill appears # to work in cygwin, however it occasionally raises EAGAIN. - retries_left = 3 if sys.platform == "cygwin" else 1 + retries_left = 10 if sys.platform == "cygwin" else 1 while retries_left > 0: try: retries_left -= 1 diff --git a/WebKitTools/Scripts/webkitpy/common/system/user.py b/WebKitTools/Scripts/webkitpy/common/system/user.py index b4df3cb..9444c00 100644 --- a/WebKitTools/Scripts/webkitpy/common/system/user.py +++ b/WebKitTools/Scripts/webkitpy/common/system/user.py @@ -51,7 +51,7 @@ except ImportError: class User(object): - # FIXME: These are @classmethods because scm.py and bugzilla.py don't have a Tool object (thus no User instance). + # FIXME: These are @classmethods because bugzilla.py doesn't have a Tool object (thus no User instance). @classmethod def prompt(cls, message, repeat=1, raw_input=raw_input): response = None diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py index a2e2091..6364511 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py @@ -429,7 +429,8 @@ class TestShellThread(threading.Thread): # previous run will be copied into the baseline.) image_hash = test_info.image_hash() if (image_hash and - (self._test_args.new_baseline or self._test_args.reset_results)): + (self._test_args.new_baseline or self._test_args.reset_results or + not self._options.pixel_tests)): image_hash = "" start = time.time() crash, timeout, actual_checksum, output, error = \ diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_layout_results_generator.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_layout_results_generator.py index bb214f7..c0525ea 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_layout_results_generator.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_layout_results_generator.py @@ -1,4 +1,3 @@ -#!/usr/bin/env python # Copyright (C) 2010 Google Inc. All rights reserved. # # Redistribution and use in source and binary forms, with or without @@ -35,7 +34,8 @@ from webkitpy.layout_tests.layout_package import test_expectations from webkitpy.layout_tests.layout_package import test_failures import webkitpy.thirdparty.simplejson as simplejson -class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGenerator): + +class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGeneratorBase): """A JSON results generator for layout tests.""" LAYOUT_TESTS_PATH = "LayoutTests" @@ -44,6 +44,16 @@ class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGenerator): WONTFIX = "wontfixCounts" DEFERRED = "deferredCounts" + # Note that we omit test_expectations.FAIL from this list because + # it should never show up (it's a legacy input expectation, never + # an output expectation). + FAILURE_TO_CHAR = {test_expectations.CRASH: "C", + test_expectations.TIMEOUT: "T", + test_expectations.IMAGE: "I", + test_expectations.TEXT: "F", + test_expectations.MISSING: "O", + test_expectations.IMAGE_PLUS_TEXT: "Z"} + def __init__(self, port, builder_name, build_name, build_number, results_file_base_path, builder_base_url, test_timings, expectations, result_summary, all_tests): @@ -53,20 +63,14 @@ class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGenerator): Args: result_summary: ResultsSummary object storing the summary of the test results. - (see the comment of JSONResultsGenerator.__init__ for other Args) """ + super(JSONLayoutResultsGenerator, self).__init__( + builder_name, build_name, build_number, results_file_base_path, + builder_base_url, {}, port.test_repository_paths()) + self._port = port - self._builder_name = builder_name - self._build_name = build_name - self._build_number = build_number - self._builder_base_url = builder_base_url - self._results_file_path = os.path.join(results_file_base_path, - self.RESULTS_FILENAME) self._expectations = expectations - # We don't use self._skipped_tests and self._passed_tests as we - # override _InsertFailureSummaries. - # We want relative paths to LayoutTest root for JSON output. path_to_name = self._get_path_relative_to_layout_test_root self._result_summary = result_summary @@ -77,9 +81,8 @@ class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGenerator): self._test_timings = dict( (path_to_name(test_tuple.filename), test_tuple.test_run_time) for test_tuple in test_timings) - self._svn_repositories = port.test_repository_paths() - self._generate_json_output() + self.generate_json_output() def _get_path_relative_to_layout_test_root(self, test): """Returns the path of the test relative to the layout test root. @@ -102,6 +105,27 @@ class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGenerator): return relativePath.replace('\\', '/') # override + def _get_test_timing(self, test_name): + if test_name in self._test_timings: + # Floor for now to get time in seconds. + return int(self._test_timings[test_name]) + return 0 + + # override + def _get_failed_test_names(self): + return set(self._failures.keys()) + + # override + def _get_result_type_char(self, test_name): + if test_name not in self._all_tests: + return self.NO_DATA_RESULT + + if test_name in self._failures: + return self.FAILURE_TO_CHAR[self._failures[test_name]] + + return self.PASS_RESULT + + # override def _convert_json_to_current_version(self, results_json): archive_version = None if self.VERSION_KEY in results_json: diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py index 1cf1b95..595fc2b 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py @@ -1,4 +1,3 @@ -#!/usr/bin/env python # Copyright (C) 2010 Google Inc. All rights reserved. # # Redistribution and use in source and binary forms, with or without @@ -38,16 +37,27 @@ import time import urllib2 import xml.dom.minidom -from webkitpy.common.checkout import scm -from webkitpy.common.system.executive import ScriptError -from webkitpy.layout_tests.layout_package import test_expectations import webkitpy.thirdparty.simplejson as simplejson -_log = logging.getLogger("webkitpy.layout_tests.layout_package." - "json_results_generator") +# A JSON results generator for generic tests. +# FIXME: move this code out of the layout_package directory. +_log = logging.getLogger("webkitpy.layout_tests.layout_package.json_results_generator") -class JSONResultsGenerator(object): + +class TestResult(object): + """A simple class that represents a single test result.""" + def __init__(self, name, failed=False, skipped=False, elapsed_time=0): + self.name = name + self.failed = failed + self.skipped = skipped + self.time = elapsed_time + + def fixable(self): + return self.failed or self.skipped + + +class JSONResultsGeneratorBase(object): """A JSON results generator for generic tests.""" MAX_NUMBER_OF_BUILD_RESULTS_TO_LOG = 750 @@ -57,6 +67,7 @@ class JSONResultsGenerator(object): JSON_SUFFIX = ");" PASS_RESULT = "P" SKIP_RESULT = "X" + FAIL_RESULT = "F" NO_DATA_RESULT = "N" VERSION = 3 VERSION_KEY = "version" @@ -70,22 +81,11 @@ class JSONResultsGenerator(object): FIXABLE = "fixableCounts" ALL_FIXABLE_COUNT = "allFixableCount" - # Note that we omit test_expectations.FAIL from this list because - # it should never show up (it's a legacy input expectation, never - # an output expectation). - FAILURE_TO_CHAR = {test_expectations.CRASH: "C", - test_expectations.TIMEOUT: "T", - test_expectations.IMAGE: "I", - test_expectations.TEXT: "F", - test_expectations.MISSING: "O", - test_expectations.IMAGE_PLUS_TEXT: "Z"} - FAILURE_CHARS = FAILURE_TO_CHAR.values() - RESULTS_FILENAME = "results.json" - def __init__(self, port, builder_name, build_name, build_number, + def __init__(self, builder_name, build_name, build_number, results_file_base_path, builder_base_url, - test_timings, failures, passed_tests, skipped_tests, all_tests): + test_results_map, svn_repositories=None): """Modifies the results.json file. Grabs it off the archive directory if it is not found locally. @@ -96,12 +96,11 @@ class JSONResultsGenerator(object): results_file_base_path: Absolute path to the directory containing the results json file. builder_base_url: the URL where we have the archived test results. - test_timings: Map of test name to a test_run-time. - failures: Map of test name to a failure type (of test_expectations). - passed_tests: A set containing all the passed tests. - skipped_tests: A set containing all the skipped tests. - all_tests: List of all the tests that were run. This should not - include skipped tests. + If this is None no archived results will be retrieved. + test_results_map: A dictionary that maps test_name to TestResult. + svn_repositories: A (json_field_name, svn_path) pair for SVN + repositories that tests rely on. The SVN revision will be + included in the JSON with the given json_field_name. """ self._builder_name = builder_name self._build_name = build_name @@ -109,31 +108,106 @@ class JSONResultsGenerator(object): self._builder_base_url = builder_base_url self._results_file_path = os.path.join(results_file_base_path, self.RESULTS_FILENAME) - self._test_timings = test_timings - self._failures = failures - self._passed_tests = passed_tests - self._skipped_tests = skipped_tests - self._all_tests = all_tests - self._svn_repositories = port.test_repository_paths() - self._generate_json_output() + self._test_results_map = test_results_map + self._test_results = test_results_map.values() + + self._svn_repositories = svn_repositories + if not self._svn_repositories: + self._svn_repositories = {} + + self._json = None - def _generate_json_output(self): + def generate_json_output(self): """Generates the JSON output file.""" - json = self._get_json() - if json: + if not self._json: + self._json = self.get_json() + if self._json: + # Specify separators in order to get compact encoding. + json_data = simplejson.dumps(self._json, separators=(',', ':')) + json_string = self.JSON_PREFIX + json_data + self.JSON_SUFFIX + results_file = codecs.open(self._results_file_path, "w", "utf-8") - results_file.write(json) + results_file.write(json_string) results_file.close() + def get_json(self): + """Gets the results for the results.json file.""" + if self._json: + return self._json + + results_json, error = self._get_archived_json_results() + if error: + # If there was an error don't write a results.json + # file at all as it would lose all the information on the bot. + _log.error("Archive directory is inaccessible. Not modifying " + "or clobbering the results.json file: " + str(error)) + return None + + builder_name = self._builder_name + if results_json and builder_name not in results_json: + _log.debug("Builder name (%s) is not in the results.json file." + % builder_name) + + self._convert_json_to_current_version(results_json) + + if builder_name not in results_json: + results_json[builder_name] = ( + self._create_results_for_builder_json()) + + results_for_builder = results_json[builder_name] + + self._insert_generic_metadata(results_for_builder) + + self._insert_failure_summaries(results_for_builder) + + # Update the all failing tests with result type and time. + tests = results_for_builder[self.TESTS] + all_failing_tests = self._get_failed_test_names() + all_failing_tests.update(tests.iterkeys()) + for test in all_failing_tests: + self._insert_test_time_and_result(test, tests) + + self._json = results_json + return self._json + + def _get_test_timing(self, test_name): + """Returns test timing data (elapsed time) in second + for the given test_name.""" + if test_name in self._test_results_map: + # Floor for now to get time in seconds. + return int(self._test_results_map[test_name].time) + return 0 + + def _get_failed_test_names(self): + """Returns a set of failed test names.""" + return set([r.name for r in self._test_results if r.failed]) + + def _get_result_type_char(self, test_name): + """Returns a single char (e.g. SKIP_RESULT, FAIL_RESULT, + PASS_RESULT, NO_DATA_RESULT, etc) that indicates the test result + for the given test_name. + """ + if test_name not in self._test_results_map: + return JSONResultsGenerator.NO_DATA_RESULT + + test_result = self._test_results_map[test_name] + if test_result.skipped: + return JSONResultsGenerator.SKIP_RESULT + if test_result.failed: + return JSONResultsGenerator.FAIL_RESULT + + return JSONResultsGenerator.PASS_RESULT + # FIXME: Callers should use scm.py instead. + # FIXME: Identify and fix the run-time errors that were observed on Windows + # chromium buildbot when we had updated this code to use scm.py once before. def _get_svn_revision(self, in_directory): """Returns the svn revision for the given directory. Args: in_directory: The directory where svn is to be run. """ - if os.path.exists(os.path.join(in_directory, '.svn')): # Note: Not thread safe: http://bugs.python.org/issue2320 output = subprocess.Popen(["svn", "info", "--xml"], @@ -196,76 +270,34 @@ class JSONResultsGenerator(object): return results_json, error - def _get_json(self): - """Gets the results for the results.json file.""" - results_json, error = self._get_archived_json_results() - if error: - # If there was an error don't write a results.json - # file at all as it would lose all the information on the bot. - _log.error("Archive directory is inaccessible. Not modifying " - "or clobbering the results.json file: " + str(error)) - return None - - builder_name = self._builder_name - if results_json and builder_name not in results_json: - _log.debug("Builder name (%s) is not in the results.json file." - % builder_name) - - self._convert_json_to_current_version(results_json) - - if builder_name not in results_json: - results_json[builder_name] = ( - self._create_results_for_builder_json()) - - results_for_builder = results_json[builder_name] - - self._insert_generic_metadata(results_for_builder) - - self._insert_failure_summaries(results_for_builder) - - # Update the all failing tests with result type and time. - tests = results_for_builder[self.TESTS] - all_failing_tests = set(self._failures.iterkeys()) - all_failing_tests.update(tests.iterkeys()) - for test in all_failing_tests: - self._insert_test_time_and_result(test, tests) - - # Specify separators in order to get compact encoding. - results_str = simplejson.dumps(results_json, separators=(',', ':')) - return self.JSON_PREFIX + results_str + self.JSON_SUFFIX - def _insert_failure_summaries(self, results_for_builder): """Inserts aggregate pass/failure statistics into the JSON. - This method reads self._skipped_tests, self._passed_tests and - self._failures and inserts FIXABLE, FIXABLE_COUNT and ALL_FIXABLE_COUNT - entries. + This method reads self._test_results and generates + FIXABLE, FIXABLE_COUNT and ALL_FIXABLE_COUNT entries. Args: results_for_builder: Dictionary containing the test results for a single builder. """ - # Insert the number of tests that failed. + # Insert the number of tests that failed or skipped. + fixable_count = len([r for r in self._test_results if r.fixable()]) self._insert_item_into_raw_list(results_for_builder, - len(set(self._failures.keys()) | self._skipped_tests), - self.FIXABLE_COUNT) + fixable_count, self.FIXABLE_COUNT) # Create a pass/skip/failure summary dictionary. entry = {} - entry[self.SKIP_RESULT] = len(self._skipped_tests) - entry[self.PASS_RESULT] = len(self._passed_tests) - get = entry.get - for failure_type in self._failures.values(): - failure_char = self.FAILURE_TO_CHAR[failure_type] - entry[failure_char] = get(failure_char, 0) + 1 + for test_name in self._test_results_map.iterkeys(): + result_char = self._get_result_type_char(test_name) + entry[result_char] = entry.get(result_char, 0) + 1 # Insert the pass/skip/failure summary dictionary. self._insert_item_into_raw_list(results_for_builder, entry, self.FIXABLE) # Insert the number of all the tests that are supposed to pass. + all_test_count = len(self._test_results) self._insert_item_into_raw_list(results_for_builder, - len(self._skipped_tests | self._all_tests), - self.ALL_FIXABLE_COUNT) + all_test_count, self.ALL_FIXABLE_COUNT) def _insert_item_into_raw_list(self, results_for_builder, item, key): """Inserts the item into the list with the given key in the results for @@ -331,18 +363,8 @@ class JSONResultsGenerator(object): tests: Dictionary containing test result entries. """ - result = JSONResultsGenerator.PASS_RESULT - time = 0 - - if test_name not in self._all_tests: - result = JSONResultsGenerator.NO_DATA_RESULT - - if test_name in self._failures: - result = self.FAILURE_TO_CHAR[self._failures[test_name]] - - if test_name in self._test_timings: - # Floor for now to get time in seconds. - time = int(self._test_timings[test_name]) + result = self._get_result_type_char(test_name) + time = self._get_test_timing(test_name) if test_name not in tests: tests[test_name] = self._create_results_and_times_json() @@ -420,3 +442,59 @@ class JSONResultsGenerator(object): """Returns whether all the results are of the given type (e.g. all passes).""" return len(results) == 1 and results[0][1] == type + + +# A wrapper class for JSONResultsGeneratorBase. +# Note: There's a script outside the WebKit codebase calling this script. +# FIXME: Please keep the interface until the other script is cleaned up. +# (http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/webkitpy/layout_tests/test_output_xml_to_json.py?view=markup) +class JSONResultsGenerator(JSONResultsGeneratorBase): + # The flag is for backward compatibility. + output_json_in_init = True + + def __init__(self, port, builder_name, build_name, build_number, + results_file_base_path, builder_base_url, + test_timings, failures, passed_tests, skipped_tests, all_tests): + """Generates a JSON results file. + + Args + builder_name: the builder name (e.g. Webkit). + build_name: the build name (e.g. webkit-rel). + build_number: the build number. + results_file_base_path: Absolute path to the directory containing the + results json file. + builder_base_url: the URL where we have the archived test results. + test_timings: Map of test name to a test_run-time. + failures: Map of test name to a failure type (of test_expectations). + passed_tests: A set containing all the passed tests. + skipped_tests: A set containing all the skipped tests. + all_tests: List of all the tests that were run. This should not + include skipped tests. + """ + + # Create a map of (name, TestResult). + test_results_map = dict() + get = test_results_map.get + for (test, time) in test_timings.iteritems(): + test_results_map[test] = TestResult(test, elapsed_time=time) + for test in failures.iterkeys(): + test_results_map[test] = test_result = get(test, TestResult(test)) + test_result.failed = True + for test in skipped_tests: + test_results_map[test] = test_result = get(test, TestResult(test)) + test_result.skipped = True + for test in passed_tests: + test_results_map[test] = test_result = get(test, TestResult(test)) + test_result.failed = False + test_result.skipped = False + for test in all_tests: + if test not in test_results_map: + test_results_map[test] = TestResult(test) + + super(JSONResultsGenerator, self).__init__( + builder_name, build_name, build_number, + results_file_base_path, builder_base_url, test_results_map, + svn_repositories=port.test_repository_paths()) + + if self.__class__.output_json_in_init: + self.generate_json_output() diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py new file mode 100644 index 0000000..0a60cc7 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py @@ -0,0 +1,126 @@ +# Copyright (C) 2010 Google Inc. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +"""Unit tests for json_results_generator.py.""" + +import unittest +import optparse +import random +import shutil +import tempfile + +from webkitpy.layout_tests.layout_package import json_results_generator +from webkitpy.layout_tests.layout_package import test_expectations +from webkitpy.layout_tests import port + + +class JSONGeneratorTest(unittest.TestCase): + def setUp(self): + json_results_generator.JSONResultsGenerator.output_json_in_init = False + self.builder_name = 'DUMMY_BUILDER_NAME' + self.build_name = 'DUMMY_BUILD_NAME' + self.build_number = 'DUMMY_BUILDER_NUMBER' + + def _test_json_generation(self, passed_tests, failed_tests, skipped_tests): + # Make sure we have sets (rather than lists). + passed_tests = set(passed_tests) + skipped_tests = set(skipped_tests) + tests_list = passed_tests | set(failed_tests.keys()) + test_timings = {} + for test in tests_list: + test_timings[test] = float(random.randint(1, 10)) + + port_obj = port.get(None) + + # Generate a JSON file. + generator = json_results_generator.JSONResultsGenerator(port_obj, + self.builder_name, self.build_name, self.build_number, + '', + None, # don't fetch past json results archive + test_timings, + failed_tests, + passed_tests, + skipped_tests, + tests_list) + + json = generator.get_json() + + # Aliasing to a short name for better access to its constants. + JRG = json_results_generator.JSONResultsGenerator + + self.assertTrue(JRG.VERSION_KEY in json) + self.assertTrue(self.builder_name in json) + + buildinfo = json[self.builder_name] + self.assertTrue(JRG.FIXABLE in buildinfo) + self.assertTrue(JRG.TESTS in buildinfo) + self.assertTrue(len(buildinfo[JRG.BUILD_NUMBERS]) == 1) + self.assertTrue(buildinfo[JRG.BUILD_NUMBERS][0] == self.build_number) + + if tests_list or skipped_tests: + fixable = buildinfo[JRG.FIXABLE][0] + if passed_tests: + self.assertTrue(fixable[JRG.PASS_RESULT] == len(passed_tests)) + else: + self.assertTrue(JRG.PASS_RESULT not in fixable or + fixable[JRG.PASS_RESULT] == 0) + if skipped_tests: + self.assertTrue(fixable[JRG.SKIP_RESULT] == len(skipped_tests)) + else: + self.assertTrue(JRG.SKIP_RESULT not in fixable or + fixable[JRG.SKIP_RESULT] == 0) + + if failed_tests: + tests = buildinfo[JRG.TESTS] + for test_name, failure in failed_tests.iteritems(): + self.assertTrue(test_name in tests) + test = tests[test_name] + self.assertTrue(test[JRG.RESULTS][0][0] == 1) + self.assertTrue(test[JRG.RESULTS][0][1] == JRG.FAIL_RESULT) + self.assertTrue(test[JRG.TIMES][0][0] == 1) + self.assertTrue(test[JRG.TIMES][0][1] == + int(test_timings[test_name])) + + fixable_count = len(skipped_tests) + len(failed_tests.keys()) + if skipped_tests or failed_tests: + self.assertTrue(buildinfo[JRG.FIXABLE_COUNT][0] == fixable_count) + + def test_json_generation(self): + reason = test_expectations.TEXT + + self._test_json_generation([], {}, []) + self._test_json_generation(['A', 'B'], {}, []) + self._test_json_generation([], {'A': reason, 'B': reason}, []) + self._test_json_generation([], {}, ['A', 'B']) + self._test_json_generation(['A'], {'B': reason, 'C': reason}, []) + self._test_json_generation([], {'A': reason, 'B': reason}, ['C', 'D']) + self._test_json_generation(['A', 'B', 'C'], {'D': reason}, ['E', 'F']) + + +if __name__ == '__main__': + unittest.main() diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py index e73579f..e1b23ac 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py @@ -128,13 +128,16 @@ class Port(object): return expected_text != actual_text def diff_image(self, expected_filename, actual_filename, - diff_filename=None): + diff_filename=None, tolerance=0): """Compare two image files and produce a delta image file. Return True if the two files are different, False if they are the same. Also produce a delta image of the two images and write that into |diff_filename| if it is not None. + |tolerance| should be a percentage value (0.0 - 100.0). + If it is omitted, the port default tolerance value is used. + While this is a generic routine, we include it in the Port interface so that it can be overriden for testing purposes.""" executable = self._path_to_image_diff() diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py index 9032a24..41b2ba0 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py @@ -32,6 +32,8 @@ import logging import os import signal +import webkit + from webkitpy.layout_tests.port.webkit import WebKitPort _log = logging.getLogger("webkitpy.layout_tests.port.qt") @@ -90,3 +92,8 @@ class QtPort(WebKitPort): def _path_to_driver(self): return self._build_path('bin/DumpRenderTree') + + def setup_environ_for_server(self): + env = webkit.WebKitPort.setup_environ_for_server(self) + env['QTWEBKIT_PLUGIN_PATH'] = self._build_path('lib/plugins') + return env diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py index e6d4c99..6eef54e 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py @@ -60,7 +60,7 @@ class TestPort(base.Port): return False def diff_image(self, expected_filename, actual_filename, - diff_filename=None): + diff_filename=None, tolerance=0): return False def diff_text(self, expected_text, actual_text, diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py index 2097ce7..e1151a6 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py @@ -116,10 +116,13 @@ class WebKitPort(base.Port): return True def diff_image(self, expected_filename, actual_filename, - diff_filename=None): + diff_filename=None, tolerance=0.1): """Return True if the two files are different. Also write a delta image of the two images into |diff_filename| if it is not None.""" + # FIXME: either expose the tolerance argument as a command-line + # parameter, or make it go away and always use exact matches. + # Handle the case where the test didn't actually generate an image. actual_length = os.stat(actual_filename).st_size if actual_length == 0: @@ -127,13 +130,11 @@ class WebKitPort(base.Port): shutil.copyfile(actual_filename, expected_filename) return True - sp = self._diff_image_request(expected_filename, actual_filename) + sp = self._diff_image_request(expected_filename, actual_filename, tolerance) return self._diff_image_reply(sp, expected_filename, diff_filename) - def _diff_image_request(self, expected_filename, actual_filename): - # FIXME: either expose the tolerance argument as a command-line - # parameter, or make it go away and aways use exact matches. - command = [self._path_to_image_diff(), '--tolerance', '0.1'] + def _diff_image_request(self, expected_filename, actual_filename, tolerance): + command = [self._path_to_image_diff(), '--tolerance', str(tolerance)] sp = server_process.ServerProcess(self, 'ImageDiff', command) actual_length = os.stat(actual_filename).st_size @@ -378,7 +379,7 @@ class WebKitDriver(base.Driver): command += [self._port._path_to_driver(), '-'] if self._image_path: command.append('--pixel-tests') - environment = os.environ + environment = self._port.setup_environ_for_server() environment['DYLD_FRAMEWORK_PATH'] = self._port._build_path() self._server_process = server_process.ServerProcess(self._port, "DumpRenderTree", command, environment) diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py index 35f32d4..fa4df9b 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py @@ -916,8 +916,14 @@ def main(): option_parser.add_option('-d', '--html_directory', default='', - help=('The directory that stores the results for' - ' rebaselining comparison.')) + help=('The directory that stores the results for ' + 'rebaselining comparison.')) + + option_parser.add_option('', '--use_drt', + action='store_true', + default=False, + help=('Use ImageDiff from DumpRenderTree instead ' + 'of image_diff for pixel tests.')) option_parser.add_option('', '--target-platform', default='chromium', diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py index a4a92c7..41aab62 100755 --- a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py @@ -1619,6 +1619,8 @@ def parse_args(args=None): help="run all tests in parallel"), # FIXME: Need --exit-after-n-failures N # Exit after the first N failures instead of running all tests + # FIXME: Need --exit-after-n-crashes N + # Exit after the first N crashes instead of running all tests # FIXME: consider: --iterations n # Number of times to run the set of tests (e.g. ABCABCABC) optparse.make_option("--print-last-failures", action="store_true", diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py b/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py index fe73a7b..65f8f3a 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py @@ -192,7 +192,7 @@ class ImageDiff(test_type_base.TestTypeBase): return failures def diff_files(self, port, file1, file2): - """Diff two image files. + """Diff two image files exactly. Args: file1, file2: full paths of the files to compare. @@ -202,6 +202,6 @@ class ImageDiff(test_type_base.TestTypeBase): False otherwise. """ try: - return port.diff_image(file1, file2) + return port.diff_image(file1, file2, None, 0) except ValueError, e: return True diff --git a/WebKitTools/Scripts/webkitpy/style/optparser.py b/WebKitTools/Scripts/webkitpy/style/optparser.py index bb4788a..3ba0fae 100644 --- a/WebKitTools/Scripts/webkitpy/style/optparser.py +++ b/WebKitTools/Scripts/webkitpy/style/optparser.py @@ -147,8 +147,7 @@ class CommandOptionValues(object): git_commit=None, is_verbose=False, min_confidence=1, - output_format="emacs", - squash=False): + output_format="emacs"): if filter_rules is None: filter_rules = [] @@ -167,7 +166,6 @@ class CommandOptionValues(object): self.is_verbose = is_verbose self.min_confidence = min_confidence self.output_format = output_format - self.squash = squash # Useful for unit testing. def __eq__(self, other): @@ -182,8 +180,6 @@ class CommandOptionValues(object): return False if self.output_format != other.output_format: return False - if self.squash != other.squash: - return False return True @@ -218,8 +214,6 @@ class ArgumentPrinter(object): flags['filter'] = ",".join(filter_rules) if options.git_commit: flags['git-commit'] = options.git_commit - if options.squash: - flags['squash'] = options.squash flag_string = '' # Alphabetizing lets us unit test this method. @@ -309,7 +303,7 @@ class ArgumentParser(object): parser.add_option("-f", "--filter-rules", metavar="RULES", dest="filter_value", help=filter_help) - git_commit_help = ("check all changes in the given git commit. " + git_commit_help = ("check all changes in the given commit. " "Use 'commit_id..' to check all changes after commmit_id") parser.add_option("-g", "--git-diff", "--git-commit", metavar="COMMIT", dest="git_commit", help=git_commit_help,) @@ -330,14 +324,6 @@ class ArgumentParser(object): dest="output_format", default=default_output_format, help=output_format_help) - squash_help = ("All diffs from the remote branch are checked." - "If excluded, prompts whether to squash when there are multiple commits.") - parser.add_option("-s", "--squash", action="store_true", dest="squash", help=squash_help) - - squash_help = ("Only working copy diffs are checked." - "If excluded, prompts whether to squash when there are multiple commits.") - parser.add_option("--no-squash", action="store_false", dest="squash", help=squash_help) - verbose_help = "enable verbose logging." parser.add_option("-v", "--verbose", dest="is_verbose", default=False, action="store_true", help=verbose_help) @@ -458,8 +444,7 @@ class ArgumentParser(object): git_commit=git_commit, is_verbose=is_verbose, min_confidence=min_confidence, - output_format=output_format, - squash=options.squash) + output_format=output_format) return (paths, options) diff --git a/WebKitTools/Scripts/webkitpy/style_references.py b/WebKitTools/Scripts/webkitpy/style_references.py index bab30ca..a42b69d 100644 --- a/WebKitTools/Scripts/webkitpy/style_references.py +++ b/WebKitTools/Scripts/webkitpy/style_references.py @@ -67,6 +67,6 @@ class WebKitCheckout(object): """Return the checkout root as an absolute path.""" return self._scm.checkout_root - def create_patch(self, git_commit, squash): - return self._scm.create_patch(git_commit, squash) + def create_patch(self, git_commit): + return self._scm.create_patch(git_commit) diff --git a/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py b/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py index e1fa673..3321293 100644 --- a/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py +++ b/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py @@ -42,6 +42,9 @@ from webkitpy.common.system.autoinstall import AutoInstaller # perhaps be done using Python's import hooks as the original # autoinstall implementation did. +# FIXME: If any of these servers is offline, webkit-patch breaks (and maybe +# other scripts do, too). See <http://webkit.org/b/42080>. + # We put auto-installed third-party modules in this directory-- # # webkitpy/thirdparty/autoinstalled @@ -85,9 +88,9 @@ installer.install(url="http://webkit-rietveld.googlecode.com/svn/trunk/static/up # organization purposes. irc_dir = os.path.join(autoinstalled_dir, "irc") installer = AutoInstaller(target_dir=irc_dir) -installer.install(url="http://hivelocity.dl.sourceforge.net/project/python-irclib/python-irclib/0.4.8/python-irclib-0.4.8.zip", +installer.install(url="http://surfnet.dl.sourceforge.net/project/python-irclib/python-irclib/0.4.8/python-irclib-0.4.8.zip", url_subpath="irclib.py") -installer.install(url="http://hivelocity.dl.sourceforge.net/project/python-irclib/python-irclib/0.4.8/python-irclib-0.4.8.zip", +installer.install(url="http://surfnet.dl.sourceforge.net/project/python-irclib/python-irclib/0.4.8/python-irclib-0.4.8.zip", url_subpath="ircbot.py") pywebsocket_dir = os.path.join(autoinstalled_dir, "pywebsocket") diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py b/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py index 887802c..de92cd3 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py @@ -29,10 +29,9 @@ import unittest from webkitpy.common.system.outputcapture import OutputCapture -from webkitpy.tool.mocktool import MockTool -from webkitpy.thirdparty.mock import Mock +from webkitpy.tool.mocktool import MockOptions, MockTool class CommandsTest(unittest.TestCase): - def assert_execute_outputs(self, command, args, expected_stdout="", expected_stderr="", options=Mock(), tool=MockTool()): + def assert_execute_outputs(self, command, args, expected_stdout="", expected_stderr="", options=MockOptions(), tool=MockTool()): command.bind_to_tool(tool) OutputCapture().assert_outputs(self, command.execute, [options, args, tool], expected_stdout=expected_stdout, expected_stderr=expected_stderr) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py index 59af16a..d27ab0e 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py @@ -93,7 +93,7 @@ If a bug id is provided, or one can be found in the ChangeLog land will update t def _prepare_state(self, options, args, tool): return { - "bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit, options.squash), + "bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit), } diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py index 958620a..75cd0f3 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py @@ -32,7 +32,7 @@ from webkitpy.common.system.outputcapture import OutputCapture from webkitpy.thirdparty.mock import Mock from webkitpy.tool.commands.commandtest import CommandsTest from webkitpy.tool.commands.download import * -from webkitpy.tool.mocktool import MockTool +from webkitpy.tool.mocktool import MockOptions, MockTool class AbstractRolloutPrepCommandTest(unittest.TestCase): @@ -56,7 +56,7 @@ class AbstractRolloutPrepCommandTest(unittest.TestCase): class DownloadCommandsTest(CommandsTest): def _default_options(self): - options = Mock() + options = MockOptions() options.force_clean = False options.clean = True options.check_builders = True diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py index 5ff390c..97c3ddb 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py @@ -42,7 +42,7 @@ from webkitpy.tool.commands.stepsequence import StepSequenceErrorHandler from webkitpy.tool.bot.patchcollection import PersistentPatchCollection, PersistentPatchCollectionDelegate from webkitpy.tool.bot.queueengine import QueueEngine, QueueEngineDelegate from webkitpy.tool.grammar import pluralize -from webkitpy.tool.multicommandtool import Command +from webkitpy.tool.multicommandtool import Command, TryAgain class AbstractQueue(Command, QueueEngineDelegate): watchers = [ @@ -279,6 +279,17 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler): validator = CommitterValidator(tool.bugs) validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error)) + @classmethod + def handle_checkout_needs_update(cls, tool, state, options, error): + # The only time when we find out that out checkout needs update is + # when we were ready to actually pull the trigger and land the patch. + # Rather than spinning in the master process, we retry without + # building or testing, which is much faster. + options.build = False + options.test = False + options.update = True + raise TryAgain() + class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler): name = "rietveld-upload-queue" diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py b/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py index c6de79f..be2ed4c 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py @@ -39,6 +39,10 @@ class StepSequenceErrorHandler(): def handle_script_error(cls, tool, patch, script_error): raise NotImplementedError, "subclasses must implement" + @classmethod + def handle_checkout_needs_update(cls, tool, state, options, error): + raise NotImplementedError, "subclasses must implement" + class StepSequence(object): def __init__(self, steps): @@ -66,6 +70,9 @@ class StepSequence(object): self._run(tool, options, state) except CheckoutNeedsUpdate, e: log("Commit failed because the checkout is out of date. Please update and try again.") + if options.parent_command: + command = tool.command_by_name(options.parent_command) + command.handle_checkout_needs_update(tool, state, options, e) QueueEngine.exit_after_handled_error(e) except ScriptError, e: if not options.quiet: diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py index 9c935e8..4a15ed6 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py @@ -53,7 +53,6 @@ class CommitMessageForCurrentDiff(AbstractDeclarativeCommand): def __init__(self): options = [ - steps.Options.squash, steps.Options.git_commit, ] AbstractDeclarativeCommand.__init__(self, options=options) @@ -61,7 +60,7 @@ class CommitMessageForCurrentDiff(AbstractDeclarativeCommand): def execute(self, options, args, tool): # This command is a useful test to make sure commit_message_for_this_commit # always returns the right value regardless of the current working directory. - print "%s" % tool.checkout().commit_message_for_this_commit(options.git_commit, options.squash).message() + print "%s" % tool.checkout().commit_message_for_this_commit(options.git_commit).message() class CleanPendingCommit(AbstractDeclarativeCommand): @@ -153,7 +152,7 @@ class AbstractPatchUploadingCommand(AbstractSequencedCommand): # Perfer a bug id passed as an argument over a bug url in the diff (i.e. ChangeLogs). bug_id = args and args[0] if not bug_id: - bug_id = tool.checkout().bug_id_for_this_commit(options.git_commit, options.squash) + bug_id = tool.checkout().bug_id_for_this_commit(options.git_commit) return bug_id def _prepare_state(self, options, args, tool): @@ -423,11 +422,11 @@ class CreateBug(AbstractDeclarativeCommand): if options.prompt: (bug_title, comment_text) = self.prompt_for_bug_title_and_comment() else: - commit_message = tool.checkout().commit_message_for_this_commit(options.git_commit, options.squash) + commit_message = tool.checkout().commit_message_for_this_commit(options.git_commit) bug_title = commit_message.description(lstrip=True, strip_url=True) comment_text = commit_message.body(lstrip=True) - diff = tool.scm().create_patch(options.git_commit, options.squash) + diff = tool.scm().create_patch(options.git_commit) bug_id = tool.bugs.create_bug(bug_title, comment_text, options.component, diff, "Patch", cc=options.cc, mark_for_review=options.review, mark_for_commit_queue=options.request_commit) def prompt_for_bug_title_and_comment(self): diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py index 8fef54a..5f3f400 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py @@ -29,7 +29,7 @@ from webkitpy.thirdparty.mock import Mock from webkitpy.tool.commands.commandtest import CommandsTest from webkitpy.tool.commands.upload import * -from webkitpy.tool.mocktool import MockTool +from webkitpy.tool.mocktool import MockOptions, MockTool class UploadCommandsTest(CommandsTest): def test_commit_message_for_current_diff(self): @@ -51,7 +51,7 @@ class UploadCommandsTest(CommandsTest): self.assert_execute_outputs(ObsoleteAttachments(), [42], expected_stderr=expected_stderr) def test_post(self): - options = Mock() + options = MockOptions() options.description = "MOCK description" options.request_commit = False options.review = True @@ -80,7 +80,7 @@ MOCK: user.open_url: http://example.com/42 self.assert_execute_outputs(Prepare(), [], expected_stderr=expected_stderr) def test_upload(self): - options = Mock() + options = MockOptions() options.description = "MOCK description" options.request_commit = False options.review = True diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py index d88190f..a467364 100644 --- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py +++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py @@ -397,7 +397,7 @@ class MockSCM(Mock): # will actually be the root. Since getcwd() is wrong, use a globally fake root for now. self.checkout_root = self.fake_checkout_root - def create_patch(self, git_commit, squash): + def create_patch(self, git_commit): return "Patch1" def commit_ids_from_commitish_arguments(self, args): @@ -437,12 +437,12 @@ class MockCheckout(object): def bug_id_for_revision(self, svn_revision): return 12345 - def modified_changelogs(self, git_commit, squash): + def modified_changelogs(self, git_commit): # Ideally we'd return something more interesting here. The problem is # that LandDiff will try to actually read the patch from disk! return [] - def commit_message_for_this_commit(self, git_commit, squash): + def commit_message_for_this_commit(self, git_commit): commit_message = Mock() commit_message.message = lambda:"This is a fake commit message that is at least 50 characters." return commit_message @@ -515,6 +515,7 @@ class MockStatusServer(object): def results_url_for_status(self, status_id): return "http://dummy_url" + class MockExecute(Mock): def __init__(self, should_log): self._should_log = should_log @@ -537,6 +538,11 @@ class MockExecute(Mock): return "MOCK output of child process" +class MockOptions(Mock): + no_squash = False + squash = False + + class MockRietveld(): def __init__(self, executive, dryrun=False): diff --git a/WebKitTools/Scripts/webkitpy/tool/multicommandtool.py b/WebKitTools/Scripts/webkitpy/tool/multicommandtool.py index 7940c06..12ede2e 100644 --- a/WebKitTools/Scripts/webkitpy/tool/multicommandtool.py +++ b/WebKitTools/Scripts/webkitpy/tool/multicommandtool.py @@ -39,6 +39,10 @@ from webkitpy.tool.grammar import pluralize from webkitpy.common.system.deprecated_logging import log +class TryAgain(Exception): + pass + + class Command(object): name = None show_in_main_help = False @@ -299,6 +303,12 @@ class MultiCommandTool(object): log(failure_reason) return 0 # FIXME: Should this really be 0? - result = command.check_arguments_and_execute(options, args, self) + while True: + try: + result = command.check_arguments_and_execute(options, args, self) + break + except TryAgain, e: + pass + self.command_completed() return result diff --git a/WebKitTools/Scripts/webkitpy/tool/multicommandtool_unittest.py b/WebKitTools/Scripts/webkitpy/tool/multicommandtool_unittest.py index 268ebf0..c19095c 100644 --- a/WebKitTools/Scripts/webkitpy/tool/multicommandtool_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/multicommandtool_unittest.py @@ -32,7 +32,7 @@ import unittest from optparse import make_option from webkitpy.common.system.outputcapture import OutputCapture -from webkitpy.tool.multicommandtool import MultiCommandTool, Command +from webkitpy.tool.multicommandtool import MultiCommandTool, Command, TryAgain class TrivialCommand(Command): @@ -44,10 +44,26 @@ class TrivialCommand(Command): def execute(self, options, args, tool): pass + class UncommonCommand(TrivialCommand): name = "uncommon" show_in_main_help = False + +class LikesToRetry(Command): + name = "likes-to-retry" + show_in_main_help = True + + def __init__(self, **kwargs): + Command.__init__(self, "help text", **kwargs) + self.execute_count = 0 + + def execute(self, options, args, tool): + self.execute_count += 1 + if self.execute_count < 2: + raise TryAgain() + + class CommandTest(unittest.TestCase): def test_name_with_arguments(self): command_with_args = TrivialCommand(argument_names="ARG1 ARG2") @@ -109,6 +125,12 @@ class MultiCommandToolTest(unittest.TestCase): exit_code = OutputCapture().assert_outputs(self, tool.main, [main_args], expected_stdout=expected_stdout, expected_stderr=expected_stderr) self.assertEqual(exit_code, expected_exit_code) + def test_retry(self): + likes_to_retry = LikesToRetry() + tool = TrivialTool(commands=[likes_to_retry]) + tool.main(["tool", "likes-to-retry"]) + self.assertEqual(likes_to_retry.execute_count, 2) + def test_global_help(self): tool = TrivialTool(commands=[TrivialCommand(), UncommonCommand()]) expected_common_commands_help = """Usage: trivial-tool [options] COMMAND [ARGS] diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py b/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py index 20f8bbf..8f0d153 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py @@ -27,6 +27,7 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. from webkitpy.common.system.deprecated_logging import log +from webkitpy.common.system.executive import ScriptError from webkitpy.common.config.ports import WebKitPort from webkitpy.tool.steps.options import Options @@ -34,6 +35,10 @@ from webkitpy.tool.steps.options import Options class AbstractStep(object): def __init__(self, tool, options): self._tool = tool + if options.no_squash: + raise ScriptError('--no-squash has been removed. Use "--git-commit=HEAD.." or "-g HEAD.." to operate on the working copy.') + if options.squash: + raise ScriptError('--squash has been removed. It is now the default behavior if --git-commit is omitted.') self._options = options self._port = None @@ -53,8 +58,8 @@ class AbstractStep(object): return self._port _well_known_keys = { - "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, self._options.squash), - "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit, self._options.squash), + "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit), + "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit), "bug_title": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]).title(), } @@ -69,8 +74,9 @@ class AbstractStep(object): @classmethod def options(cls): return [ - # We need these options here because cached_lookup uses them. :( + # We need this option here because cached_lookup uses it. :( Options.git_commit, + # FIXME: Get rid of these. Options.no_squash, Options.squash, ] diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/applypatchwithlocalcommit.py b/WebKitTools/Scripts/webkitpy/tool/steps/applypatchwithlocalcommit.py index d6b026d..3dcd8d9 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/applypatchwithlocalcommit.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/applypatchwithlocalcommit.py @@ -39,5 +39,5 @@ class ApplyPatchWithLocalCommit(ApplyPatch): def run(self, state): ApplyPatch.run(self, state) if self._options.local_commit: - commit_message = self._tool.checkout().commit_message_for_this_commit(git_commit=None, squash=False) + commit_message = self._tool.checkout().commit_message_for_this_commit(git_commit=None) self._tool.scm().commit_locally_with_message(commit_message.message() or state["patch"].name()) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py b/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py index 93e6215..af38214 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py @@ -40,8 +40,6 @@ class CheckStyle(AbstractStep): Options.non_interactive, Options.check_style, Options.git_commit, - Options.no_squash, - Options.squash, ] def run(self, state): @@ -53,10 +51,6 @@ class CheckStyle(AbstractStep): if self._options.git_commit: args.append("--git-commit") args.append(self._options.git_commit) - if self._tool.scm().should_squash(self._options.squash): - args.append("--squash") - else: - args.append("--no-squash") try: self._run_script("check-webkit-style", args) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py deleted file mode 100644 index a23ea1b..0000000 --- a/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py +++ /dev/null @@ -1,46 +0,0 @@ -# Copyright (C) 2009 Google Inc. All rights reserved. -# -# Redistribution and use in source and binary forms, with or without -# modification, are permitted provided that the following conditions are -# met: -# -# * Redistributions of source code must retain the above copyright -# notice, this list of conditions and the following disclaimer. -# * Redistributions in binary form must reproduce the above -# copyright notice, this list of conditions and the following disclaimer -# in the documentation and/or other materials provided with the -# distribution. -# * Neither the name of Google Inc. nor the names of its -# contributors may be used to endorse or promote products derived from -# this software without specific prior written permission. -# -# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -import unittest - -from webkitpy.common.system.executive import ScriptError -from webkitpy.thirdparty.mock import Mock -from webkitpy.tool.mocktool import MockTool -from webkitpy.tool.steps.checkstyle import CheckStyle - - -class CheckStyleTest(unittest.TestCase): - def test_should_squash_error(self): - """should_squash can throw an error. That error should not be eaten by CheckStyle.""" - def should_squash(squash): - raise ScriptError(message="Dummy error") - - tool = MockTool() - tool._scm.should_squash = should_squash - step = CheckStyle(tool, Mock()) - self.assertRaises(ScriptError, step.run, []) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/closebugforlanddiff_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/closebugforlanddiff_unittest.py index 4e7f9e6..0a56564 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/closebugforlanddiff_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/closebugforlanddiff_unittest.py @@ -29,13 +29,12 @@ import unittest from webkitpy.common.system.outputcapture import OutputCapture -from webkitpy.thirdparty.mock import Mock -from webkitpy.tool.mocktool import MockTool +from webkitpy.tool.mocktool import MockOptions, MockTool from webkitpy.tool.steps.closebugforlanddiff import CloseBugForLandDiff class CloseBugForLandDiffTest(unittest.TestCase): def test_empty_state(self): capture = OutputCapture() - step = CloseBugForLandDiff(MockTool(), Mock()) + step = CloseBugForLandDiff(MockTool(), MockOptions()) expected_stderr = "Committed r49824: <http://trac.webkit.org/changeset/49824>\nNo bug id provided.\n" capture.assert_outputs(self, step.run, [{"commit_text" : "Mock commit text"}], expected_stderr=expected_stderr) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/commit.py b/WebKitTools/Scripts/webkitpy/tool/steps/commit.py index 7bf8b8a..9f93120 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/commit.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/commit.py @@ -26,6 +26,9 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from webkitpy.common.checkout.scm import AuthenticationError, AmbiguousCommitError +from webkitpy.common.system.executive import ScriptError +from webkitpy.common.system.user import User from webkitpy.tool.steps.abstractstep import AbstractStep from webkitpy.tool.steps.options import Options @@ -35,13 +38,38 @@ class Commit(AbstractStep): def options(cls): return AbstractStep.options() + [ Options.git_commit, - Options.no_squash, - Options.squash, ] + def _commit_warning(self, error): + working_directory_message = "" if error.working_directory_is_clean else " and working copy changes" + return ('There are %s local commits%s. Everything will be committed as a single commit. ' + 'To avoid this prompt, set "git config webkit-patch.squash true".' % ( + error.num_local_commits, working_directory_message)) + def run(self, state): - commit_message = self._tool.checkout().commit_message_for_this_commit(self._options.git_commit, self._options.squash) - if len(commit_message.message()) < 50: + self._commit_message = self._tool.checkout().commit_message_for_this_commit(self._options.git_commit).message() + if len(self._commit_message) < 50: raise Exception("Attempted to commit with a commit message shorter than 50 characters. Either your patch is missing a ChangeLog or webkit-patch may have a bug.") - state["commit_text"] = self._tool.scm().commit_with_message(commit_message.message(), - git_commit=self._options.git_commit, squash=self._options.squash) + + self._state = state + + username = None + force_squash = False + + num_tries = 0 + while num_tries < 3: + num_tries += 1 + + try: + self._state["commit_text"] = self._tool.scm().commit_with_message(self._commit_message, git_commit=self._options.git_commit, username=username, force_squash=force_squash) + break; + except AmbiguousCommitError, e: + if self._tool.user.confirm(self._commit_warning(e)): + force_squash = True + else: + # This will correctly interrupt the rest of the commit process. + raise ScriptError(message="Did not commit") + except AuthenticationError, e: + username = self._tool.user.prompt("%s login: " % e.server_host, repeat=5) + if not username: + raise ScriptError("You need to specify the username on %s to perform the commit as." % self.svn_server_host) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/createbug.py b/WebKitTools/Scripts/webkitpy/tool/steps/createbug.py index cd043d6..0ab6f68 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/createbug.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/createbug.py @@ -36,6 +36,7 @@ class CreateBug(AbstractStep): return AbstractStep.options() + [ Options.cc, Options.component, + Options.blocks, ] def run(self, state): @@ -45,4 +46,7 @@ class CreateBug(AbstractStep): cc = self._options.cc if not cc: cc = state.get("bug_cc") - state["bug_id"] = self._tool.bugs.create_bug(state["bug_title"], state["bug_description"], blocked=state.get("bug_blocked"), component=self._options.component, cc=cc) + blocks = self._options.blocks + if not blocks: + blocks = state.get("bug_blocked") + state["bug_id"] = self._tool.bugs.create_bug(state["bug_title"], state["bug_description"], blocked=blocks, component=self._options.component, cc=cc) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/options.py b/WebKitTools/Scripts/webkitpy/tool/steps/options.py index fa36f73..9c73f5a 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/options.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/options.py @@ -29,6 +29,7 @@ from optparse import make_option class Options(object): + blocks = make_option("--blocks", action="store", dest="blocks", default=None, help="Bug number which the created bug blocks.") build = make_option("--build", action="store_true", dest="build", default=False, help="Build and run run-webkit-tests before committing.") build_style = make_option("--build-style", action="store", dest="build_style", default=None, help="Whether to build debug, release, or both.") cc = make_option("--cc", action="store", type="string", dest="cc", help="Comma-separated list of email addresses to carbon-copy.") @@ -42,11 +43,11 @@ class Options(object): description = make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment (default: \"patch\")") email = make_option("--email", action="store", type="string", dest="email", help="Email address to use in ChangeLogs.") force_clean = make_option("--force-clean", action="store_true", dest="force_clean", default=False, help="Clean working directory before applying patches (removes local changes and commits)") -# FIXME: Make commit ranges treat each commit separately instead of squashing them into one. - git_commit = make_option("--git-commit", action="store", dest="git_commit", help="Local git commit to upload/land. If a range, the commits are squashed into one.") + git_commit = make_option("-g", "--git-commit", action="store", dest="git_commit", help="Operate on a local commit. If a range, the commits are squashed into one. HEAD.. operates on working copy changes only.") local_commit = make_option("--local-commit", action="store_true", dest="local_commit", default=False, help="Make a local commit for each applied patch") - no_squash = make_option("--no-squash", action="store_false", dest="squash", help="Don't squash local commits into one on upload/land (git-only).") non_interactive = make_option("--non-interactive", action="store_true", dest="non_interactive", default=False, help="Never prompt the user, fail as fast as possible.") + # FIXME: Remove --no-squash, once people have adjusted to using --git-commit. + no_squash = make_option("--no-squash", action="store_true", dest="no_squash", default=False, help="Obsolete. Use --git-commit=HEAD.. instead.") obsolete_patches = make_option("--no-obsolete", action="store_false", dest="obsolete_patches", default=True, help="Do not obsolete old patches before posting this one.") open_bug = make_option("--open-bug", action="store_true", dest="open_bug", default=False, help="Opens the associated bug in a browser.") parent_command = make_option("--parent-command", action="store", dest="parent_command", default=None, help="(Internal) The command that spawned this instance.") @@ -55,6 +56,7 @@ class Options(object): request_commit = make_option("--request-commit", action="store_true", dest="request_commit", default=False, help="Mark the patch as needing auto-commit after review.") review = make_option("--no-review", action="store_false", dest="review", default=True, help="Do not mark the patch for review.") reviewer = make_option("-r", "--reviewer", action="store", type="string", dest="reviewer", help="Update ChangeLogs to say Reviewed by REVIEWER.") - squash = make_option("-s", "--squash", action="store_true", dest="squash", help="Squash all local commits into one on upload/land (git-only).") + # FIXME: Remove --squash, once people have adjusted to using --git-commit. + squash = make_option("-s", "--squash", action="store_true", dest="squash", default=False, help="Obsolete. This is now the default behavior.") test = make_option("--test", action="store_true", dest="test", default=False, help="Run run-webkit-tests before committing.") update = make_option("--no-update", action="store_false", dest="update", default=True, help="Don't update the working directory.") diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py index 7f0c1a8..ce04024 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py @@ -43,8 +43,6 @@ class PrepareChangeLog(AbstractStep): Options.quiet, Options.email, Options.git_commit, - Options.no_squash, - Options.squash, ] def _ensure_bug_url(self, state): @@ -69,10 +67,9 @@ class PrepareChangeLog(AbstractStep): args.append("--bug=%s" % state["bug_id"]) if self._options.email: args.append("--email=%s" % self._options.email) - if self._tool.scm().should_squash(self._options.squash): - args.append("--merge-base=%s" % self._tool.scm().remote_merge_base()) - if self._options.git_commit: - args.append("--git-commit=%s" % self._options.git_commit) + + if self._tool.scm().supports_local_commits(): + args.append("--merge-base=%s" % self._tool.scm().merge_base(self._options.git_commit)) try: self._tool.executive.run_and_throw_if_fail(args, self._options.quiet) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py index 1d0db75..eceffdf 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py @@ -31,15 +31,14 @@ import unittest from webkitpy.common.checkout.changelog_unittest import ChangeLogTest from webkitpy.common.system.outputcapture import OutputCapture -from webkitpy.thirdparty.mock import Mock -from webkitpy.tool.mocktool import MockTool +from webkitpy.tool.mocktool import MockOptions, MockTool from webkitpy.tool.steps.preparechangelog import PrepareChangeLog class PrepareChangeLogTest(ChangeLogTest): def test_ensure_bug_url(self): capture = OutputCapture() - step = PrepareChangeLog(MockTool(), Mock()) + step = PrepareChangeLog(MockTool(), MockOptions()) changelog_contents = u"%s\n%s" % (self._new_entry_boilerplate, self._example_changelog) changelog_path = self._write_tmp_file_with_contents(changelog_contents.encode("utf-8")) state = { diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py index 4d299fa..0e78bc2 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py @@ -36,7 +36,7 @@ class PrepareChangeLogForRevert(AbstractStep): def run(self, state): # This could move to prepare-ChangeLog by adding a --revert= option. self._run_script("prepare-ChangeLog") - changelog_paths = self._tool.checkout().modified_changelogs(git_commit=None, squash=False) + changelog_paths = self._tool.checkout().modified_changelogs(git_commit=None) bug_url = self._tool.bugs.bug_url_for_bug_id(state["bug_id"]) if state["bug_id"] else None for changelog_path in changelog_paths: # FIXME: Seems we should prepare the message outside of changelogs.py and then just pass in diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py index 1fd2bad..766801b 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py @@ -30,8 +30,7 @@ import unittest from webkitpy.common.system.outputcapture import OutputCapture from webkitpy.common.config.ports import WebKitPort -from webkitpy.thirdparty.mock import Mock -from webkitpy.tool.mocktool import MockTool +from webkitpy.tool.mocktool import MockOptions, MockTool from webkitpy.tool.steps.update import Update from webkitpy.tool.steps.runtests import RunTests from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle @@ -42,13 +41,13 @@ class StepsTest(unittest.TestCase): if not tool: tool = MockTool() if not options: - options = Mock() + options = MockOptions() if not state: state = {} step(tool, options).run(state) def test_update_step(self): - options = Mock() + options = MockOptions() options.update = True expected_stderr = "Updating working directory\n" OutputCapture().assert_outputs(self, self._run_step, [Update, options], expected_stderr=expected_stderr) @@ -63,7 +62,7 @@ class StepsTest(unittest.TestCase): OutputCapture().assert_outputs(self, self._run_step, [RunTests], expected_stderr=expected_stderr) def test_runtests_leopard_commit_queue_hack(self): - mock_options = Mock() + mock_options = MockOptions() mock_options.non_interactive = True step = RunTests(MockTool(log_executive=True), mock_options) # FIXME: We shouldn't use a real port-object here, but there is too much to mock at the moment. diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py index 0534718..a037422 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreview_unittest.py @@ -29,18 +29,17 @@ import unittest from webkitpy.common.system.outputcapture import OutputCapture -from webkitpy.thirdparty.mock import Mock -from webkitpy.tool.mocktool import MockTool +from webkitpy.tool.mocktool import MockOptions, MockTool from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer class UpdateChangeLogsWithReviewerTest(unittest.TestCase): def test_guess_reviewer_from_bug(self): capture = OutputCapture() - step = UpdateChangeLogsWithReviewer(MockTool(), Mock()) + step = UpdateChangeLogsWithReviewer(MockTool(), MockOptions()) expected_stderr = "0 reviewed patches on bug 75, cannot infer reviewer.\n" capture.assert_outputs(self, step._guess_reviewer_from_bug, [75], expected_stderr=expected_stderr) def test_empty_state(self): capture = OutputCapture() - step = UpdateChangeLogsWithReviewer(MockTool(), Mock()) + step = UpdateChangeLogsWithReviewer(MockTool(), MockOptions()) capture.assert_outputs(self, step.run, [{}]) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py b/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py index ef4baa2..e46b790 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py @@ -40,8 +40,6 @@ class UpdateChangeLogsWithReviewer(AbstractStep): return AbstractStep.options() + [ Options.git_commit, Options.reviewer, - Options.no_squash, - Options.squash, ] def _guess_reviewer_from_bug(self, bug_id): diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py b/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py index 9f4d44e..bdf729e 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py @@ -41,8 +41,6 @@ class ValidateReviewer(AbstractStep): def options(cls): return AbstractStep.options() + [ Options.git_commit, - Options.no_squash, - Options.squash, ] # FIXME: This should probably move onto ChangeLogEntry diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer_unittest.py index 9105102..d9b856a 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer_unittest.py @@ -30,8 +30,7 @@ import unittest from webkitpy.common.checkout.changelog import ChangeLogEntry from webkitpy.common.system.outputcapture import OutputCapture -from webkitpy.thirdparty.mock import Mock -from webkitpy.tool.mocktool import MockTool +from webkitpy.tool.mocktool import MockOptions, MockTool from webkitpy.tool.steps.validatereviewer import ValidateReviewer class ValidateReviewerTest(unittest.TestCase): @@ -48,7 +47,7 @@ class ValidateReviewerTest(unittest.TestCase): self.assertEqual(step._has_valid_reviewer(entry), expected) def test_has_valid_reviewer(self): - step = ValidateReviewer(MockTool(), Mock()) + step = ValidateReviewer(MockTool(), MockOptions()) self._test_review_text(step, "Reviewed by Eric Seidel.", True) self._test_review_text(step, "Reviewed by Eric Seidel", True) # Not picky about the '.' self._test_review_text(step, "Reviewed by Eric.", False) |