diff options
Diffstat (limited to 'WebKitTools/Scripts')
73 files changed, 1137 insertions, 637 deletions
diff --git a/WebKitTools/Scripts/VCSUtils.pm b/WebKitTools/Scripts/VCSUtils.pm index 8d7e766..faed7ed 100644 --- a/WebKitTools/Scripts/VCSUtils.pm +++ b/WebKitTools/Scripts/VCSUtils.pm @@ -68,7 +68,9 @@ BEGIN { &parsePatch &pathRelativeToSVNRepositoryRootForPath &prepareParsedPatch + &removeEOL &runPatchCommand + &scmMoveOrRenameFile &scmToggleExecutableBit &setChangeLogDateAndReviewer &svnRevisionForDirectory @@ -412,6 +414,7 @@ sub canonicalizePath($) sub removeEOL($) { my ($line) = @_; + return "" unless $line; $line =~ s/[\r\n]+$//g; return $line; diff --git a/WebKitTools/Scripts/build-webkit b/WebKitTools/Scripts/build-webkit index e7f9d1f..da336fe 100755 --- a/WebKitTools/Scripts/build-webkit +++ b/WebKitTools/Scripts/build-webkit @@ -1,6 +1,6 @@ #!/usr/bin/perl -w -# Copyright (C) 2005, 2006 Apple Inc. All rights reserved. +# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved. # Copyright (C) 2009 Google Inc. All rights reserved. # Copyright (C) 2010 moiji-mobile.com All rights reserved. # @@ -57,27 +57,69 @@ my $prefixPath; my $makeArgs; my $startTime = time(); -my ($linkPrefetchSupport, $accelerated2dCanvasSupport, $threeDCanvasSupport, $threeDRenderingSupport, $channelMessagingSupport, $clientBasedGeolocationSupport, $databaseSupport, $datagridSupport, $datalistSupport, - $domStorageSupport, $eventsourceSupport, $filtersSupport, $geolocationSupport, $iconDatabaseSupport, $imageResizerSupport, $indexedDatabaseSupport, $inputSpeechSupport, - $javaScriptDebuggerSupport, $mathmlSupport, $offlineWebApplicationSupport, $rubySupport, $systemMallocSupport, $sandboxSupport, $sharedWorkersSupport, - $svgSupport, $svgAnimationSupport, $svgAsImageSupport, $svgDOMObjCBindingsSupport, $svgFontsSupport, - $svgForeignObjectSupport, $svgUseSupport, $videoSupport, $webSocketsSupport, $webTimingSupport, $wmlSupport, $wcssSupport, $xhtmlmpSupport, $workersSupport, - $xpathSupport, $xsltSupport, $coverageSupport, $notificationsSupport, $blobSupport, $tiledBackingStoreSupport, - $fileWriterSupport, $fileSystemSupport, $directoryUploadSupport, $deviceOrientationSupport); +my ( + $threeDCanvasSupport, + $threeDRenderingSupport, + $accelerated2dCanvasSupport, + $blobSupport, + $channelMessagingSupport, + $clientBasedGeolocationSupport, + $coverageSupport, + $databaseSupport, + $datagridSupport, + $datalistSupport, + $deviceOrientationSupport, + $directoryUploadSupport, + $domStorageSupport, + $eventsourceSupport, + $fileSystemSupport, + $filtersSupport, + $fullscreenAPISupport, + $geolocationSupport, + $iconDatabaseSupport, + $imageResizerSupport, + $indexedDatabaseSupport, + $inputSpeechSupport, + $javaScriptDebuggerSupport, + $linkPrefetchSupport, + $mathmlSupport, + $meterTagSupport, + $notificationsSupport, + $offlineWebApplicationSupport, + $progressTagSupport, + $rubySupport, + $sharedWorkersSupport, + $svgSupport, + $svgAnimationSupport, + $svgAsImageSupport, + $svgDOMObjCBindingsSupport, + $svgFontsSupport, + $svgForeignObjectSupport, + $svgUseSupport, + $systemMallocSupport, + $tiledBackingStoreSupport, + $videoSupport, + $wcssSupport, + $webAudioSupport, + $webSocketsSupport, + $webTimingSupport, + $wmlSupport, + $workersSupport, + $xhtmlmpSupport, + $xpathSupport, + $xsltSupport, +); my @features = ( - { option => "link-prefetch", desc => "Toggle pre fetching support", - define => "ENABLE_LINK_PREFETCH", default => 0, value => \$linkPrefetchSupport }, - - { option => "accelerated-2d-canvas", desc => "Toggle accelerated 2D canvas support", - define => "ENABLE_ACCELERATED_2D_CANVAS", default => 0, value => \$accelerated2dCanvasSupport }, - { option => "3d-canvas", desc => "Toggle 3D canvas support", define => "ENABLE_3D_CANVAS", default => (isAppleMacWebKit() && !isTiger() && !isLeopard()), value => \$threeDCanvasSupport }, { option => "3d-rendering", desc => "Toggle 3D rendering support", define => "ENABLE_3D_RENDERING", default => (isAppleMacWebKit() && !isTiger()), value => \$threeDRenderingSupport }, + { option => "accelerated-2d-canvas", desc => "Toggle accelerated 2D canvas support", + define => "ENABLE_ACCELERATED_2D_CANVAS", default => 0, value => \$accelerated2dCanvasSupport }, + { option => "blob", desc => "Toggle Blob support", define => "ENABLE_BLOB", default => (isAppleMacWebKit()), value => \$blobSupport }, @@ -95,10 +137,13 @@ my @features = ( { option => "datagrid", desc => "Toggle Datagrid Support", define => "ENABLE_DATAGRID", default => 0, value => \$datagridSupport }, - + { option => "datalist", desc => "Toggle HTML5 datalist support", define => "ENABLE_DATALIST", default => 1, value => \$datalistSupport }, - + + { option => "device-orientation", desc => "Toggle DeviceOrientation support", + define => "ENABLE_DEVICE_ORIENTATION", default => 0, value => \$deviceOrientationSupport }, + { option => "directory-upload", desc => "Toogle Directory upload support", define => "ENABLE_DIRECTORY_UPLOAD", default => 0, value => \$directoryUploadSupport }, @@ -108,15 +153,21 @@ my @features = ( { option => "eventsource", desc => "Toggle server-sent events support", define => "ENABLE_EVENTSOURCE", default => 1, value => \$eventsourceSupport }, + { option => "file-system", desc => "Toggle FileSystem support", + define => "ENABLE_FILE_SYSTEM", default => 0, value => \$fileSystemSupport }, + { option => "filters", desc => "Toggle Filters support", define => "ENABLE_FILTERS", default => (isAppleWebKit() || isGtk() || isQt() || isEfl()), value => \$filtersSupport }, + { option => "fullscreen-api", desc => "Toggle Fullscreen API support", + define => "ENABLE_FULLSCREEN_API", default => isAppleMacWebKit(), value => \$fullscreenAPISupport }, + { option => "geolocation", desc => "Toggle Geolocation support", define => "ENABLE_GEOLOCATION", default => (isAppleWebKit() || isGtk()), value => \$geolocationSupport }, { option => "icon-database", desc => "Toggle Icon database support", define => "ENABLE_ICONDATABASE", default => 1, value => \$iconDatabaseSupport }, - + { option => "image-resizer", desc => "Toggle Image Resizer API support", define => "ENABLE_IMAGE_RESIZER", default => 0, value => \$imageResizerSupport }, @@ -129,24 +180,30 @@ my @features = ( { option => "javascript-debugger", desc => "Toggle JavaScript Debugger/Profiler support", define => "ENABLE_JAVASCRIPT_DEBUGGER", default => 1, value => \$javaScriptDebuggerSupport }, + { option => "link-prefetch", desc => "Toggle pre fetching support", + define => "ENABLE_LINK_PREFETCH", default => 0, value => \$linkPrefetchSupport }, + { option => "mathml", desc => "Toggle MathML support", define => "ENABLE_MATHML", default => 1, value => \$mathmlSupport }, + { option => "meter-tag", desc => "Meter Tag support", + define => "ENABLE_METER_TAG", default => !isGtk() && !isAppleWinWebKit(), value => \$meterTagSupport }, + { option => "notifications", desc => "Toggle Desktop Notifications Support", define => "ENABLE_NOTIFICATIONS", default => 0, value => \$notificationsSupport }, { option => "offline-web-applications", desc => "Toggle Offline Web Application Support", define => "ENABLE_OFFLINE_WEB_APPLICATIONS", default => 1, value => \$offlineWebApplicationSupport }, + { option => "progress-tag", desc => "Progress Tag support", + define => "ENABLE_PROGRESS_TAG", default => 1, value => \$progressTagSupport }, + { option => "ruby", desc => "Toggle HTML5 Ruby support", define => "ENABLE_RUBY", default => 1, value => \$rubySupport }, { option => "system-malloc", desc => "Toggle system allocator instead of TCmalloc", define => "USE_SYSTEM_MALLOC", default => 0, value => \$systemMallocSupport }, - { option => "sandbox", desc => "Toggle HTML5 Sandboxed iframe support", - define => "ENABLE_SANDBOX", default => 1, value => \$sandboxSupport }, - { option => "shared-workers", desc => "Toggle SharedWorkers support", define => "ENABLE_SHARED_WORKERS", default => (isAppleWebKit() || isGtk()), value => \$sharedWorkersSupport }, @@ -177,6 +234,12 @@ my @features = ( { option => "video", desc => "Toggle Video support", define => "ENABLE_VIDEO", default => (isAppleWebKit() || isGtk()), value => \$videoSupport }, + { option => "wcss", desc => "Toggle WCSS support", + define => "ENABLE_WCSS", default => 0, value => \$wcssSupport }, + + { option => "web-audio", desc => "Toggle Web Audio support", + define => "ENABLE_WEB_AUDIO", default => 0, value=> \$webAudioSupport }, + { option => "web-sockets", desc => "Toggle Web Sockets support", define => "ENABLE_WEB_SOCKETS", default => 1, value=> \$webSocketsSupport }, @@ -186,26 +249,17 @@ my @features = ( { option => "wml", desc => "Toggle WML support", define => "ENABLE_WML", default => 0, value => \$wmlSupport }, - { option => "xhtmlmp", desc => "Toggle XHTML-MP support", - define => "ENABLE_XHTMLMP", default => 0, value => \$xhtmlmpSupport }, - - { option => "wcss", desc => "Toggle WCSS support", - define => "ENABLE_WCSS", default => 0, value => \$wcssSupport }, - { option => "workers", desc => "Toggle Web Workers support", define => "ENABLE_WORKERS", default => (isAppleWebKit() || isGtk()), value => \$workersSupport }, + { option => "xhtmlmp", desc => "Toggle XHTML-MP support", + define => "ENABLE_XHTMLMP", default => 0, value => \$xhtmlmpSupport }, + { option => "xpath", desc => "Toggle XPath support", define => "ENABLE_XPATH", default => 1, value => \$xpathSupport }, { option => "xslt", desc => "Toggle XSLT support", define => "ENABLE_XSLT", default => 1, value => \$xsltSupport }, - - { option => "file-system", desc => "Toggle FileSystem support", - define => "ENABLE_FILE_SYSTEM", default => 0, value => \$fileSystemSupport }, - - { option => "device-orientation", desc => "Toggle DeviceOrientation support", - define => "ENABLE_DEVICE_ORIENTATION", default => 0, value => \$deviceOrientationSupport }, ); # Update defaults from Qt's project file @@ -473,18 +527,24 @@ for my $dir (@projects) { $result = buildVisualStudioProject("win/WebKit.vcproj/WebKit.sln", $clean); } } + # Various build* calls above may change the CWD. + chdirWebKit(); if (exitStatus($result)) { + my $scriptDir = relativeScriptsDir(); + if (usingVisualStudioExpress()) { + # Visual Studio Express is so lame it can't stdout build failures. + # So we find its logs and dump them to the console ourselves. + system(File::Spec->catfile($scriptDir, "print-vse-failure-logs")); + } if (isAppleWinWebKit()) { print "\n\n===== BUILD FAILED ======\n\n"; - my $scriptDir = relativeScriptsDir(); print "Please ensure you have run $scriptDir/update-webkit to install dependencies.\n\n"; my $baseProductDir = baseProductDir(); print "You can view build errors by checking the BuildLog.htm files located at:\n$baseProductDir/obj/<project>/<config>.\n"; } exit exitStatus($result); } - chdirWebKit(); } # Don't report the "WebKit is now built" message after a clean operation. diff --git a/WebKitTools/Scripts/old-run-webkit-tests b/WebKitTools/Scripts/old-run-webkit-tests index a468b4d..e40b3ad 100755 --- a/WebKitTools/Scripts/old-run-webkit-tests +++ b/WebKitTools/Scripts/old-run-webkit-tests @@ -527,11 +527,37 @@ if (isCygwin()) { if (!$hasAcceleratedCompositing) { $ignoredDirectories{'compositing'} = 1; + + # This test has slightly different floating-point rounding when accelerated + # compositing is enabled. + $ignoredFiles{'svg/custom/use-on-symbol-inside-pattern.svg'} = 1; + + if (isAppleWebKit()) { + # In Apple's ports, the default controls for <video> contain a "full + # screen" button only if accelerated compositing is enabled. + $ignoredFiles{'media/controls-after-reload.html'} = 1; + $ignoredFiles{'media/controls-drag-timebar.html'} = 1; + $ignoredFiles{'media/controls-strict.html'} = 1; + $ignoredFiles{'media/controls-styling.html'} = 1; + $ignoredFiles{'media/video-controls-rendering.html'} = 1; + $ignoredFiles{'media/video-display-toggle.html'} = 1; + $ignoredFiles{'media/video-no-audio.html'} = 1; + } + + # Here we're using !$hasAcceleratedCompositing as a proxy for "is a headless XP machine" (like + # our test slaves). Headless XP machines can neither support accelerated compositing nor pass + # this test, so skipping the test here is expedient, if a little sloppy. See + # <http://webkit.org/b/48333>. + $ignoredFiles{'platform/win/plugins/npn-invalidate-rect-invalidates-window.html'} = 1 if isAppleWinWebKit(); } if (!$has3DRendering) { $ignoredDirectories{'animations/3d'} = 1; $ignoredDirectories{'transforms/3d'} = 1; + + # These tests use the -webkit-transform-3d media query. + $ignoredFiles{'fast/media/mq-transform-02.html'} = 1; + $ignoredFiles{'fast/media/mq-transform-03.html'} = 1; } if (!checkWebCoreFeatureSupport("3D Canvas", 0)) { diff --git a/WebKitTools/Scripts/prepare-ChangeLog b/WebKitTools/Scripts/prepare-ChangeLog index 608c9ce..ad84e4f 100755 --- a/WebKitTools/Scripts/prepare-ChangeLog +++ b/WebKitTools/Scripts/prepare-ChangeLog @@ -1302,7 +1302,7 @@ sub statusCommand(@) if ($isSVN) { $command = "$SVN stat $filesString"; } elsif ($isGit) { - $command = "$GIT diff -r --name-status -C -C -M " . diffFromToString(); + $command = "$GIT diff -r --name-status -M -C " . diffFromToString(); $command .= " -- $filesString" unless $gitCommit; } @@ -1317,7 +1317,7 @@ sub createPatchCommand($) if ($isSVN) { $command = "'$FindBin::Bin/svn-create-patch' $changedFilesString"; } elsif ($isGit) { - $command = "$GIT diff -C -C -M " . diffFromToString(); + $command = "$GIT diff -M -C " . diffFromToString(); $command .= " -- $changedFilesString" unless $gitCommit; } diff --git a/WebKitTools/Scripts/print-vse-failure-logs b/WebKitTools/Scripts/print-vse-failure-logs new file mode 100755 index 0000000..cec806e --- /dev/null +++ b/WebKitTools/Scripts/print-vse-failure-logs @@ -0,0 +1,80 @@ +#!/usr/bin/env python +# 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. +# +# This is a very simple script designed to crawl the build directory +# for visual studio express build logs and print them to stdout. + +from __future__ import with_statement + +import codecs +import os.path +import os + +from webkitpy.common.system.executive import Executive + + +# This is just enough to make the win-ews bot useable. Others who +# actually use Windows should feel encouraged to improve this class, +# including just printing the text instead of the raw html. +class PrintVisualStudioExpressLogs(object): + def __init__(self): + self._executive = Executive() + + def _find_buildlogs(self, build_directory): + build_log_paths = [] + for dirpath, dirnames, filenames in os.walk(build_directory): + for file_name in filenames: + if file_name == "BuildLog.htm": + file_path = os.path.join(dirpath, file_name) + build_log_paths.append(file_path) + return build_log_paths + + def _obj_directory(self): + scripts_directory = os.path.dirname(__file__) + build_directory_script_path = os.path.join(scripts_directory, "webkit-build-directory") + # FIXME: ports/webkit.py should provide the build directory in a nice API. + # NOTE: The windows VSE build does not seem to use different directories + # for Debug and Release. + build_directory = self._executive.run_command([build_directory_script_path, "--top-level"]).rstrip() + return os.path.join(build_directory, "obj") + + def main(self): + obj_directory = self._obj_directory() + build_log_paths = self._find_buildlogs(obj_directory) + + print "Found %s Visual Studio Express Build Logs:\n%s" % (len(build_log_paths), "\n".join(build_log_paths)) + + for build_log_path in build_log_paths: + print "%s:\n" % build_log_path + with codecs.open(build_log_path, "r", "utf-16") as build_log: + print build_log.read() + + +if __name__ == '__main__': + PrintVisualStudioExpressLogs().main() diff --git a/WebKitTools/Scripts/webkitdirs.pm b/WebKitTools/Scripts/webkitdirs.pm index fa85667..2c1d8da 100644 --- a/WebKitTools/Scripts/webkitdirs.pm +++ b/WebKitTools/Scripts/webkitdirs.pm @@ -1186,6 +1186,12 @@ sub buildXCodeProject($$@) return system "xcodebuild", "-project", "$project.xcodeproj", @extraOptions; } +sub usingVisualStudioExpress() +{ + determineConfigurationForVisualStudio(); + return $willUseVCExpressWhenBuilding; +} + sub buildVisualStudioProject { my ($project, $clean) = @_; diff --git a/WebKitTools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl b/WebKitTools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl new file mode 100644 index 0000000..8bd8e90 --- /dev/null +++ b/WebKitTools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl @@ -0,0 +1,56 @@ +#!/usr/bin/perl +# +# Copyright (C) Research In Motion Limited 2010. 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 Research In Motion Limited 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 of VCSUtils::removeEOL(). + +use Test::Simple tests => 5; +use VCSUtils; + +my $title; + +# New test +$title = "removeEOL: Undefined argument."; +ok(removeEOL(undef) eq ""); + +# New test +$title = "removeEOL: Line with Windows line ending."; +ok(removeEOL("This line ends with a Windows line ending.\r\n") eq "This line ends with a Windows line ending."); + +# New test +$title = "removeEOL: Line with Unix line ending."; +ok(removeEOL("This line ends with a Unix line ending.\n") eq "This line ends with a Unix line ending."); + +# New test +$title = "removeEOL: Line with Mac line ending."; +ok(removeEOL("This line ends with a Mac line ending.\r") eq "This line ends with a Mac line ending."); + +# New test +$title = "removeEOL: Line with a mix of line endings."; +ok(removeEOL("This line contains a mix of line endings.\r\n\r\n\r\r\n\n\n\n") eq "This line contains a mix of line endings."); diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/__init__.py b/WebKitTools/Scripts/webkitpy/common/checkout/__init__.py index ef65bee..597dcbd 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/__init__.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/__init__.py @@ -1 +1,3 @@ # Required for Python to search this directory for module files + +from api import Checkout diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/api.py b/WebKitTools/Scripts/webkitpy/common/checkout/api.py index 72cad8d..dbe1e84 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/api.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/api.py @@ -32,6 +32,7 @@ import StringIO from webkitpy.common.checkout.changelog import ChangeLog from webkitpy.common.checkout.commitinfo import CommitInfo from webkitpy.common.checkout.scm import CommitMessage +from webkitpy.common.memoized import memoized from webkitpy.common.net.bugzilla import parse_bug_id from webkitpy.common.system.executive import Executive, run_command, ScriptError from webkitpy.common.system.deprecated_logging import log @@ -59,6 +60,7 @@ class Checkout(object): changed_files = self._scm.changed_files_for_revision(revision) return [self._latest_entry_for_changelog_at_revision(path, revision) for path in changed_files if self._is_path_to_changelog(path)] + @memoized def commit_info_for_revision(self, revision): committer_email = self._scm.committer_email_for_revision(revision) changelog_entries = self.changelog_entries_for_revision(revision) @@ -98,8 +100,8 @@ class Checkout(object): def modified_non_changelogs(self, git_commit, changed_files=None): return self._modified_files_matching_predicate(git_commit, lambda path: not self._is_path_to_changelog(path), changed_files=changed_files) - def commit_message_for_this_commit(self, git_commit): - changelog_paths = self.modified_changelogs(git_commit) + def commit_message_for_this_commit(self, git_commit, changed_files=None): + changelog_paths = self.modified_changelogs(git_commit, changed_files) 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" @@ -120,16 +122,16 @@ class Checkout(object): revisions = set(sum(map(self._scm.revisions_changing_file, paths), [])) return set(map(self.commit_info_for_revision, revisions)) - def suggested_reviewers(self, git_commit): - changed_files = self.modified_non_changelogs(git_commit) + def suggested_reviewers(self, git_commit, changed_files=None): + changed_files = self.modified_non_changelogs(git_commit, changed_files) commit_infos = self.recent_commit_infos_for_files(changed_files) reviewers = [commit_info.reviewer() for commit_info in commit_infos if commit_info.reviewer()] reviewers.extend([commit_info.author() for commit_info in commit_infos if commit_info.author() and commit_info.author().can_review]) return sorted(set(reviewers)) - def bug_id_for_this_commit(self, git_commit): + def bug_id_for_this_commit(self, git_commit, changed_files=None): try: - return parse_bug_id(self.commit_message_for_this_commit(git_commit).message()) + return parse_bug_id(self.commit_message_for_this_commit(git_commit, changed_files).message()) except ScriptError, e: pass # We might not have ChangeLogs. diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py b/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py index d7bd95e..1f97abd 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py @@ -114,7 +114,7 @@ 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: ["ChangeLog1", "ChangeLog2"] + checkout.modified_changelogs = lambda git_commit, changed_files=None: ["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, @@ -163,7 +163,7 @@ 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: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines()) + checkout.commit_message_for_this_commit = lambda git_commit, changed_files=None: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines()) self.assertEqual(checkout.bug_id_for_this_commit(git_commit=None), 36629) def test_modified_changelogs(self): diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/scm.py b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py index 4bd9ed6..9b602c3 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/scm.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py @@ -36,6 +36,7 @@ import shutil from webkitpy.common.system.executive import Executive, run_command, ScriptError from webkitpy.common.system.deprecated_logging import error, log +from webkitpy.common.memoized import memoized def find_checkout_root(): @@ -319,7 +320,6 @@ class SVN(SCM): def __init__(self, cwd): SCM.__init__(self, cwd) - self.cached_version = None self._bogus_dir = None @staticmethod @@ -369,16 +369,20 @@ class SVN(SCM): find_output = self.run(find_args, cwd=home_directory, error_handler=Executive.ignore_error).rstrip() return find_output and os.path.isfile(os.path.join(home_directory, find_output)) + @memoized def svn_version(self): - if not self.cached_version: - self.cached_version = self.run(['svn', '--version', '--quiet']) - - return self.cached_version + return self.run(['svn', '--version', '--quiet']) def working_directory_is_clean(self): return self.run(["svn", "diff"], cwd=self.checkout_root, decode_output=False) == "" def clean_working_directory(self): + # Make sure there are no locks lying around from a previously aborted svn invocation. + # This is slightly dangerous, as it's possible the user is running another svn process + # on this checkout at the same time. However, it's much more likely that we're running + # under windows and svn just sucks (or the user interrupted svn and it failed to clean up). + self.run(["svn", "cleanup"], cwd=self.checkout_root) + # svn revert -R is not as awesome as git reset --hard. # It will leave added files around, causing later svn update # calls to fail on the bots. We make this mirror git reset --hard @@ -432,6 +436,7 @@ class SVN(SCM): def revisions_changing_file(self, path, limit=5): revisions = [] + # svn log will exit(1) (and thus self.run will raise) if the path does not exist. log_command = ['svn', 'log', '--quiet', '--limit=%s' % limit, path] for line in self.run(log_command, cwd=self.checkout_root).splitlines(): match = re.search('^r(?P<revision>\d+) ', line) @@ -565,6 +570,7 @@ class SVN(SCM): dir, base = os.path.split(path) return self.run(['svn', 'pget', pname, base], cwd=dir).encode('utf-8').rstrip("\n") + # All git-specific logic should go here. class Git(SCM): def __init__(self, cwd): @@ -667,7 +673,8 @@ class Git(SCM): return self._changes_files_for_commit(commit_id) def revisions_changing_file(self, path, limit=5): - commit_ids = self.run(["git", "log", "--pretty=format:%H", "-%s" % limit, path]).splitlines() + # git rev-list head --remove-empty --limit=5 -- path would be equivalent. + commit_ids = self.run(["git", "log", "--remove-empty", "--pretty=format:%H", "-%s" % limit, "--", path]).splitlines() return filter(lambda revision: revision, map(self.svn_revision_from_git_commit, commit_ids)) def conflicted_files(self): @@ -696,21 +703,29 @@ class Git(SCM): # 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), "--"] + changed_files, decode_output=False) - @classmethod - def git_commit_from_svn_revision(cls, revision): - # FIXME: This should probably use cwd=self.checkout_root - git_commit = run_command(['git', 'svn', 'find-rev', 'r%s' % revision]).rstrip() - # git svn find-rev always exits 0, even when the revision is not found. - if not git_commit: - raise ScriptError(message='Failed to find git commit for revision %s, your checkout likely needs an update.' % revision) - return git_commit + def _run_git_svn_find_rev(self, arg): + # git svn find-rev always exits 0, even when the revision or commit is not found. + return self.run(['git', 'svn', 'find-rev', arg], cwd=self.checkout_root).rstrip() - def svn_revision_from_git_commit(self, commit_id): + def _string_to_int_or_none(self, string): try: - return int(self.run(['git', 'svn', 'find-rev', commit_id]).rstrip()) + return int(string) except ValueError, e: return None + @memoized + def git_commit_from_svn_revision(self, svn_revision): + git_commit = self._run_git_svn_find_rev('r%s' % svn_revision) + if not git_commit: + # FIXME: Alternatively we could offer to update the checkout? Or return None? + raise ScriptError(message='Failed to find git commit for revision %s, your checkout likely needs an update.' % svn_revision) + return git_commit + + @memoized + def svn_revision_from_git_commit(self, git_commit): + svn_revision = self._run_git_svn_find_rev(git_commit) + return self._string_to_int_or_none(svn_revision) + def contents_at_revision(self, path, revision): """Returns a byte array (str()) containing the contents of path @ revision in the repository.""" diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py b/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py index 4aa5279..8af9ad5 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py @@ -760,6 +760,15 @@ Q1dTBx0AAAB42itg4GlgYJjGwMDDyODMxMDw34GBgQEAJPQDJA== self.do_test_diff_for_file() self.assertFalse(os.path.exists(self.bogus_dir)) + def test_svn_lock(self): + svn_root_lock_path = ".svn/lock" + write_into_file_at_path(svn_root_lock_path, "", "utf-8") + # webkit-patch uses a Checkout object and runs update-webkit, just use svn update here. + self.assertRaises(ScriptError, run_command, ['svn', 'update']) + self.scm.clean_working_directory() + self.assertFalse(os.path.exists(svn_root_lock_path)) + run_command(['svn', 'update']) # Should succeed and not raise. + class GitTest(SCMTest): diff --git a/WebKitTools/Scripts/webkitpy/common/config/__init__.py b/WebKitTools/Scripts/webkitpy/common/config/__init__.py index 62d129e..ef65bee 100644 --- a/WebKitTools/Scripts/webkitpy/common/config/__init__.py +++ b/WebKitTools/Scripts/webkitpy/common/config/__init__.py @@ -1,6 +1 @@ # Required for Python to search this directory for module files - -import re - -codereview_server_host = "wkrietveld.appspot.com" -codereview_server_url = "https://%s/" % codereview_server_host diff --git a/WebKitTools/Scripts/webkitpy/common/config/committers.py b/WebKitTools/Scripts/webkitpy/common/config/committers.py index f768cf9..71d764c 100644 --- a/WebKitTools/Scripts/webkitpy/common/config/committers.py +++ b/WebKitTools/Scripts/webkitpy/common/config/committers.py @@ -137,6 +137,7 @@ committers_unable_to_review = [ Committer("Keishi Hattori", "keishi@webkit.org", "keishi"), Committer("Kelly Norton", "knorton@google.com"), Committer("Kent Hansen", "kent.hansen@nokia.com", "khansen"), + Committer("Kimmo Kinnunen", ["kimmo.t.kinnunen@nokia.com", "kimmok@iki.fi", "ktkinnun@webkit.org"], "kimmok"), Committer("Kinuko Yasuda", "kinuko@chromium.org", "kinuko"), Committer("Krzysztof Kowalczyk", "kkowalczyk@gmail.com"), Committer("Kwang Yul Seo", ["kwangyul.seo@gmail.com", "skyul@company100.net", "kseo@webkit.org"], "kwangseo"), @@ -146,6 +147,7 @@ committers_unable_to_review = [ Committer("Luiz Agostini", ["luiz@webkit.org", "luiz.agostini@openbossa.org"], "lca"), Committer("Mads Ager", "ager@chromium.org"), Committer("Marcus Voltis Bulach", "bulach@chromium.org"), + Committer("Mario Sanchez Prada", ["msanchez@igalia.com", "mario@webkit.org"]), Committer("Matt Delaney", "mdelaney@apple.com"), Committer("Matt Lilek", ["webkit@mattlilek.com", "pewtermoose@webkit.org"]), Committer("Matt Perry", "mpcomplete@chromium.org"), @@ -213,7 +215,7 @@ reviewers_list = [ Reviewer("Andreas Kling", ["kling@webkit.org", "andreas.kling@nokia.com"], "kling"), Reviewer("Antonio Gomes", ["tonikitoo@webkit.org", "agomes@rim.com"], "tonikitoo"), Reviewer("Antti Koivisto", ["koivisto@iki.fi", "antti@apple.com", "antti.j.koivisto@nokia.com"], "anttik"), - Reviewer("Ariya Hidayat", ["ariya@sencha.com", "ariya.hidayat@gmail.com", "ariya@webkit.org"], "ariya"), + Reviewer("Ariya Hidayat", ["ariya.hidayat@gmail.com", "ariya@sencha.com", "ariya@webkit.org"], "ariya"), Reviewer("Beth Dakin", "bdakin@apple.com", "dethbakin"), Reviewer("Brady Eidson", "beidson@apple.com", "bradee-oh"), Reviewer("Cameron Zwarich", ["zwarich@apple.com", "cwzwarich@apple.com", "cwzwarich@webkit.org"]), diff --git a/WebKitTools/Scripts/webkitpy/common/memoized.py b/WebKitTools/Scripts/webkitpy/common/memoized.py new file mode 100644 index 0000000..dc844a5 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/common/memoized.py @@ -0,0 +1,55 @@ +# 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. + +# Python does not (yet) seem to provide automatic memoization. So we've +# written a small decorator to do so. + +import functools + + +class memoized(object): + def __init__(self, function): + self._function = function + self._results_cache = {} + + def __call__(self, *args): + try: + return self._results_cache[args] + except KeyError: + # If we didn't find the args in our cache, call and save the results. + result = self._function(*args) + self._results_cache[args] = result + return result + # FIXME: We may need to handle TypeError here in the case + # that "args" is not a valid dictionary key. + + # Use python "descriptor" protocol __get__ to appear + # invisible during property access. + def __get__(self, instance, owner): + # Return a function partial with obj already bound as self. + return functools.partial(self.__call__, instance) diff --git a/WebKitTools/Scripts/webkitpy/common/memoized_unittest.py b/WebKitTools/Scripts/webkitpy/common/memoized_unittest.py new file mode 100644 index 0000000..dd7c793 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/common/memoized_unittest.py @@ -0,0 +1,65 @@ +# 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. + +import unittest + +from webkitpy.common.memoized import memoized + + +class _TestObject(object): + def __init__(self): + self.callCount = 0 + + @memoized + def memoized_add(self, argument): + """testing docstring""" + self.callCount += 1 + if argument is None: + return None # Avoid the TypeError from None + 1 + return argument + 1 + + +class MemoizedTest(unittest.TestCase): + def test_caching(self): + test = _TestObject() + test.callCount = 0 + self.assertEqual(test.memoized_add(1), 2) + self.assertEqual(test.callCount, 1) + self.assertEqual(test.memoized_add(1), 2) + self.assertEqual(test.callCount, 1) + + # Validate that callCount is working as expected. + self.assertEqual(test.memoized_add(2), 3) + self.assertEqual(test.callCount, 2) + + def test_tearoff(self): + test = _TestObject() + # Make sure that get()/tear-offs work: + tearoff = test.memoized_add + self.assertEqual(tearoff(4), 5) + self.assertEqual(test.callCount, 1) diff --git a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py index 94519a7..1cc8e2e 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py +++ b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py @@ -113,9 +113,6 @@ class Attachment(object): def commit_queue(self): return self._attachment_dictionary.get("commit-queue") - def in_rietveld(self): - return self._attachment_dictionary.get("in-rietveld") - def url(self): # FIXME: This should just return # self._bugzilla().attachment_url_for_id(self.id()). scm_unittest.py @@ -221,9 +218,6 @@ class Bug(object): # a valid committer. return filter(lambda patch: patch.committer(), patches) - def in_rietveld_queue_patches(self): - return [patch for patch in self.patches() if patch.in_rietveld() == None] - # A container for all of the logic for making and parsing buzilla queries. class BugzillaQueries(object): @@ -287,16 +281,6 @@ class BugzillaQueries(object): return sum([self._fetch_bug(bug_id).commit_queued_patches() for bug_id in self.fetch_bug_ids_from_commit_queue()], []) - def fetch_first_patch_from_rietveld_queue(self): - # rietveld-queue processes all patches that don't have in-rietveld set. - query_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=notsubstring&value0-0-0=in-rietveld&field0-1-0=attachments.ispatch&type0-1-0=equals&value0-1-0=1&order=Last+Changed&field0-2-0=attachments.isobsolete&type0-2-0=equals&value0-2-0=0" - bugs = self._fetch_bug_ids_advanced_query(query_url) - if not len(bugs): - return None - - patches = self._fetch_bug(bugs[0]).in_rietveld_queue_patches() - return patches[0] if len(patches) else None - def _fetch_bug_ids_from_review_queue(self): review_queue_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review?" return self._fetch_bug_ids_advanced_query(review_queue_url) @@ -502,8 +486,6 @@ class Bugzilla(object): self._parse_attachment_flag( element, 'review', attachment, 'reviewer_email') self._parse_attachment_flag( - element, 'in-rietveld', attachment, 'rietveld_uploader_email') - self._parse_attachment_flag( element, 'commit-queue', attachment, 'committer_email') return attachment @@ -591,11 +573,12 @@ class Bugzilla(object): self.authenticated = True return + credentials = Credentials(self.bug_server_host, git_prefix="bugzilla") + attempts = 0 while not self.authenticated: attempts += 1 - (username, password) = Credentials( - self.bug_server_host, git_prefix="bugzilla").read_credentials() + username, password = credentials.read_credentials() log("Logging in as %s..." % username) self.browser.open(self.bug_server_url + @@ -766,8 +749,6 @@ class Bugzilla(object): return self.browser.find_control(type='select', nr=0) elif flag_name == "commit-queue": return self.browser.find_control(type='select', nr=1) - elif flag_name == "in-rietveld": - return self.browser.find_control(type='select', nr=2) raise Exception("Don't know how to find flag named \"%s\"" % flag_name) def clear_attachment_flags(self, diff --git a/WebKitTools/Scripts/webkitpy/common/net/bugzilla_unittest.py b/WebKitTools/Scripts/webkitpy/common/net/bugzilla_unittest.py index 3a454d6..df1fcf6 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/bugzilla_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/net/bugzilla_unittest.py @@ -83,11 +83,6 @@ class BugzillaTest(unittest.TestCase): status="+" setter="two@test.com" /> - <flag name="in-rietveld" - id="17933" - status="+" - setter="three@test.com" - /> </attachment> ''' _expected_example_attachment_parsing = { @@ -103,8 +98,6 @@ class BugzillaTest(unittest.TestCase): 'reviewer_email' : 'one@test.com', 'commit-queue' : '+', 'committer_email' : 'two@test.com', - 'in-rietveld': '+', - 'rietveld_uploader_email': 'three@test.com', 'attacher_email' : 'christian.plesner.hansen@gmail.com', } diff --git a/WebKitTools/Scripts/webkitpy/common/net/credentials.py b/WebKitTools/Scripts/webkitpy/common/net/credentials.py index 1c3e6c0..30480b3 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/credentials.py +++ b/WebKitTools/Scripts/webkitpy/common/net/credentials.py @@ -48,6 +48,7 @@ except ImportError: class Credentials(object): + _environ_prefix = "webkit_bugzilla_" def __init__(self, host, git_prefix=None, executive=None, cwd=os.getcwd(), keyring=keyring): @@ -58,8 +59,17 @@ class Credentials(object): self._keyring = keyring def _credentials_from_git(self): - return [Git.read_git_config(self.git_prefix + "username"), - Git.read_git_config(self.git_prefix + "password")] + try: + if not Git.in_working_directory(self.cwd): + return (None, None) + return (Git.read_git_config(self.git_prefix + "username"), + Git.read_git_config(self.git_prefix + "password")) + except OSError, e: + # Catch and ignore OSError exceptions such as "no such file + # or directory" (OSError errno 2), which imply that the Git + # command cannot be found/is not installed. + pass + return (None, None) def _keychain_value_with_label(self, label, source_text): match = re.search("%s\"(?P<value>.+)\"" % label, @@ -110,21 +120,28 @@ class Credentials(object): else: return [None, None] - def read_credentials(self): - username = None - password = None + def _read_environ(self, key): + environ_key = self._environ_prefix + key + return os.environ.get(environ_key.upper()) - try: - if Git.in_working_directory(self.cwd): - (username, password) = self._credentials_from_git() - except OSError, e: - # Catch and ignore OSError exceptions such as "no such file - # or directory" (OSError errno 2), which imply that the Git - # command cannot be found/is not installed. - pass + def _credentials_from_environment(self): + return (self._read_environ("username"), self._read_environ("password")) + + def _offer_to_store_credentials_in_keyring(self, username, password): + if not self._keyring: + return + if not User().confirm("Store password in system keyring?", User.DEFAULT_NO): + return + self._keyring.set_password(self.host, username, password) + def read_credentials(self): + username, password = self._credentials_from_environment() + # FIXME: We don't currently support pulling the username from one + # source and the password from a separate source. + if not username or not password: + username, password = self._credentials_from_git() if not username or not password: - (username, password) = self._credentials_from_keychain(username) + username, password = self._credentials_from_keychain(username) if username and not password and self._keyring: password = self._keyring.get_password(self.host, username) @@ -132,13 +149,7 @@ class Credentials(object): if not username: username = User.prompt("%s login: " % self.host) if not password: - password = getpass.getpass("%s password for %s: " % (self.host, - username)) + password = getpass.getpass("%s password for %s: " % (self.host, username)) + self._offer_to_store_credentials_in_keyring(username, password) - if self._keyring: - store_password = User().confirm( - "Store password in system keyring?", User.DEFAULT_NO) - if store_password: - self._keyring.set_password(self.host, username, password) - - return [username, password] + return (username, password) diff --git a/WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py b/WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py index d30291b..6f2d909 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py @@ -26,6 +26,8 @@ # (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 __future__ import with_statement + import os import tempfile import unittest @@ -34,6 +36,21 @@ from webkitpy.common.system.executive import Executive from webkitpy.common.system.outputcapture import OutputCapture from webkitpy.thirdparty.mock import Mock + +# FIXME: Other unit tests probably want this class. +class _TemporaryDirectory(object): + def __init__(self, **kwargs): + self._kwargs = kwargs + self._directory_path = None + + def __enter__(self): + self._directory_path = tempfile.mkdtemp(**self._kwargs) + return self._directory_path + + def __exit__(self, type, value, traceback): + os.rmdir(self._directory_path) + + class CredentialsTest(unittest.TestCase): example_security_output = """keychain: "/Users/test/Library/Keychains/login.keychain" class: "inet" @@ -101,40 +118,59 @@ password: "SECRETSAUCE" self._assert_security_call() self._assert_security_call(username="foo") + def test_credentials_from_environment(self): + executive_mock = Mock() + credentials = Credentials("example.com", executive=executive_mock) + + saved_environ = os.environ.copy() + os.environ['WEBKIT_BUGZILLA_USERNAME'] = "foo" + os.environ['WEBKIT_BUGZILLA_PASSWORD'] = "bar" + username, password = credentials._credentials_from_environment() + self.assertEquals(username, "foo") + self.assertEquals(password, "bar") + os.environ = saved_environ + def test_read_credentials_without_git_repo(self): + # FIXME: This should share more code with test_keyring_without_git_repo class FakeCredentials(Credentials): def _is_mac_os_x(self): return True + def _credentials_from_keychain(self, username): - return ["test@webkit.org", "SECRETSAUCE"] + return ("test@webkit.org", "SECRETSAUCE") + + def _credentials_from_environment(self): + return (None, None) + + with _TemporaryDirectory(suffix="not_a_git_repo") as temp_dir_path: + credentials = FakeCredentials("bugs.webkit.org", cwd=temp_dir_path) + # FIXME: Using read_credentials here seems too broad as higher-priority + # credential source could be affected by the user's environment. + self.assertEqual(credentials.read_credentials(), ("test@webkit.org", "SECRETSAUCE")) - temp_dir_path = tempfile.mkdtemp(suffix="not_a_git_repo") - credentials = FakeCredentials("bugs.webkit.org", cwd=temp_dir_path) - self.assertEqual(credentials.read_credentials(), ["test@webkit.org", "SECRETSAUCE"]) - os.rmdir(temp_dir_path) def test_keyring_without_git_repo(self): + # FIXME: This should share more code with test_read_credentials_without_git_repo class MockKeyring(object): def get_password(self, host, username): return "NOMNOMNOM" class FakeCredentials(Credentials): - def __init__(self, cwd): - Credentials.__init__(self, "fake.hostname", cwd=cwd, - keyring=MockKeyring()) - def _is_mac_os_x(self): return True def _credentials_from_keychain(self, username): return ("test@webkit.org", None) - temp_dir_path = tempfile.mkdtemp(suffix="not_a_git_repo") - credentials = FakeCredentials(temp_dir_path) - try: - self.assertEqual(credentials.read_credentials(), ["test@webkit.org", "NOMNOMNOM"]) - finally: - os.rmdir(temp_dir_path) + def _credentials_from_environment(self): + return (None, None) + + with _TemporaryDirectory(suffix="not_a_git_repo") as temp_dir_path: + credentials = FakeCredentials("fake.hostname", cwd=temp_dir_path, keyring=MockKeyring()) + # FIXME: Using read_credentials here seems too broad as higher-priority + # credential source could be affected by the user's environment. + self.assertEqual(credentials.read_credentials(), ("test@webkit.org", "NOMNOMNOM")) + if __name__ == '__main__': unittest.main() diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py deleted file mode 100644 index b9a0821..0000000 --- a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py +++ /dev/null @@ -1,79 +0,0 @@ -# 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. - -import logging -import os -import re -import stat - -import webkitpy.common.config as config -from webkitpy.common.system.deprecated_logging import log -from webkitpy.common.system.executive import ScriptError -import webkitpy.thirdparty.autoinstalled.rietveld.upload as upload - - -class Rietveld(object): - def __init__(self, executive, dryrun=False): - self.dryrun = dryrun - self._executive = executive - - def url_for_issue(self, codereview_issue): - if not codereview_issue: - return None - return "%s%s" % (config.codereview_server_url, codereview_issue) - - def post(self, diff, patch_id, codereview_issue, message=None, cc=None): - if not message: - raise ScriptError("Rietveld requires a message.") - - # Rietveld has a 100 character limit on message length. - if len(message) > 100: - message = message[:100] - - args = [ - # First argument is empty string to mimic sys.argv. - "", - "--assume_yes", - "--server=%s" % config.codereview_server_host, - "--message=%s" % message, - "--webkit_patch_id=%s" % patch_id, - ] - if codereview_issue: - args.append("--issue=%s" % codereview_issue) - if cc: - args.append("--cc=%s" % cc) - - if self.dryrun: - log("Would have run %s" % args) - return - - # 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 --git-commit. - issue, patchset = upload.RealMain(args, data=diff) - return issue diff --git a/WebKitTools/Scripts/webkitpy/common/net/statusserver.py b/WebKitTools/Scripts/webkitpy/common/net/statusserver.py index 3d03dcd..64dd77b 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/statusserver.py +++ b/WebKitTools/Scripts/webkitpy/common/net/statusserver.py @@ -127,7 +127,7 @@ class StatusServer: self._browser.submit() def release_work_item(self, queue_name, patch): - _log.debug("Releasing work item %s from %s" % (patch.id(), queue_name)) + _log.info("Releasing work item %s from %s" % (patch.id(), queue_name)) return NetworkTransaction(convert_404_to_None=True).run(lambda: self._post_release_work_item(queue_name, patch)) def update_work_items(self, queue_name, work_items): 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 e0fd1b6..3e3ba0b 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 @@ -51,6 +51,7 @@ import time import traceback import test_failures +import test_results _log = logging.getLogger("webkitpy.layout_tests.layout_package." "dump_render_tree_thread") @@ -133,8 +134,8 @@ def _process_output(port, options, test_info, test_types, test_args, time.time() - start_diff_time) total_time_for_all_diffs = time.time() - start_diff_time - return TestResult(test_info.filename, failures, test_run_time, - total_time_for_all_diffs, time_for_diffs) + return test_results.TestResult(test_info.filename, failures, test_run_time, + total_time_for_all_diffs, time_for_diffs) def _pad_timeout(timeout): @@ -152,16 +153,11 @@ def _milliseconds_to_seconds(msecs): return float(msecs) / 1000.0 -class TestResult(object): - - def __init__(self, filename, failures, test_run_time, - total_time_for_all_diffs, time_for_diffs): - self.failures = failures - self.filename = filename - self.test_run_time = test_run_time - self.time_for_diffs = time_for_diffs - self.total_time_for_all_diffs = total_time_for_all_diffs - self.type = test_failures.determine_result_type(failures) +def _image_hash(test_info, test_args, options): + """Returns the image hash of the test if it's needed, otherwise None.""" + if (test_args.new_baseline or test_args.reset_results or not options.pixel_tests): + return None + return test_info.image_hash() class SingleTestThread(threading.Thread): @@ -196,10 +192,11 @@ class SingleTestThread(threading.Thread): self._driver = self._port.create_driver(self._test_args.png_path, self._options) self._driver.start() + image_hash = _image_hash(test_info, self._test_args, self._options) start = time.time() crash, timeout, actual_checksum, output, error = \ self._driver.run_test(test_info.uri.strip(), test_info.timeout, - test_info.image_hash()) + image_hash) end = time.time() self._test_result = _process_output(self._port, self._options, test_info, self._test_types, self._test_args, @@ -256,8 +253,8 @@ class TestShellThread(WatchableThread): options: command line options argument from optparse filename_list_queue: A thread safe Queue class that contains lists of tuples of (filename, uri) pairs. - result_queue: A thread safe Queue class that will contain tuples of - (test, failure lists) for the test results. + result_queue: A thread safe Queue class that will contain + serialized TestResult objects. test_types: A list of TestType objects to run the test output against. test_args: A TestArguments object to pass to each TestType. @@ -441,7 +438,7 @@ class TestShellThread(WatchableThread): else: _log.debug("%s %s passed" % (self.getName(), self._port.relative_test_filename(filename))) - self._result_queue.put(result) + self._result_queue.put(result.dumps()) if batch_size > 0 and batch_count > batch_size: # Bounce the shell and reset count. @@ -497,9 +494,8 @@ class TestShellThread(WatchableThread): failures = [] _log.error('Cannot get results of test: %s' % test_info.filename) - result = TestResult(test_info.filename, failures=[], - test_run_time=0, total_time_for_all_diffs=0, - time_for_diffs=0) + result = test_results.TestResult(test_info.filename, failures=[], + test_run_time=0, total_time_for_all_diffs=0, time_for_diffs=0) return result @@ -509,20 +505,14 @@ class TestShellThread(WatchableThread): Args: test_info: Object containing the test filename, uri and timeout - Returns: - A list of TestFailure objects describing the error. - + Returns: a TestResult object. """ self._ensure_dump_render_tree_is_running() # The pixel_hash is used to avoid doing an image dump if the # checksums match, so it should be set to a blank value if we # are generating a new baseline. (Otherwise, an image from a # 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 or - not self._options.pixel_tests)): - image_hash = "" + image_hash = _image_hash(test_info, self._test_args, self._options) start = time.time() thread_timeout = _milliseconds_to_seconds( 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 c6c3066..1cf88ef 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 @@ -42,7 +42,6 @@ class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGeneratorBase # Additional JSON fields. 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 @@ -167,9 +166,6 @@ class JSONLayoutResultsGenerator(json_results_generator.JSONResultsGeneratorBase len(self._expectations.get_tests_with_timeline( test_expectations.NOW)), self.ALL_FIXABLE_COUNT) self._insert_item_into_raw_list(results_for_builder, - self._get_failure_summary_entry(test_expectations.DEFER), - self.DEFERRED) - self._insert_item_into_raw_list(results_for_builder, self._get_failure_summary_entry(test_expectations.WONTFIX), self.WONTFIX) diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py index 0344aa7..9a0f4ee 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py @@ -40,7 +40,7 @@ from webkitpy.common import array_stream from webkitpy.common.system import logtesting from webkitpy.layout_tests import port from webkitpy.layout_tests.layout_package import printing -from webkitpy.layout_tests.layout_package import dump_render_tree_thread +from webkitpy.layout_tests.layout_package import test_results from webkitpy.layout_tests.layout_package import test_expectations from webkitpy.layout_tests.layout_package import test_failures from webkitpy.layout_tests import run_webkit_tests @@ -141,9 +141,9 @@ class Testprinter(unittest.TestCase): elif result_type == test_expectations.CRASH: failures = [test_failures.FailureCrash()] path = os.path.join(self._port.layout_tests_dir(), test) - return dump_render_tree_thread.TestResult(path, failures, run_time, - total_time_for_all_diffs=0, - time_for_diffs=0) + return test_results.TestResult(path, failures, run_time, + total_time_for_all_diffs=0, + time_for_diffs=0) def get_result_summary(self, tests, expectations_str): test_paths = [os.path.join(self._port.layout_tests_dir(), test) for diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py index 508a6ad..67873a8 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py @@ -43,7 +43,7 @@ _log = logging.getLogger("webkitpy.layout_tests.layout_package." # Test expectation and modifier constants. (PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, TIMEOUT, CRASH, SKIP, WONTFIX, - DEFER, SLOW, REBASELINE, MISSING, FLAKY, NOW, NONE) = range(16) + SLOW, REBASELINE, MISSING, FLAKY, NOW, NONE) = range(15) # Test expectation file update action constants (NO_CHANGE, REMOVE_TEST, REMOVE_PLATFORM, ADD_PLATFORMS_EXCEPT_THIS) = range(4) @@ -228,12 +228,11 @@ class TestExpectationsFile: DEBUG : LayoutTests/fast/js/no-good.js = TIMEOUT PASS DEBUG SKIP : LayoutTests/fast/js/no-good.js = TIMEOUT PASS LINUX DEBUG SKIP : LayoutTests/fast/js/no-good.js = TIMEOUT PASS - DEFER LINUX WIN : LayoutTests/fast/js/no-good.js = TIMEOUT PASS + LINUX WIN : LayoutTests/fast/js/no-good.js = TIMEOUT PASS SKIP: Doesn't run the test. SLOW: The test takes a long time to run, but does not timeout indefinitely. WONTFIX: For tests that we never intend to pass on a given platform. - DEFER: Test does not count in our statistics for the current release. DEBUG: Expectations apply only to the debug build. RELEASE: Expectations apply only to release build. LINUX/WIN/WIN-XP/WIN-VISTA/WIN-7/MAC: Expectations apply only to these @@ -241,7 +240,6 @@ class TestExpectationsFile: Notes: -A test cannot be both SLOW and TIMEOUT - -A test cannot be both DEFER and WONTFIX -A test should only be one of IMAGE, TEXT, IMAGE+TEXT, or FAIL. FAIL is a migratory state that currently means either IMAGE, TEXT, or IMAGE+TEXT. Once we have finished migrating the expectations, we will @@ -249,7 +247,7 @@ class TestExpectationsFile: identifier. -A test can be included twice, but not via the same path. -If a test is included twice, then the more precise path wins. - -CRASH tests cannot be DEFER or WONTFIX + -CRASH tests cannot be WONTFIX """ EXPECTATIONS = {'pass': PASS, @@ -282,14 +280,12 @@ class TestExpectationsFile: MODIFIERS = {'skip': SKIP, 'wontfix': WONTFIX, - 'defer': DEFER, 'slow': SLOW, 'rebaseline': REBASELINE, 'none': NONE} TIMELINES = {'wontfix': WONTFIX, - 'now': NOW, - 'defer': DEFER} + 'now': NOW} RESULT_TYPES = {'skip': SKIP, 'pass': PASS, @@ -621,10 +617,6 @@ class TestExpectationsFile: if not self._is_debug_mode and 'release' not in options: return False - if 'wontfix' in options and 'defer' in options: - self._add_error(lineno, 'Test cannot be both DEFER and WONTFIX.', - test_and_expectations) - if self._is_lint_mode and 'rebaseline' in options: self._add_error(lineno, 'REBASELINE should only be used for running rebaseline.py. ' @@ -773,8 +765,6 @@ class TestExpectationsFile: if 'wontfix' in modifiers: self._timeline_to_tests[WONTFIX].add(test) - elif 'defer' in modifiers: - self._timeline_to_tests[DEFER].add(test) else: self._timeline_to_tests[NOW].add(test) diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py index 2e1b6ec..55eaf99 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py @@ -134,17 +134,12 @@ class TestExpectationsTest(Base): self.get_test('failures/expected/text.html')), set([TEXT, CRASH])) - def test_defer(self): - self.parse_exp('BUGX DEFER : failures/expected/text.html = TEXT') - self.assertEqual(self._exp.get_options( - self.get_test('failures/expected/text.html')), ['bugx', 'defer']) - def test_precedence(self): # This tests handling precedence of specific lines over directories # and tests expectations covering entire directories. exp_str = """ BUGX : failures/expected/text.html = TEXT -BUGX DEFER : failures/expected = IMAGE +BUGX WONTFIX : failures/expected = IMAGE """ self.parse_exp(exp_str) self.assert_exp('failures/expected/text.html', TEXT) @@ -227,11 +222,6 @@ BUGX DEFER : failures/expected = IMAGE self.assertRaises(SyntaxError, self.parse_exp, 'BUG_TEST SLOW : failures/expected/timeout.html = TIMEOUT') - def test_semantic_wontfix_defer(self): - # A test cannot be WONTFIX and DEFER. - self.assertRaises(SyntaxError, self.parse_exp, - 'BUG_TEST WONTFIX DEFER : failures/expected/text.html = TEXT') - def test_semantic_rebaseline(self): # Can't lint a file w/ 'REBASELINE' in it. self.assertRaises(SyntaxError, self.parse_exp, diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py index 340d075..6d55761 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py @@ -32,6 +32,8 @@ import os import test_expectations +import cPickle + def determine_result_type(failure_list): """Takes a set of test_failures and returns which result type best fits @@ -71,10 +73,25 @@ class TestFailure(object): """Abstract base class that defines the failure interface.""" @staticmethod + def loads(s): + """Creates a TestFailure object from the specified string.""" + return cPickle.loads(s) + + @staticmethod def message(): """Returns a string describing the failure in more detail.""" raise NotImplementedError + def __eq__(self, other): + return self.__class__.__name__ == other.__class__.__name__ + + def __ne__(self, other): + return self.__class__.__name__ != other.__class__.__name__ + + def dumps(self): + """Returns the string/JSON representation of a TestFailure.""" + return cPickle.dumps(self) + def result_html_output(self, filename): """Returns an HTML string to be included on the results.html page.""" raise NotImplementedError @@ -112,7 +129,7 @@ class FailureWithType(TestFailure): TestFailure.__init__(self) # Filename suffixes used by ResultHtmlOutput. - OUT_FILENAMES = [] + OUT_FILENAMES = () def output_links(self, filename, out_names): """Returns a string holding all applicable output file links. @@ -128,6 +145,10 @@ class FailureWithType(TestFailure): # FIXME: Seems like a bad idea to separate the display name data # from the path data by hard-coding the display name here # and passing in the path information via out_names. + # + # FIXME: Also, we don't know for sure that these files exist, + # and we shouldn't be creating links to files that don't exist + # (for example, if we don't actually have wdiff output). links = [''] uris = [self.relative_output_filename(filename, fn) for fn in out_names] @@ -170,7 +191,7 @@ class FailureCrash(TestFailure): return "Test shell crashed" def result_html_output(self, filename): - # TODO(tc): create a link to the minidump file + # FIXME: create a link to the minidump file stack = self.relative_output_filename(filename, "-stack.txt") return "<strong>%s</strong> <a href=%s>stack</a>" % (self.message(), stack) @@ -181,7 +202,7 @@ class FailureCrash(TestFailure): class FailureMissingResult(FailureWithType): """Expected result was missing.""" - OUT_FILENAMES = ["-actual.txt"] + OUT_FILENAMES = ("-actual.txt",) @staticmethod def message(): @@ -196,14 +217,8 @@ class FailureTextMismatch(FailureWithType): """Text diff output failed.""" # Filename suffixes used by ResultHtmlOutput. # FIXME: Why don't we use the constants from TestTypeBase here? - OUT_FILENAMES = ["-actual.txt", "-expected.txt", "-diff.txt"] - OUT_FILENAMES_WDIFF = ["-actual.txt", "-expected.txt", "-diff.txt", - "-wdiff.html", "-pretty-diff.html"] - - def __init__(self, has_wdiff): - FailureWithType.__init__(self) - if has_wdiff: - self.OUT_FILENAMES = self.OUT_FILENAMES_WDIFF + OUT_FILENAMES = ("-actual.txt", "-expected.txt", "-diff.txt", + "-wdiff.html", "-pretty-diff.html") @staticmethod def message(): @@ -214,7 +229,6 @@ class FailureMissingImageHash(FailureWithType): """Actual result hash was missing.""" # Chrome doesn't know to display a .checksum file as text, so don't bother # putting in a link to the actual result. - OUT_FILENAMES = [] @staticmethod def message(): @@ -226,7 +240,7 @@ class FailureMissingImageHash(FailureWithType): class FailureMissingImage(FailureWithType): """Actual result image was missing.""" - OUT_FILENAMES = ["-actual.png"] + OUT_FILENAMES = ("-actual.png",) @staticmethod def message(): @@ -239,7 +253,7 @@ class FailureMissingImage(FailureWithType): class FailureImageHashMismatch(FailureWithType): """Image hashes didn't match.""" - OUT_FILENAMES = ["-actual.png", "-expected.png", "-diff.png"] + OUT_FILENAMES = ("-actual.png", "-expected.png", "-diff.png") @staticmethod def message(): @@ -252,7 +266,6 @@ class FailureImageHashIncorrect(FailureWithType): """Actual result hash is incorrect.""" # Chrome doesn't know to display a .checksum file as text, so don't bother # putting in a link to the actual result. - OUT_FILENAMES = [] @staticmethod def message(): @@ -260,3 +273,10 @@ class FailureImageHashIncorrect(FailureWithType): def result_html_output(self, filename): return "<strong>%s</strong>" % self.message() + +# Convenient collection of all failure classes for anything that might +# need to enumerate over them all. +ALL_FAILURE_CLASSES = (FailureTimeout, FailureCrash, FailureMissingResult, + FailureTextMismatch, FailureMissingImageHash, + FailureMissingImage, FailureImageHashMismatch, + FailureImageHashIncorrect) diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py index 92fe276..3e3528d 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py @@ -28,13 +28,26 @@ """"Tests code paths not covered by the regular unit tests.""" -from webkitpy.layout_tests.layout_package.test_failures import * import unittest +from webkitpy.layout_tests.layout_package.test_failures import * + + class Test(unittest.TestCase): def assertResultHtml(self, failure_obj): self.assertNotEqual(failure_obj.result_html_output('foo'), None) + def assert_loads(self, cls): + failure_obj = cls() + s = failure_obj.dumps() + new_failure_obj = TestFailure.loads(s) + self.assertTrue(isinstance(new_failure_obj, cls)) + + self.assertEqual(failure_obj, new_failure_obj) + + # Also test that != is implemented. + self.assertFalse(failure_obj != new_failure_obj) + def test_crash(self): self.assertResultHtml(FailureCrash()) @@ -63,6 +76,9 @@ class Test(unittest.TestCase): self.assertRaises(NotImplementedError, failure_obj.result_html_output, "foo.txt") + def test_loads(self): + for c in ALL_FAILURE_CLASSES: + self.assert_loads(c) if __name__ == '__main__': unittest.main() diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results.py new file mode 100644 index 0000000..2417fb7 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results.py @@ -0,0 +1,61 @@ +# 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. + +import cPickle + +import test_failures + + +class TestResult(object): + """Data object containing the results of a single test.""" + + @staticmethod + def loads(str): + return cPickle.loads(str) + + def __init__(self, filename, failures, test_run_time, + total_time_for_all_diffs, time_for_diffs): + self.failures = failures + self.filename = filename + self.test_run_time = test_run_time + self.time_for_diffs = time_for_diffs + self.total_time_for_all_diffs = total_time_for_all_diffs + self.type = test_failures.determine_result_type(failures) + + def __eq__(self, other): + return (self.filename == other.filename and + self.failures == other.failures and + self.test_run_time == other.test_run_time and + self.time_for_diffs == other.time_for_diffs and + self.total_time_for_all_diffs == other.total_time_for_all_diffs) + + def __ne__(self, other): + return not (self == other) + + def dumps(self): + return cPickle.dumps(self) diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_unittest.py index 9c5a29e..5921666 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_unittest.py @@ -1,4 +1,4 @@ -# Copyright (c) 2010 Google Inc. All rights reserved. +# 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 @@ -28,12 +28,25 @@ import unittest -from webkitpy.common.net.rietveld import Rietveld -from webkitpy.thirdparty.mock import Mock +from test_results import TestResult -class RietveldTest(unittest.TestCase): - def test_url_for_issue(self): - rietveld = Rietveld(Mock()) - self.assertEqual(rietveld.url_for_issue(34223), - "https://wkrietveld.appspot.com/34223") +class Test(unittest.TestCase): + def test_loads(self): + result = TestResult(filename='foo', + failures=[], + test_run_time=1.1, + total_time_for_all_diffs=0.5, + time_for_diffs=0.5) + s = result.dumps() + new_result = TestResult.loads(s) + self.assertTrue(isinstance(new_result, TestResult)) + + self.assertEqual(new_result, result) + + # Also check that != is implemented. + self.assertFalse(new_result != result) + + +if __name__ == '__main__': + unittest.main() diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py index 93f8808..ee868e8 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py @@ -250,7 +250,7 @@ class PortTest(unittest.TestCase): abspath_to_uri(test_file)) def test_get_option__set(self): - options, args = optparse.OptionParser().parse_args() + options, args = optparse.OptionParser().parse_args([]) options.foo = 'bar' port = base.Port(options=options) self.assertEqual(port.get_option('foo'), 'bar') @@ -269,7 +269,7 @@ class PortTest(unittest.TestCase): self.assertEqual(port.get_option('foo'), 'bar') def test_set_option_default__set(self): - options, args = optparse.OptionParser().parse_args() + options, args = optparse.OptionParser().parse_args([]) options.foo = 'bar' port = base.Port(options=options) # This call should have no effect. diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py index 4d17b51..f93f9a8 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py @@ -135,7 +135,7 @@ class ChromiumPort(base.Port): override_step, logging) def diff_image(self, expected_contents, actual_contents, - diff_filename=None, tolerance=0): + diff_filename=None): executable = self._path_to_image_diff() tempdir = tempfile.mkdtemp() @@ -385,6 +385,12 @@ class ChromiumDriver(base.Driver): if self._port.get_option('gp_fault_error_box'): driver_args.append('--gp-fault-error-box') + if self._options.js_flags is not None: + driver_args.append('--js-flags="' + self._options.js_flags + '"') + + if self._options.multiple_loads is not None and self._options.multiple_loads > 0: + driver_args.append('--multiple-loads=' + str(self._options.multiple_loads)) + if self._port.get_option('accelerated_compositing'): driver_args.append('--enable-accelerated-compositing') diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py index 95c716e..5d28fae 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py @@ -69,6 +69,8 @@ def _set_gpu_options(options): options.accelerated_compositing = True if options.accelerated_2d_canvas is None: options.accelerated_2d_canvas = True + if options.builder_name is not None: + options.builder_name = options.builder_name + ' - GPU' def _gpu_overrides(port): diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py index 7a13b1c..88524fc 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py @@ -44,10 +44,12 @@ class ChromiumGpuTest(unittest.TestCase): def assertOverridesWorked(self, port_name): # test that we got the right port mock_options = mocktool.MockOptions(accelerated_compositing=None, - accelerated_2d_canvas=None) + accelerated_2d_canvas=None, + builder_name='foo') port = chromium_gpu.get(port_name=port_name, options=mock_options) self.assertTrue(port._options.accelerated_compositing) self.assertTrue(port._options.accelerated_2d_canvas) + self.assertEqual(port._options.builder_name, 'foo - GPU') # we use startswith() instead of Equal to gloss over platform versions. self.assertTrue(port.name().startswith(port_name)) diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py index cb45430..92a31fb 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py @@ -121,7 +121,7 @@ class ChromiumPortTest(unittest.TestCase): fake_test = os.path.join(port.layout_tests_dir(), "fast/js/not-good.js") port.test_expectations = lambda: """BUG_TEST SKIP : fast/js/not-good.js = TEXT -DEFER LINUX WIN : fast/js/very-good.js = TIMEOUT PASS""" +LINUX WIN : fast/js/very-good.js = TIMEOUT PASS""" port.test_expectations_overrides = lambda: '' port.tests = lambda paths: set() port.path_exists = lambda test: True diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py index 81db32c..36f3c6b 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py @@ -51,7 +51,7 @@ class ChromiumWinTest(unittest.TestCase): def test_setup_environ_for_server(self): port = chromium_win.ChromiumWinPort() - port._executive = mocktool.MockExecute(True) + port._executive = mocktool.MockExecutive(should_log=True) port.path_from_chromium_base = self._mock_path_from_chromium_base output = outputcapture.OutputCapture() orig_environ = os.environ.copy() @@ -63,7 +63,7 @@ class ChromiumWinTest(unittest.TestCase): sys.platform = "win32" port = chromium_win.ChromiumWinPort( options=ChromiumWinTest.RegisterCygwinOption()) - port._executive = mocktool.MockExecute(True) + port._executive = mocktool.MockExecutive(should_log=True) port.path_from_chromium_base = self._mock_path_from_chromium_base setup_mount = self._mock_path_from_chromium_base("third_party", "cygwin", diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py index 73200a0..b2615a3 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py @@ -71,13 +71,15 @@ class HttpLock(object): return self._extract_lock_number(lock_list[-1]) + 1 def _check_pid(self, current_pid): - """Return True if pid is alive, otherwise return False.""" - try: - os.kill(current_pid, 0) - except OSError: - return False - else: - return True + """Return True if pid is alive, otherwise return False. + FIXME: os.kill() doesn't work on Windows for checking if + a pid is alive, so always return True""" + if sys.platform in ('darwin', 'linux2'): + try: + os.kill(current_pid, 0) + except OSError: + return False + return True def _curent_lock_pid(self): """Return with the current lock pid. If the lock is not valid @@ -89,9 +91,7 @@ class HttpLock(object): current_lock_file = open(lock_list[0], 'r') current_pid = current_lock_file.readline() current_lock_file.close() - if not (current_pid and - sys.platform in ('darwin', 'linux2') and - self._check_pid(int(current_pid))): + if not (current_pid and self._check_pid(int(current_pid))): os.unlink(lock_list[0]) return except IOError, OSError: @@ -104,9 +104,7 @@ class HttpLock(object): numbers are sequential.""" while(True): try: - sequential_guard_lock = os.open(self._guard_lock_file, - os.O_CREAT | os.O_NONBLOCK | os.O_EXCL) - + sequential_guard_lock = os.open(self._guard_lock_file, os.O_CREAT | os.O_EXCL) self._process_lock_file_name = (self._lock_file_path_prefix + str(self._next_lock_number())) lock_file = open(self._process_lock_file_name, 'w') diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py index 4c8fa0a..af94acc 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py @@ -31,6 +31,7 @@ import logging import os import signal +import sys import webkit @@ -46,6 +47,17 @@ class QtPort(WebKitPort): kwargs.setdefault('port_name', 'qt') WebKitPort.__init__(self, **kwargs) + def baseline_search_path(self): + port_names = [] + if sys.platform == 'linux2': + port_names.append("qt-linux") + elif sys.platform in ('win32', 'cygwin'): + port_names.append("qt-win") + elif sys.platform == 'darwin': + port_names.append("qt-mac") + port_names.append("qt") + return map(self._webkit_baseline_path, port_names) + def _tests_for_other_platforms(self): # FIXME: This list could be dynamic based on platform name and # pushed into base.Port. @@ -92,6 +104,9 @@ class QtPort(WebKitPort): def _path_to_driver(self): return self._build_path('bin/DumpRenderTree') + def _path_to_image_diff(self): + return self._build_path('bin/ImageDiff') + def _path_to_webcore_library(self): return self._build_path('lib/libQtWebKit.so') diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py index 3691c5a..ff4086c 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py @@ -137,7 +137,7 @@ class TestPort(base.Port): return True def diff_image(self, expected_contents, actual_contents, - diff_filename=None, tolerance=0): + diff_filename=None): diffed = actual_contents != expected_contents if diffed and diff_filename: with codecs.open(diff_filename, "w", "utf-8") as diff_fh: diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py index c940f1e..0d0d3e0 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py @@ -122,22 +122,25 @@ class WebKitPort(base.Port): return True def diff_image(self, expected_contents, actual_contents, - diff_filename=None, tolerance=0.1): + diff_filename=None): """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. if not actual_contents: return True - sp = self._diff_image_request(expected_contents, actual_contents, - tolerance) + sp = self._diff_image_request(expected_contents, actual_contents) return self._diff_image_reply(sp, diff_filename) - def _diff_image_request(self, expected_contents, actual_contents, tolerance): + def _diff_image_request(self, expected_contents, actual_contents): + # FIXME: use self.get_option('tolerance') and + # self.set_option_default('tolerance', 0.1) once that behaves correctly + # with default values. + if self.get_option('tolerance') is not None: + tolerance = self.get_option('tolerance') + else: + tolerance = 0.1 command = [self._path_to_image_diff(), '--tolerance', str(tolerance)] sp = server_process.ServerProcess(self, 'ImageDiff', command) 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 a47370d..434058e 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py @@ -496,7 +496,7 @@ class Rebaseliner(object): """ if is_image: - return self._port.diff_image(output1, output2, None, 0) + return self._port.diff_image(output1, output2, None) else: return self._port.compare_text(output1, output2) diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py index ef33a47..8db31a6 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py @@ -110,7 +110,6 @@ class TestRebaseliner(unittest.TestCase): is_image=False)) def test_diff_baselines_png(self): - return rebaseliner = self.make_rebaseliner() image = rebaseliner._port.expected_image( os.path.join(rebaseliner._port.layout_tests_dir(), diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py index 9cc7895..704180c 100755 --- a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py @@ -69,6 +69,7 @@ from layout_package import json_layout_results_generator from layout_package import printing from layout_package import test_expectations from layout_package import test_failures +from layout_package import test_results from layout_package import test_results_uploader from test_types import image_diff from test_types import text_diff @@ -457,7 +458,7 @@ class TestRunner: # subtracted out of self._test_files, above), but we stub out the # results here so the statistics can remain accurate. for test in skip_chunk: - result = dump_render_tree_thread.TestResult(test, + result = test_results.TestResult(test, failures=[], test_run_time=0, total_time_for_all_diffs=0, time_for_diffs=0) result.type = test_expectations.SKIP @@ -852,7 +853,7 @@ class TestRunner: """Update the summary and print results with any completed tests.""" while True: try: - result = self._result_queue.get_nowait() + result = test_results.TestResult.loads(self._result_queue.get_nowait()) except Queue.Empty: return @@ -950,7 +951,12 @@ class TestRunner: _log.info("Uploading JSON files for builder: %s", self._options.builder_name) - attrs = [("builder", self._options.builder_name)] + attrs = [("builder", self._options.builder_name), ("testtype", "layout-tests")] + # FIXME: master_name should be required if test_results_server is set. + # Throw an error if master_name isn't set. + if self._options.master_name: + attrs.append(("master", self._options.master_name)) + json_files = ["expectations.json"] if self._options.upload_full_results: json_files.append("results.json") @@ -960,6 +966,13 @@ class TestRunner: files = [(file, os.path.join(self._options.results_directory, file)) for file in json_files] + # FIXME: Remove this. This is temporary debug logging. + if self._options.builder_name.startswith("Webkit Linux"): + for filename in files: + _log.debug(filename[1]) + with codecs.open(filename[1], "r") as results_file: + _log.debug("%s:\n%s" % (filename[0], results_file.read())) + uploader = test_results_uploader.TestResultsUploader( self._options.test_results_server) try: @@ -1011,16 +1024,13 @@ class TestRunner: tests = self._expectations.get_tests_with_result_type(result_type) now = result_summary.tests_by_timeline[test_expectations.NOW] wontfix = result_summary.tests_by_timeline[test_expectations.WONTFIX] - defer = result_summary.tests_by_timeline[test_expectations.DEFER] # We use a fancy format string in order to print the data out in a # nicely-aligned table. - fmtstr = ("Expect: %%5d %%-8s (%%%dd now, %%%dd defer, %%%dd wontfix)" - % (self._num_digits(now), self._num_digits(defer), - self._num_digits(wontfix))) + fmtstr = ("Expect: %%5d %%-8s (%%%dd now, %%%dd wontfix)" + % (self._num_digits(now), self._num_digits(wontfix))) self._printer.print_expected(fmtstr % - (len(tests), result_type_str, len(tests & now), - len(tests & defer), len(tests & wontfix))) + (len(tests), result_type_str, len(tests & now), len(tests & wontfix))) def _num_digits(self, num): """Returns the number of digits needed to represent the length of a @@ -1241,12 +1251,7 @@ class TestRunner: (passed, total, pct_passed)) self._printer.print_actual("") self._print_result_summary_entry(result_summary, - test_expectations.NOW, "Tests to be fixed for the current release") - - self._printer.print_actual("") - self._print_result_summary_entry(result_summary, - test_expectations.DEFER, - "Tests we'll fix in the future if they fail (DEFER)") + test_expectations.NOW, "Tests to be fixed") self._printer.print_actual("") self._print_result_summary_entry(result_summary, @@ -1301,7 +1306,8 @@ class TestRunner: page += u"<p><a href='%s'>%s</a><br />\n" % (test_url, test_name) test_failures = failures.get(test_file, []) for failure in test_failures: - page += u" %s<br/>" % failure.result_html_output(test_name) + page += (u" %s<br/>" % + failure.result_html_output(test_name)) page += "</p>\n" page += "</body></html>\n" return page @@ -1436,7 +1442,8 @@ def _set_up_derived_options(port_obj, options): if not options.child_processes: # FIXME: Investigate perf/flakiness impact of using cpu_count + 1. - options.child_processes = str(port_obj.default_child_processes()) + options.child_processes = os.environ.get("WEBKIT_TEST_CHILD_PROCESSES", + str(port_obj.default_child_processes())) if not options.configuration: options.configuration = port_obj.default_configuration() @@ -1515,6 +1522,10 @@ def parse_args(args=None): default=False, help="create a dialog on DumpRenderTree startup"), optparse.make_option("--gp-fault-error-box", action="store_true", default=False, help="enable Windows GP fault error box"), + optparse.make_option("--multiple-loads", + type="int", help="turn on multiple loads of each test"), + optparse.make_option("--js-flags", + type="string", help="JavaScript flags to pass to tests"), optparse.make_option("--nocheck-sys-deps", action="store_true", default=False, help="Don't check the system dependencies (themes)"), @@ -1561,8 +1572,9 @@ def parse_args(args=None): dest="pixel_tests", help="Enable pixel-to-pixel PNG comparisons"), optparse.make_option("--no-pixel-tests", action="store_false", dest="pixel_tests", help="Disable pixel-to-pixel PNG comparisons"), - # old-run-webkit-tests allows a specific tolerance: --tolerance t - # Ignore image differences less than this percentage (default: 0.1) + optparse.make_option("--tolerance", + help="Ignore image differences less than this percentage (some " + "ports may ignore this option)", type="float"), optparse.make_option("--results-directory", default="layout-test-results", help="Output results directory source dir, relative to Debug or " @@ -1686,6 +1698,7 @@ def parse_args(args=None): # FIXME: Move these into json_results_generator.py results_json_options = [ + optparse.make_option("--master-name", help="The name of the buildbot master."), optparse.make_option("--builder-name", default="DUMMY_BUILDER_NAME", help=("The name of the builder shown on the waterfall running " "this script e.g. WebKit.")), diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py index a716cec..0f09ffa 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py @@ -48,6 +48,7 @@ from webkitpy.common.system import user from webkitpy.layout_tests import port from webkitpy.layout_tests import run_webkit_tests from webkitpy.layout_tests.layout_package import dump_render_tree_thread +from webkitpy.layout_tests.port.test import TestPort from webkitpy.thirdparty.mock import Mock @@ -260,6 +261,31 @@ class MainTest(unittest.TestCase): tests_included=True) self.assertEqual(user.url, '/tmp/foo/results.html') + def test_tolerance(self): + class ImageDiffTestPort(TestPort): + def diff_image(self, expected_contents, actual_contents, + diff_filename=None): + self.tolerance_used_for_diff_image = self._options.tolerance + return True + + def get_port_for_run(args): + options, parsed_args = run_webkit_tests.parse_args(args) + test_port = ImageDiffTestPort(options=options, user=MockUser()) + passing_run(args=args, port_obj=test_port, tests_included=True) + return test_port + + base_args = ['--pixel-tests', 'failures/expected/*'] + + # If we pass in an explicit tolerance argument, then that will be used. + test_port = get_port_for_run(base_args + ['--tolerance', '.1']) + self.assertEqual(0.1, test_port.tolerance_used_for_diff_image) + test_port = get_port_for_run(base_args + ['--tolerance', '0']) + self.assertEqual(0, test_port.tolerance_used_for_diff_image) + + # Otherwise the port's default tolerance behavior (including ignoring it) + # should be used. + test_port = get_port_for_run(base_args) + self.assertEqual(None, test_port.tolerance_used_for_diff_image) def _mocked_open(original_open, file_list): diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py b/WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py index b1f621e..4c32f0d 100644 --- a/WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py @@ -89,6 +89,6 @@ class TestTextDiff(test_type_base.TestTypeBase): if expected == '': failures.append(test_failures.FailureMissingResult()) else: - failures.append(test_failures.FailureTextMismatch(True)) + failures.append(test_failures.FailureTextMismatch()) return failures diff --git a/WebKitTools/Scripts/webkitpy/style/checker.py b/WebKitTools/Scripts/webkitpy/style/checker.py index e0c956f..11e3e33 100644 --- a/WebKitTools/Scripts/webkitpy/style/checker.py +++ b/WebKitTools/Scripts/webkitpy/style/checker.py @@ -116,9 +116,9 @@ _PATH_RULES_SPECIFIER = [ # API and therefore do not follow the same header including # discipline as WebCore. (["WebKitTools/WebKitAPITest/", - "WebKit/qt/QGVLauncher/"], + "WebKitTools/TestWebKitAPI/"], ["-build/include", - "-readability/streams"]), + "-readability/naming"]), ([# The EFL APIs use EFL naming style, which includes # both lower-cased and camel-cased, underscore-sparated # values. @@ -148,6 +148,24 @@ _PATH_RULES_SPECIFIER = [ "/JavaScriptCore/assembler/"], ["-readability/naming"]), + # WebKit2 rules: + # WebKit2 doesn't use config.h, and certain directories have other + # idiosyncracies. + ([# NPAPI has function names with underscores. + "WebKit2/WebProcess/Plugins/Netscape"], + ["-build/include_order", + "-readability/naming"]), + ([# The WebKit2 C API has names with underscores and whitespace-aligned + # struct members. + "WebKit2/UIProcess/API/C/", + "WebKit2/WebProcess/InjectedBundle/API/c/"], + ["-build/include_order", + "-readability/naming", + "-whitespace/declaration"]), + ([# Nothing in WebKit2 uses config.h. + "WebKit2/"], + ["-build/include_order"]), + # For third-party Python code, keep only the following checks-- # # No tabs: to avoid having to set the SVN allow-tabs property. diff --git a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py index 5254275..43d24fe 100755 --- a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py @@ -213,11 +213,6 @@ class GlobalVariablesTest(unittest.TestCase): "build/include") assertNoCheck("WebKitTools/WebKitAPITest/main.cpp", "build/include") - assertNoCheck("WebKit/qt/QGVLauncher/main.cpp", - "build/include") - assertNoCheck("WebKit/qt/QGVLauncher/main.cpp", - "readability/streams") - assertCheck("random_path.cpp", "readability/naming") assertNoCheck("WebKit/gtk/webkit/webkit.h", diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py b/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py index 7c1cb3e..cd9e6ae 100644 --- a/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py @@ -1293,7 +1293,7 @@ def check_spacing(file_extension, clean_lines, line_number, error): line = clean_lines.elided[line_number] # get rid of comments and strings # Don't try to do spacing checks for operator methods - line = sub(r'operator(==|!=|<|<<|<=|>=|>>|>)\(', 'operator\(', line) + line = sub(r'operator(==|!=|<|<<|<=|>=|>>|>|\+=|-=|\*=|/=|%=|&=|\|=|^=|<<=|>>=)\(', 'operator\(', line) # Don't try to do spacing checks for #include or #import statements at # minimum because it messes up checks for spacing around / if match(r'\s*#\s*(?:include|import)', line): diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py b/WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py index 071ce50..6d5c24b 100644 --- a/WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py @@ -1313,6 +1313,19 @@ class CppStyleTest(CppStyleTestBase): self.assert_multi_line_lint('#include "config.h"\n#import <foo/bar.h>\n', '') + def test_operator_methods(self): + self.assert_lint('String operator+(const String&, const String&);', '') + self.assert_lint('bool operator==(const String&, const String&);', '') + self.assert_lint('String& operator-=(const String&, const String&);', '') + self.assert_lint('String& operator+=(const String&, const String&);', '') + self.assert_lint('String& operator*=(const String&, const String&);', '') + self.assert_lint('String& operator%=(const String&, const String&);', '') + self.assert_lint('String& operator&=(const String&, const String&);', '') + self.assert_lint('String& operator<<=(const String&, const String&);', '') + self.assert_lint('String& operator>>=(const String&, const String&);', '') + self.assert_lint('String& operator|=(const String&, const String&);', '') + self.assert_lint('String& operator^=(const String&, const String&);', '') + def test_spacing_before_last_semicolon(self): self.assert_lint('call_function() ;', 'Extra space before last semicolon. If this should be an ' diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py b/WebKitTools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py index 747b8b4..31f0b40 100644 --- a/WebKitTools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py @@ -111,7 +111,7 @@ class TestExpectationsTestCase(unittest.TestCase): ["BUG1234 DEBUG MAC : passes/text.html = TIMEOUT PASS"], "") self.assert_lines_lint( - ["SLOW DEFER BUG1234 : passes/text.html = PASS"], + ["SLOW BUG1234 : passes/text.html = PASS"], "") self.assert_lines_lint( ["WONTFIX SKIP : passes/text.html = TIMEOUT"], @@ -126,10 +126,6 @@ class TestExpectationsTestCase(unittest.TestCase): ["SKIP : passes/text.html = PASS"], "Test lacks BUG modifier. " "passes/text.html [test/expectations] [2]") - self.assert_lines_lint( - ["WONTFIX DEFER : passes/text.html = PASS"], - "Test cannot be both DEFER and WONTFIX. " - "passes/text.html [test/expectations] [5]") def test_expectation_errors(self): self.assert_lines_lint( diff --git a/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py b/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py index 7eac1cb..c2249c2 100644 --- a/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py +++ b/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py @@ -77,12 +77,6 @@ installer.install(url="http://pypi.python.org/packages/source/p/pep8/pep8-0.5.0. installer.install(url="http://www.adambarth.com/webkit/eliza", target_name="eliza.py") -rietveld_dir = os.path.join(autoinstalled_dir, "rietveld") -installer = AutoInstaller(target_dir=rietveld_dir) -installer.install(url="http://webkit-rietveld.googlecode.com/svn/trunk/upload_v26/upload.py", - target_name="upload.py") - - # Since irclib and ircbot are two top-level packages, we need to import # them separately. We group them into an irc package for better # organization purposes. diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py index 02e203c..ea12702 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py @@ -85,7 +85,6 @@ class CommitQueueTask(object): "apply-attachment", "--force-clean", "--non-interactive", - "--quiet", self._patch.id(), ], "Applied patch", @@ -97,7 +96,6 @@ class CommitQueueTask(object): "--no-clean", "--no-update", "--build-style=both", - "--quiet", ], "Built patch", "Patch does not build") @@ -108,7 +106,6 @@ class CommitQueueTask(object): "--force-clean", "--no-update", "--build-style=both", - "--quiet", ], "Able to build without patch", "Unable to build without patch") @@ -120,7 +117,6 @@ class CommitQueueTask(object): "--no-update", # Notice that we don't pass --build, which means we won't build! "--test", - "--quiet", "--non-interactive", ], "Passed tests", @@ -133,7 +129,6 @@ class CommitQueueTask(object): "--no-update", "--build", "--test", - "--quiet", "--non-interactive", ], "Able to pass tests without patch", @@ -146,11 +141,11 @@ class CommitQueueTask(object): return results.failing_tests() def _land(self): + # Unclear if this should pass --quiet or not. If --parent-command always does the reporting, then it should. return self._run_command([ "land-attachment", "--force-clean", "--ignore-builders", - "--quiet", "--non-interactive", "--parent-command=commit-queue", self._patch.id(), diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py index 6fa0667..15a4a6b 100644 --- a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py @@ -75,13 +75,13 @@ class CommitQueueTaskTest(unittest.TestCase): def test_success_case(self): commit_queue = MockCommitQueue([]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both'] command_passed: success_message='Built patch' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_passed: success_message='Passed tests' patch='197' -run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] +run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197] command_passed: success_message='Landed patch' patch='197' """ self._run_through_task(commit_queue, expected_stderr) @@ -90,7 +90,7 @@ command_passed: success_message='Landed patch' patch='197' commit_queue = MockCommitQueue([ ScriptError("MOCK apply failure"), ]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_failed: failure_message='Patch does not apply' script_error='MOCK apply failure' patch='197' """ self._run_through_task(commit_queue, expected_stderr, ScriptError) @@ -100,11 +100,11 @@ command_failed: failure_message='Patch does not apply' script_error='MOCK apply None, ScriptError("MOCK build failure"), ]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both'] command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='197' -run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both'] command_passed: success_message='Able to build without patch' patch='197' """ self._run_through_task(commit_queue, expected_stderr, ScriptError) @@ -115,11 +115,11 @@ command_passed: success_message='Able to build without patch' patch='197' ScriptError("MOCK build failure"), ScriptError("MOCK clean build failure"), ]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both'] command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='197' -run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both'] command_failed: failure_message='Unable to build without patch' script_error='MOCK clean build failure' patch='197' """ self._run_through_task(commit_queue, expected_stderr) @@ -130,16 +130,16 @@ command_failed: failure_message='Unable to build without patch' script_error='MO None, ScriptError("MOCK tests failure"), ]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both'] command_passed: success_message='Built patch' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK tests failure' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_passed: success_message='Passed tests' patch='197' report_flaky_tests: patch='197' flaky_tests='None' -run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] +run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197] command_passed: success_message='Landed patch' patch='197' """ self._run_through_task(commit_queue, expected_stderr) @@ -151,15 +151,15 @@ command_passed: success_message='Landed patch' patch='197' ScriptError("MOCK test failure"), ScriptError("MOCK test failure again"), ]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both'] command_passed: success_message='Built patch' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure again' patch='197' -run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--non-interactive'] command_passed: success_message='Able to pass tests without patch' patch='197' """ self._run_through_task(commit_queue, expected_stderr, ScriptError) @@ -172,15 +172,15 @@ command_passed: success_message='Able to pass tests without patch' patch='197' ScriptError("MOCK test failure again"), ScriptError("MOCK clean test failure"), ]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both'] command_passed: success_message='Built patch' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure again' patch='197' -run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--non-interactive'] command_failed: failure_message='Unable to pass tests without patch (tree is red?)' script_error='MOCK clean test failure' patch='197' """ self._run_through_task(commit_queue, expected_stderr) @@ -192,13 +192,13 @@ command_failed: failure_message='Unable to pass tests without patch (tree is red None, ScriptError("MOCK land failure"), ]) - expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197] command_passed: success_message='Applied patch' patch='197' -run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both'] command_passed: success_message='Built patch' patch='197' -run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] command_passed: success_message='Passed tests' patch='197' -run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] +run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197] command_failed: failure_message='Unable to land patch' script_error='MOCK land failure' patch='197' """ self._run_through_task(commit_queue, expected_stderr, ScriptError) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py index ed5c604..541c9c4 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py @@ -106,8 +106,10 @@ land will NOT build and run the tests before committing, but you can use the --b If a bug id is provided, or one can be found in the ChangeLog land will update the bug after committing.""" def _prepare_state(self, options, args, tool): + changed_files = self._tool.scm().changed_files(options.git_commit) return { - "bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit), + "changed_files": changed_files, + "bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit, changed_files), } @@ -221,18 +223,6 @@ class BuildAndTestAttachment(AbstractPatchSequencingCommand, ProcessAttachmentsM ] -class PostAttachmentToRietveld(AbstractPatchSequencingCommand, ProcessAttachmentsMixin): - name = "post-attachment-to-rietveld" - help_text = "Uploads a bugzilla attachment to rietveld" - arguments_names = "ATTACHMENTID" - main_steps = [ - steps.CleanWorkingDirectory, - steps.Update, - steps.ApplyPatch, - steps.PostCodeReview, - ] - - class AbstractPatchApplyingCommand(AbstractPatchSequencingCommand): prepare_steps = [ steps.EnsureLocalCommitIfNeeded, diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py index 6af1f64..bfca139 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py @@ -118,10 +118,6 @@ class DownloadCommandsTest(CommandsTest): expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nBuilding WebKit\n" self.assert_execute_outputs(BuildAttachment(), [197], options=self._default_options(), expected_stderr=expected_stderr) - def test_post_attachment_to_rietveld(self): - expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nMOCK: Uploading patch to rietveld\nMOCK setting flag 'in-rietveld' to '+' on attachment '197' with comment 'None' and additional comment 'None'\n" - self.assert_execute_outputs(PostAttachmentToRietveld(), [197], options=self._default_options(), expected_stderr=expected_stderr) - def test_land_attachment(self): # FIXME: This expected result is imperfect, notice how it's seeing the same patch as still there after it thought it would have cleared the flags. expected_stderr = """Processing 1 patch from 1 bug. @@ -186,5 +182,6 @@ where ATTACHMENT_ID is the ID of this attachment. def test_rollout(self): expected_stderr = "Preparing rollout for bug 42.\nUpdating working directory\nRunning prepare-ChangeLog\nMOCK: user.open_url: file://...\nBuilding WebKit\n" - self.assert_execute_outputs(Rollout(), [852, "Reason"], options=self._default_options(), expected_stderr=expected_stderr) + expected_stdout = "Was that diff correct?\n" + self.assert_execute_outputs(Rollout(), [852, "Reason"], options=self._default_options(), expected_stdout=expected_stdout, expected_stderr=expected_stderr) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py index 5ec468e..3b53d1a 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py @@ -50,8 +50,7 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue): self.port.flag(), "--build-style=%s" % self._build_style, "--force-clean", - "--no-update", - "--quiet"]) + "--no-update"]) return True except ScriptError, e: failure_log = self._log_from_script_error_for_upload(e) @@ -81,6 +80,14 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue): raise def review_patch(self, patch): + if patch.is_obsolete(): + self._did_error(patch, "%s does not process obsolete patches." % self.name) + return False + + if patch.bug().is_closed(): + self._did_error(patch, "%s does not process patches on closed bugs." % self.name) + return False + if not self._build(patch, first_run=True): if not self._can_build(): return False diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py index c400f81..830e11c 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py @@ -29,15 +29,54 @@ import os from webkitpy.thirdparty.mock import Mock +from webkitpy.common.system.outputcapture import OutputCapture +from webkitpy.tool.bot.queueengine import QueueEngine from webkitpy.tool.commands.earlywarningsystem import * from webkitpy.tool.commands.queuestest import QueuesTest +from webkitpy.tool.mocktool import MockTool, MockOptions + + +class AbstractEarlyWarningSystemTest(QueuesTest): + def test_can_build(self): + # Needed to define port_name, used in AbstractEarlyWarningSystem.__init__ + class TestEWS(AbstractEarlyWarningSystem): + port_name = "win" # Needs to be a port which port/factory understands. + + queue = TestEWS() + queue.bind_to_tool(MockTool(log_executive=True)) + queue._options = MockOptions(port=None) + expected_stderr = "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--port=win', '--build-style=release', '--force-clean', '--no-update']\n" + OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr) + + def mock_run_webkit_patch(args): + raise ScriptError("MOCK script error") + + queue.run_webkit_patch = mock_run_webkit_patch + expected_stderr = "MOCK: update_status: None Unable to perform a build\n" + OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr) + + # FIXME: This belongs on an AbstractReviewQueueTest object in queues_unittest.py + def test_subprocess_handled_error(self): + queue = AbstractReviewQueue() + queue.bind_to_tool(MockTool()) + + def mock_review_patch(patch): + raise ScriptError('MOCK script error', exit_code=QueueEngine.handled_error_code) + + queue.review_patch = mock_review_patch + mock_patch = queue._tool.bugs.fetch_attachment(197) + expected_stderr = "MOCK: release_work_item: None 197\n" + OutputCapture().assert_outputs(self, queue.process_work_item, [mock_patch], expected_stderr=expected_stderr, expected_exception=ScriptError) + class EarlyWarningSytemTest(QueuesTest): def test_failed_builds(self): ews = ChromiumLinuxEWS() + ews.bind_to_tool(MockTool()) ews._build = lambda patch, first_run=False: False ews._can_build = lambda: True - ews.review_patch(Mock()) + mock_patch = ews._tool.bugs.fetch_attachment(197) + ews.review_patch(mock_patch) def _default_expected_stderr(self, ews): string_replacemnts = { @@ -46,20 +85,29 @@ class EarlyWarningSytemTest(QueuesTest): "watchers": ews.watchers, } expected_stderr = { - "begin_work_queue": self._default_begin_work_queue_stderr(ews.name, os.getcwd()), # FIXME: Use of os.getcwd() is wrong, should be scm.checkout_root + "begin_work_queue": self._default_begin_work_queue_stderr(ews.name, ews._tool.scm().checkout_root), "handle_unexpected_error": "Mock error message\n", "next_work_item": "", "process_work_item": "MOCK: update_status: %(name)s Pass\nMOCK: release_work_item: %(name)s 197\n" % string_replacemnts, - "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=142, cc=%(watchers)s\n--- Begin comment ---\nAttachment 197 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts, + "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=42, cc=%(watchers)s\n--- Begin comment ---\nAttachment 197 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts, } return expected_stderr def _test_ews(self, ews): + ews.bind_to_tool(MockTool()) expected_exceptions = { "handle_script_error": SystemExit, } self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews), expected_exceptions=expected_exceptions) + def _test_committer_only_ews(self, ews): + ews.bind_to_tool(MockTool()) + expected_stderr = self._default_expected_stderr(ews) + string_replacemnts = {"name": ews.name} + expected_stderr["process_work_item"] = "MOCK: update_status: %(name)s Error: %(name)s cannot process patches from non-committers :(\nMOCK: release_work_item: %(name)s 197\n" % string_replacemnts + expected_exceptions = {"handle_script_error": SystemExit} + self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions) + # FIXME: If all EWSes are going to output the same text, we # could test them all in one method with a for loop over an array. def test_chromium_linux_ews(self): @@ -78,19 +126,7 @@ class EarlyWarningSytemTest(QueuesTest): self._test_ews(EflEWS()) def test_mac_ews(self): - ews = MacEWS() - expected_stderr = self._default_expected_stderr(ews) - expected_stderr["process_work_item"] = "MOCK: update_status: mac-ews Error: mac-ews cannot process patches from non-committers :(\n" - expected_exceptions = { - "handle_script_error": SystemExit, - } - self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions) + self._test_committer_only_ews(MacEWS()) def test_chromium_mac_ews(self): - ews = ChromiumMacEWS() - expected_stderr = self._default_expected_stderr(ews) - expected_stderr["process_work_item"] = "MOCK: update_status: cr-mac-ews Error: cr-mac-ews cannot process patches from non-committers :(\n" - expected_exceptions = { - "handle_script_error": SystemExit, - } - self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions) + self._test_committer_only_ews(ChromiumMacEWS()) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py index 7b3002a..6b4213b 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py @@ -38,7 +38,7 @@ from datetime import datetime from optparse import make_option from StringIO import StringIO -from webkitpy.common.net.bugzilla import CommitterValidator +from webkitpy.common.net.bugzilla import CommitterValidator, Attachment from webkitpy.common.net.layouttestresults import path_for_layout_test, LayoutTestResults from webkitpy.common.net.statusserver import StatusServer from webkitpy.common.system.executive import ScriptError @@ -47,7 +47,7 @@ from webkitpy.tool.commands.stepsequence import StepSequenceErrorHandler from webkitpy.tool.bot.commitqueuetask import CommitQueueTask, CommitQueueTaskDelegate from webkitpy.tool.bot.feeders import CommitQueueFeeder, EWSFeeder from webkitpy.tool.bot.queueengine import QueueEngine, QueueEngineDelegate -from webkitpy.tool.grammar import pluralize +from webkitpy.tool.grammar import pluralize, join_with_separators from webkitpy.tool.multicommandtool import Command, TryAgain @@ -87,6 +87,11 @@ class AbstractQueue(Command, QueueEngineDelegate): if self._options.port: webkit_patch_args += ["--port=%s" % self._options.port] webkit_patch_args.extend(args) + # FIXME: There is probably no reason to use run_and_throw_if_fail anymore. + # run_and_throw_if_fail was invented to support tee'd output + # (where we write both to a log file and to the console at once), + # but the queues don't need live-progress, a dump-of-output at the + # end should be sufficient. return self._tool.executive.run_and_throw_if_fail(webkit_patch_args) def _log_directory(self): @@ -196,24 +201,40 @@ class AbstractPatchQueue(AbstractQueue): def _update_status(self, message, patch=None, results_file=None): return self._tool.status_server.update_status(self.name, message, patch, results_file) - def _fetch_next_work_item(self): - return self._tool.status_server.next_work_item(self.name) + def _next_patch(self): + patch_id = self._tool.status_server.next_work_item(self.name) + if not patch_id: + return None + patch = self._tool.bugs.fetch_attachment(patch_id) + if not patch: + # FIXME: Using a fake patch because release_work_item has the wrong API. + # We also don't really need to release the lock (although that's fine), + # mostly we just need to remove this bogus patch from our queue. + # If for some reason bugzilla is just down, then it will be re-fed later. + patch = Attachment({'id': patch_id}, None) + self._release_work_item(patch) + return None + return patch def _release_work_item(self, patch): self._tool.status_server.release_work_item(self.name, patch) def _did_pass(self, patch): self._update_status(self._pass_status, patch) + self._release_work_item(patch) def _did_fail(self, patch): self._update_status(self._fail_status, patch) + self._release_work_item(patch) def _did_retry(self, patch): self._update_status(self._retry_status, patch) + self._release_work_item(patch) def _did_error(self, patch, reason): message = "%s: %s" % (self._error_status, reason) self._update_status(message, patch) + self._release_work_item(patch) def work_item_log_path(self, patch): return os.path.join(self._log_directory(), "%s.log" % patch.bug_id()) @@ -229,10 +250,7 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD self.committer_validator = CommitterValidator(self._tool.bugs) def next_work_item(self): - patch_id = self._fetch_next_work_item() - if not patch_id: - return None - return self._tool.bugs.fetch_attachment(patch_id) + return self._next_patch() def should_proceed_with_work_item(self, patch): patch_text = "rollout patch" if patch.is_rollout() else "patch" @@ -251,7 +269,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD validator = CommitterValidator(self._tool.bugs) validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task.failure_status_id, e)) self._did_fail(patch) - self._release_work_item(patch) def _error_message_for_bug(self, status_id, script_error): if not script_error.output: @@ -297,13 +314,17 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD def _author_emails_for_tests(self, flaky_tests): test_paths = map(path_for_layout_test, flaky_tests) commit_infos = self._tool.checkout().recent_commit_infos_for_files(test_paths) - return [commit_info.author().bugzilla_email() for commit_info in commit_infos if commit_info.author()] + return set([commit_info.author().bugzilla_email() for commit_info in commit_infos if commit_info.author()]) def report_flaky_tests(self, patch, flaky_tests): - authors = self._author_emails_for_tests(flaky_tests) - cc_explaination = " The author(s) of the test(s) have been CCed on this bug." if authors else "" - message = "The %s encountered the following flaky tests while processing attachment %s:\n\n%s\n\nPlease file bugs against the tests.%s The commit-queue is continuing to process your patch." % (self.name, patch.id(), "\n".join(flaky_tests), cc_explaination) - self._tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=authors) + message = "The %s encountered the following flaky tests while processing attachment %s:" % (self.name, patch.id()) + message += "\n\n%s\n\n" % ("\n".join(flaky_tests)) + message += "Please file bugs against the tests. " + author_emails = self._author_emails_for_tests(flaky_tests) + if author_emails: + message += "These tests were authored by %s. " % (join_with_separators(sorted(author_emails))) + message += "The commit-queue is continuing to process your patch." + self._tool.bugs.post_comment_to_bug(patch.bug_id(), message) # StepSequenceErrorHandler methods @@ -327,52 +348,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD raise TryAgain() -# FIXME: All the Rietveld code is no longer used and should be deleted. -class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler): - name = "rietveld-upload-queue" - - def __init__(self): - AbstractPatchQueue.__init__(self) - - # AbstractPatchQueue methods - - def next_work_item(self): - patch_id = self._tool.bugs.queries.fetch_first_patch_from_rietveld_queue() - if patch_id: - return patch_id - self._update_status("Empty queue") - - def should_proceed_with_work_item(self, patch): - self._update_status("Uploading patch", patch) - return True - - def process_work_item(self, patch): - try: - self.run_webkit_patch(["post-attachment-to-rietveld", "--force-clean", "--non-interactive", "--parent-command=rietveld-upload-queue", patch.id()]) - self._did_pass(patch) - return True - except ScriptError, e: - if e.exit_code != QueueEngine.handled_error_code: - self._did_fail(patch) - raise e - - @classmethod - def _reject_patch(cls, tool, patch_id): - tool.bugs.set_flag_on_attachment(patch_id, "in-rietveld", "-") - - def handle_unexpected_error(self, patch, message): - log(message) - self._reject_patch(self._tool, patch.id()) - - # StepSequenceErrorHandler methods - - @classmethod - def handle_script_error(cls, tool, state, script_error): - log(script_error.message_with_output()) - cls._update_status_for_script_error(tool, state, script_error) - cls._reject_patch(tool, state["patch"].id()) - - class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler): """This is the base-class for the EWS queues and the style-queue.""" def __init__(self, options=None): @@ -387,10 +362,7 @@ class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler): AbstractPatchQueue.begin_work_queue(self) def next_work_item(self): - patch_id = self._fetch_next_work_item() - if not patch_id: - return None - return self._tool.bugs.fetch_attachment(patch_id) + return self._next_patch() def should_proceed_with_work_item(self, patch): raise NotImplementedError("subclasses must implement") @@ -404,9 +376,11 @@ class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler): except ScriptError, e: if e.exit_code != QueueEngine.handled_error_code: self._did_fail(patch) + else: + # The subprocess handled the error, but won't have released the patch, so we do. + # FIXME: We need to simplify the rules by which _release_work_item is called. + self._release_work_item(patch) raise e - finally: - self._release_work_item(patch) def handle_unexpected_error(self, patch, message): log(message) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py index b37b5dc..b45db7b 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py @@ -29,12 +29,13 @@ import os from webkitpy.common.checkout.scm import CheckoutNeedsUpdate +from webkitpy.common.config.committers import Committer from webkitpy.common.net.bugzilla import Attachment from webkitpy.common.system.outputcapture import OutputCapture from webkitpy.thirdparty.mock import Mock from webkitpy.tool.commands.commandtest import CommandsTest from webkitpy.tool.commands.queues import * -from webkitpy.tool.commands.queuestest import QueuesTest, MockPatch +from webkitpy.tool.commands.queuestest import QueuesTest from webkitpy.tool.commands.stepsequence import StepSequence from webkitpy.tool.mocktool import MockTool, MockSCM, MockStatusServer @@ -51,11 +52,6 @@ class TestFeederQueue(FeederQueue): _sleep_duration = 0 -class MockRolloutPatch(MockPatch): - def is_rollout(self): - return True - - class AbstractQueueTest(CommandsTest): def test_log_directory(self): self.assertEquals(TestQueue()._log_directory(), "test-queue-logs") @@ -144,15 +140,19 @@ MOCK: submit_to_ews: 103 class AbstractPatchQueueTest(CommandsTest): - def test_fetch_next_work_item(self): + def test_next_patch(self): queue = AbstractPatchQueue() tool = MockTool() queue.bind_to_tool(tool) queue._options = Mock() queue._options.port = None - self.assertEquals(queue._fetch_next_work_item(), None) - tool.status_server = MockStatusServer(work_items=[2, 1, 3]) - self.assertEquals(queue._fetch_next_work_item(), 2) + self.assertEquals(queue._next_patch(), None) + tool.status_server = MockStatusServer(work_items=[2, 197]) + expected_stdout = "MOCK: fetch_attachment: 2 is not a known attachment id\n" # A mock-only message to prevent us from making mistakes. + expected_stderr = "MOCK: release_work_item: None 2\n" + patch_id = OutputCapture().assert_outputs(self, queue._next_patch, [], expected_stdout=expected_stdout, expected_stderr=expected_stderr) + self.assertEquals(patch_id, None) # 2 is an invalid patch id + self.assertEquals(queue._next_patch().id(), 197) class NeedsUpdateSequence(StepSequence): @@ -198,6 +198,19 @@ class SecondThoughtsCommitQueue(CommitQueue): return Attachment(attachment_dictionary, None) +# Creating fake CommitInfos is a pain, so we use a mock one here. +class MockCommitInfo(object): + def __init__(self, author_email): + self._author_email = author_email + + def author(self): + # It's definitely possible to have commits with authors who + # are not in our committers.py list. + if not self._author_email: + return None + return Committer("Mock Committer", self._author_email) + + class CommitQueueTest(QueuesTest): def test_commit_queue(self): expected_stderr = { @@ -209,6 +222,7 @@ MOCK: update_status: commit-queue Built patch MOCK: update_status: commit-queue Passed tests MOCK: update_status: commit-queue Landed patch MOCK: update_status: commit-queue Pass +MOCK: release_work_item: commit-queue 197 """, "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'Mock error message'\n", "handle_script_error": "ScriptError error message\n", @@ -243,15 +257,16 @@ MOCK: release_work_item: commit-queue 197 "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root), "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n", "next_work_item": "", - "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', 197] MOCK: update_status: commit-queue Applied patch -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both'] MOCK: update_status: commit-queue Built patch -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'] MOCK: update_status: commit-queue Passed tests -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197] MOCK: update_status: commit-queue Landed patch MOCK: update_status: commit-queue Pass +MOCK: release_work_item: commit-queue 197 """, "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'Mock error message'\n", "handle_script_error": "ScriptError error message\n", @@ -261,22 +276,22 @@ MOCK: update_status: commit-queue Pass def test_rollout_lands(self): tool = MockTool(log_executive=True) tool.buildbot.light_tree_on_fire() - rollout_patch = MockRolloutPatch() + rollout_patch = tool.bugs.fetch_attachment(106) # _patch6, a rollout patch. + assert(rollout_patch.is_rollout()) expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root), "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing rollout patch\n", "next_work_item": "", - "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197] + "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', 106] MOCK: update_status: commit-queue Applied patch -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both', '--quiet'] +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both'] MOCK: update_status: commit-queue Built patch -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive'] -MOCK: update_status: commit-queue Passed tests -MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197] +MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 106] MOCK: update_status: commit-queue Landed patch MOCK: update_status: commit-queue Pass +MOCK: release_work_item: commit-queue 106 """, - "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'Mock error message'\n", + "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '106' with comment 'Rejecting patch 106 from commit-queue.' and additional comment 'Mock error message'\n", "handle_script_error": "ScriptError error message\n", } self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr) @@ -307,23 +322,36 @@ MOCK: update_status: commit-queue Passed tests MOCK: update_status: commit-queue Retry MOCK: release_work_item: commit-queue 197 """ - OutputCapture().assert_outputs(self, queue.process_work_item, [MockPatch()], expected_stderr=expected_stderr) + OutputCapture().assert_outputs(self, queue.process_work_item, [QueuesTest.mock_work_item], expected_stderr=expected_stderr) + + def _assert_emails_for_tests(self, emails): + queue = CommitQueue() + tool = MockTool() + queue.bind_to_tool(tool) + commit_infos = [MockCommitInfo(email) for email in emails] + tool.checkout().recent_commit_infos_for_files = lambda paths: set(commit_infos) + self.assertEqual(queue._author_emails_for_tests([]), set(emails)) + + def test_author_emails_for_tests(self): + self._assert_emails_for_tests([]) + self._assert_emails_for_tests(["test1@test.com", "test1@test.com"]) + self._assert_emails_for_tests(["test1@test.com", "test2@test.com"]) def test_report_flaky_tests(self): queue = CommitQueue() queue.bind_to_tool(MockTool()) - expected_stderr = """MOCK bug comment: bug_id=142, cc=['abarth@webkit.org'] + expected_stderr = """MOCK bug comment: bug_id=42, cc=None --- Begin comment --- The commit-queue encountered the following flaky tests while processing attachment 197: foo/bar.html bar/baz.html -Please file bugs against the tests. The author(s) of the test(s) have been CCed on this bug. The commit-queue is continuing to process your patch. +Please file bugs against the tests. These tests were authored by abarth@webkit.org. The commit-queue is continuing to process your patch. --- End comment --- """ - OutputCapture().assert_outputs(self, queue.report_flaky_tests, [MockPatch(), ["foo/bar.html", "bar/baz.html"]], expected_stderr=expected_stderr) + OutputCapture().assert_outputs(self, queue.report_flaky_tests, [QueuesTest.mock_work_item, ["foo/bar.html", "bar/baz.html"]], expected_stderr=expected_stderr) def test_layout_test_results(self): queue = CommitQueue() @@ -333,17 +361,6 @@ Please file bugs against the tests. The author(s) of the test(s) have been CCed queue._read_file_contents = lambda path: "" self.assertEquals(queue.layout_test_results(), None) -class RietveldUploadQueueTest(QueuesTest): - def test_rietveld_upload_queue(self): - expected_stderr = { - "begin_work_queue": self._default_begin_work_queue_stderr("rietveld-upload-queue", MockSCM.fake_checkout_root), - "should_proceed_with_work_item": "MOCK: update_status: rietveld-upload-queue Uploading patch\n", - "process_work_item": "MOCK: update_status: rietveld-upload-queue Pass\n", - "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '197' with comment 'None' and additional comment 'None'\n", - "handle_script_error": "ScriptError error message\nMOCK: update_status: rietveld-upload-queue ScriptError error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '197' with comment 'None' and additional comment 'None'\n", - } - self.assert_queue_outputs(RietveldUploadQueue(), expected_stderr=expected_stderr) - class StyleQueueTest(QueuesTest): def test_style_queue(self): @@ -353,7 +370,7 @@ class StyleQueueTest(QueuesTest): "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n", "process_work_item": "MOCK: update_status: style-queue Pass\nMOCK: release_work_item: style-queue 197\n", "handle_unexpected_error": "Mock error message\n", - "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=142, cc=[]\n--- Begin comment ---\nAttachment 197 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n", + "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=42, cc=[]\n--- Begin comment ---\nAttachment 197 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n", } expected_exceptions = { "handle_script_error": SystemExit, diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py index 379d380..6455617 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py @@ -44,25 +44,9 @@ class MockQueueEngine(object): pass -class MockPatch(): - def id(self): - return 197 - - def bug_id(self): - return 142 - - def is_rollout(self): - return False - - class QueuesTest(unittest.TestCase): - # Ids match patch1 in mocktool.py - mock_work_item = Attachment({ - "id": 197, - "bug_id": 142, - "name": "Patch", - "attacher_email": "adam@example.com", - }, None) + # This is _patch1 in mocktool.py + mock_work_item = MockTool().bugs.fetch_attachment(197) def assert_outputs(self, func, func_name, args, expected_stdout, expected_stderr, expected_exceptions): exception = None @@ -108,4 +92,4 @@ class QueuesTest(unittest.TestCase): self.assert_outputs(queue.handle_unexpected_error, "handle_unexpected_error", [work_item, "Mock error message"], expected_stdout, expected_stderr, expected_exceptions) # Should we have a different function for testing StepSequenceErrorHandlers? if isinstance(queue, StepSequenceErrorHandler): - self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": MockPatch()}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions) + self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": self.mock_work_item}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions) diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py index 32eb016..4db463e 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py @@ -38,9 +38,10 @@ class SheriffBotTest(QueuesTest): builder2 = MockBuilder("Builder2") def test_sheriff_bot(self): - mock_work_item = MockFailureMap(MockTool().buildbot) + tool = MockTool() + mock_work_item = MockFailureMap(tool.buildbot) expected_stderr = { - "begin_work_queue": self._default_begin_work_queue_stderr("sheriff-bot", os.getcwd()), + "begin_work_queue": self._default_begin_work_queue_stderr("sheriff-bot", tool.scm().checkout_root), "next_work_item": "", "process_work_item": """MOCK: irc.post: abarth, darin, eseidel: http://trac.webkit.org/changeset/29837 might have broken Builder1 MOCK bug comment: bug_id=42, cc=['abarth@webkit.org', 'eric@webkit.org'] diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py index 107d8db..ed91f5a 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py @@ -152,7 +152,9 @@ 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) + changed_files = self._tool.scm().changed_files(options.git_commit) + state["changed_files"] = changed_files + bug_id = tool.checkout().bug_id_for_this_commit(options.git_commit, changed_files) return bug_id def _prepare_state(self, options, args, tool): @@ -171,6 +173,7 @@ class Post(AbstractPatchUploadingCommand): steps.CheckStyle, steps.ConfirmDiff, steps.ObsoletePatches, + steps.SuggestReviewers, steps.PostDiff, ] @@ -219,6 +222,7 @@ class Upload(AbstractPatchUploadingCommand): steps.EditChangeLog, steps.ConfirmDiff, steps.ObsoletePatches, + steps.SuggestReviewers, steps.PostDiff, ] long_help = """upload uploads the current diff to bugs.webkit.org. diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py index 0d096b6..bd1fbd6 100644 --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py @@ -58,6 +58,7 @@ class UploadCommandsTest(CommandsTest): options.description = "MOCK description" options.request_commit = False options.review = True + options.suggest_reviewers = False expected_stderr = """Running check-webkit-style MOCK: user.open_url: file://... Obsoleting 2 old patches on bug 42 @@ -67,7 +68,8 @@ None -- End comment -- MOCK: user.open_url: http://example.com/42 """ - self.assert_execute_outputs(Post(), [42], options=options, expected_stderr=expected_stderr) + expected_stdout = "Was that diff correct?\n" + self.assert_execute_outputs(Post(), [42], options=options, expected_stdout=expected_stdout, expected_stderr=expected_stderr) def test_land_safely(self): expected_stderr = "Obsoleting 2 old patches on bug 42\nMOCK add_patch_to_bug: bug_id=42, description=Patch for landing, mark_for_review=False, mark_for_commit_queue=False, mark_for_landing=True\n-- Begin comment --\nNone\n-- End comment --\n" @@ -88,6 +90,7 @@ MOCK: user.open_url: http://example.com/42 options.description = "MOCK description" options.request_commit = False options.review = True + options.suggest_reviewers = False expected_stderr = """Running check-webkit-style MOCK: user.open_url: file://... Obsoleting 2 old patches on bug 42 @@ -97,7 +100,8 @@ None -- End comment -- MOCK: user.open_url: http://example.com/42 """ - self.assert_execute_outputs(Upload(), [42], options=options, expected_stderr=expected_stderr) + expected_stdout = "Was that diff correct?\n" + self.assert_execute_outputs(Upload(), [42], options=options, expected_stdout=expected_stdout, expected_stderr=expected_stderr) def test_mark_bug_fixed(self): tool = MockTool() @@ -106,7 +110,8 @@ MOCK: user.open_url: http://example.com/42 options.bug_id = 42 options.comment = "MOCK comment" expected_stderr = "Bug: <http://example.com/42> Bug with two r+'d and cq+'d patches, one of which has an invalid commit-queue setter.\nRevision: 9876\nMOCK: user.open_url: http://example.com/42\nAdding comment to Bug 42.\nMOCK bug comment: bug_id=42, cc=None\n--- Begin comment ---\nMOCK comment\n\nCommitted r9876: <http://trac.webkit.org/changeset/9876>\n--- End comment ---\n\n" - self.assert_execute_outputs(MarkBugFixed(), [], expected_stderr=expected_stderr, tool=tool, options=options) + expected_stdout = "Is this correct?\n" + self.assert_execute_outputs(MarkBugFixed(), [], expected_stdout=expected_stdout, expected_stderr=expected_stderr, tool=tool, options=options) def test_edit_changelog(self): self.assert_execute_outputs(EditChangeLogs(), []) diff --git a/WebKitTools/Scripts/webkitpy/tool/main.py b/WebKitTools/Scripts/webkitpy/tool/main.py index ce6666e..e0862c5 100755 --- a/WebKitTools/Scripts/webkitpy/tool/main.py +++ b/WebKitTools/Scripts/webkitpy/tool/main.py @@ -37,7 +37,6 @@ from webkitpy.common.checkout.scm import default_scm from webkitpy.common.config.ports import WebKitPort from webkitpy.common.net.bugzilla import Bugzilla from webkitpy.common.net.buildbot import BuildBot -from webkitpy.common.net.rietveld import Rietveld from webkitpy.common.net.irc.ircproxy import IRCProxy from webkitpy.common.system.executive import Executive from webkitpy.common.system.user import User @@ -79,7 +78,6 @@ class WebKitPatch(MultiCommandTool): self._scm = None self._checkout = None self.status_server = StatusServer() - self.codereview = Rietveld(self.executive) self.port_factory = port.factory def scm(self): @@ -126,7 +124,6 @@ class WebKitPatch(MultiCommandTool): if options.dry_run: self.scm().dryrun = True self.bugs.dryrun = True - self.codereview.dryrun = True if options.status_host: self.status_server.set_host(options.status_host) if options.bot_id: diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py index 05b30dd..af232d9 100644 --- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py +++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py @@ -33,7 +33,6 @@ from webkitpy.common.config.committers import CommitterList, Reviewer from webkitpy.common.checkout.commitinfo import CommitInfo from webkitpy.common.checkout.scm import CommitMessage from webkitpy.common.net.bugzilla import Bug, Attachment -from webkitpy.common.net.rietveld import Rietveld from webkitpy.thirdparty.mock import Mock from webkitpy.common.system.deprecated_logging import log @@ -86,7 +85,6 @@ _patch3 = { "name": "Patch3", "is_obsolete": False, "is_patch": True, - "in-rietveld": "?", "review": "?", "attacher_email": "eric@webkit.org", } @@ -113,7 +111,6 @@ _patch5 = { "name": "Patch5", "is_obsolete": False, "is_patch": True, - "in-rietveld": "?", "review": "+", "reviewer_email": "foo@bar.com", "attacher_email": "eric@webkit.org", @@ -127,7 +124,6 @@ _patch6 = { # Valid committer, but no reviewer. "name": "ROLLOUT of r3489", "is_obsolete": False, "is_patch": True, - "in-rietveld": "-", "commit-queue": "+", "committer_email": "foo@bar.com", "attacher_email": "eric@webkit.org", @@ -141,7 +137,6 @@ _patch7 = { # Valid review, patch is marked obsolete. "name": "Patch7", "is_obsolete": True, "is_patch": True, - "in-rietveld": "+", "review": "+", "reviewer_email": "foo@bar.com", "attacher_email": "eric@webkit.org", @@ -192,6 +187,7 @@ _bug4 = { } +# FIXME: This should not inherit from Mock class MockBugzillaQueries(Mock): def __init__(self, bugzilla): @@ -229,18 +225,14 @@ class MockBugzillaQueries(Mock): def fetch_patches_from_pending_commit_list(self): return sum([bug.reviewed_patches() for bug in self._all_bugs()], []) - def fetch_first_patch_from_rietveld_queue(self): - for bug in self._all_bugs(): - patches = bug.in_rietveld_queue_patches() - if len(patches): - return patches[0] - raise Exception('No patches in the rietveld queue') + +_mock_reviewer = Reviewer("Foo Bar", "foo@bar.com") + # FIXME: Bugzilla is the wrong Mock-point. Once we have a BugzillaNetwork # class we should mock that instead. # Most of this class is just copy/paste from Bugzilla. - - +# FIXME: This should not inherit from Mock class MockBugzilla(Mock): bug_server_url = "http://example.com" @@ -258,7 +250,7 @@ class MockBugzilla(Mock): def __init__(self): Mock.__init__(self) self.queries = MockBugzillaQueries(self) - self.committers = CommitterList(reviewers=[Reviewer("Foo Bar", "foo@bar.com")]) + self.committers = CommitterList(reviewers=[_mock_reviewer]) self._override_patch = None def create_bug(self, @@ -288,8 +280,10 @@ class MockBugzilla(Mock): if self._override_patch: return self._override_patch - # This could be changed to .get() if we wish to allow failed lookups. - attachment_dictionary = self.attachment_cache[attachment_id] + attachment_dictionary = self.attachment_cache.get(attachment_id) + if not attachment_dictionary: + print "MOCK: fetch_attachment: %s is not a known attachment id" % attachment_id + return None bug = self.fetch_bug(attachment_dictionary["bug_id"]) for attachment in bug.attachments(include_obsolete=True): if attachment.id() == int(attachment_id): @@ -418,6 +412,7 @@ class MockBuildBot(object): return MockFailureMap(self) +# FIXME: This should not inherit from Mock class MockSCM(Mock): fake_checkout_root = os.path.realpath("/tmp") # realpath is needed to allow for Mac OS X's /private/tmp @@ -480,7 +475,7 @@ class MockCheckout(object): # that LandDiff will try to actually read the patch from disk! return [] - def commit_message_for_this_commit(self, git_commit): + def commit_message_for_this_commit(self, git_commit, changed_files=None): commit_message = Mock() commit_message.message = lambda:"This is a fake commit message that is at least 50 characters." return commit_message @@ -491,6 +486,9 @@ class MockCheckout(object): def apply_reverse_diff(self, revision): pass + def suggested_reviewers(self, git_commit, changed_files=None): + return [_mock_reviewer] + class MockUser(object): @@ -508,6 +506,7 @@ class MockUser(object): pass def confirm(self, message=None, default='y'): + print message return default == 'y' def can_open_url(self): @@ -545,7 +544,7 @@ class MockStatusServer(object): def next_work_item(self, queue_name): if not self._work_items: return None - return self._work_items[0] + return self._work_items.pop(0) def release_work_item(self, queue_name, patch): log("MOCK: release_work_item: %s %s" % (queue_name, patch.id())) @@ -568,7 +567,8 @@ class MockStatusServer(object): return "http://dummy_url" -class MockExecute(Mock): +# FIXME: This should not inherit from Mock +class MockExecutive(Mock): def __init__(self, should_log): self._should_log = should_log @@ -603,47 +603,37 @@ class MockOptions(object): self.__dict__[key] = value -class MockRietveld(): - - def __init__(self, executive, dryrun=False): - pass - - def post(self, diff, patch_id, codereview_issue, message=None, cc=None): - log("MOCK: Uploading patch to rietveld") - - -class MockTestPort1(): +class MockTestPort1(object): def skips_layout_test(self, test_name): return test_name in ["media/foo/bar.html", "foo"] -class MockTestPort2(): +class MockTestPort2(object): def skips_layout_test(self, test_name): return test_name == "media/foo/bar.html" -class MockPortFactory(): +class MockPortFactory(object): def get_all(self, options=None): return {"test_port1": MockTestPort1(), "test_port2": MockTestPort2()} -class MockTool(): +class MockTool(object): def __init__(self, log_executive=False): self.wakeup_event = threading.Event() self.bugs = MockBugzilla() self.buildbot = MockBuildBot() - self.executive = MockExecute(should_log=log_executive) + self.executive = MockExecutive(should_log=log_executive) self._irc = None self.user = MockUser() self._scm = MockSCM() self._checkout = MockCheckout() self.status_server = MockStatusServer() self.irc_password = "MOCK irc password" - self.codereview = MockRietveld(self.executive) self.port_factory = MockPortFactory() def scm(self): diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py b/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py index d59cdc5..64d9d05 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py @@ -44,7 +44,6 @@ from webkitpy.tool.steps.ensurebuildersaregreen import EnsureBuildersAreGreen from webkitpy.tool.steps.ensurelocalcommitifneeded import EnsureLocalCommitIfNeeded from webkitpy.tool.steps.obsoletepatches import ObsoletePatches from webkitpy.tool.steps.options import Options -from webkitpy.tool.steps.postcodereview import PostCodeReview from webkitpy.tool.steps.postdiff import PostDiff from webkitpy.tool.steps.postdiffforcommit import PostDiffForCommit from webkitpy.tool.steps.postdiffforrevert import PostDiffForRevert @@ -54,6 +53,7 @@ from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle from webkitpy.tool.steps.reopenbugafterrollout import ReopenBugAfterRollout from webkitpy.tool.steps.revertrevision import RevertRevision from webkitpy.tool.steps.runtests import RunTests +from webkitpy.tool.steps.suggestreviewers import SuggestReviewers from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer from webkitpy.tool.steps.update import Update from webkitpy.tool.steps.validatereviewer import ValidateReviewer diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/options.py b/WebKitTools/Scripts/webkitpy/tool/steps/options.py index 835fdba..4f17dd3 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/options.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/options.py @@ -54,5 +54,6 @@ 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.") + suggest_reviewers = make_option("--suggest-reviewers", action="store_true", default=False, help="Offer to CC appropriate reviewers.") 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/revertrevision.py b/WebKitTools/Scripts/webkitpy/tool/steps/revertrevision.py index 81b6bcb..bbb794c 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/revertrevision.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/revertrevision.py @@ -32,3 +32,4 @@ from webkitpy.tool.steps.abstractstep import AbstractStep class RevertRevision(AbstractStep): def run(self, state): self._tool.checkout().apply_reverse_diff(state["revision"]) + self.did_modify_checkout(state) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py index dcbfc44..282e381 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py @@ -57,6 +57,7 @@ class RunTests(AbstractStep): log("Running run-webkit-tests") args = self._tool.port().run_webkit_tests_command() if self._options.non_interactive: + args.append("--no-new-test-results") args.append("--no-launch-safari") args.append("--exit-after-n-failures=1") args.append("--wait-for-httpd") diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py index 7eb8e3a..eabb656 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py @@ -87,6 +87,6 @@ MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/test-webkitperl'] Running JavaScriptCore tests MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/run-javascriptcore-tests'] Running run-webkit-tests -MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet'] +MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/run-webkit-tests', '--no-new-test-results', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet'] """ OutputCapture().assert_outputs(self, step.run, [{}], expected_stderr=expected_stderr) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers.py index d2f79f3..76bef35 100644 --- a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py +++ b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers.py @@ -30,41 +30,22 @@ from webkitpy.tool.steps.abstractstep import AbstractStep from webkitpy.tool.steps.options import Options -class PostCodeReview(AbstractStep): +class SuggestReviewers(AbstractStep): @classmethod def options(cls): return AbstractStep.options() + [ - Options.cc, - Options.description, + Options.git_commit, + Options.suggest_reviewers, ] def run(self, state): - patch = state.get("patch") - bug_id = patch.bug_id() - title = patch.name() + if not self._options.suggest_reviewers: + return - # If the issue already exists, then the message becomes the label - # of the new patch. Otherwise, it becomes the title of the whole - # issue. - if title: - # This is the common case for the the first "upload" command. - message = title - elif bug_id: - # This is the common case for the "post" command and - # subsequent runs of the "upload" command. - message = "Code review for %s" % self._tool.bugs.bug_url_for_bug_id(bug_id) - else: - # Unreachable with our current commands, but we might hit - # this case if we support bug-less code reviews. - message = "Code review" - - # Use the bug ID as the rietveld issue number. This means rietveld code reviews - # when there are multiple different patches on a bug will be a bit wonky, but - # webkit-patch assumes one-patch-per-bug. - created_issue = self._tool.codereview.post(diff=self.cached_lookup(state, "diff"), - message=message, - codereview_issue=bug_id, - cc=self._options.cc, - patch_id=patch.id()) - - self._tool.bugs.set_flag_on_attachment(patch.id(), 'in-rietveld', '+') + reviewers = self._tool.checkout().suggested_reviewers(self._options.git_commit, self._changed_files(state)) + print "The following reviewers have recently modified files in your patch:" + print "\n".join([reviewer.full_name for reviewer in reviewers]) + if not self._tool.user.confirm("Would you like to CC them?"): + return + reviewer_emails = [reviewer.bugzilla_email() for reviewer in reviewers] + self._tool.bugs.add_cc_to_bug(state['bug_id'], reviewer_emails) diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py new file mode 100644 index 0000000..0c86535 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py @@ -0,0 +1,45 @@ +# 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.outputcapture import OutputCapture +from webkitpy.tool.mocktool import MockOptions, MockTool +from webkitpy.tool.steps.suggestreviewers import SuggestReviewers + + +class SuggestReviewersTest(unittest.TestCase): + def test_disabled(self): + step = SuggestReviewers(MockTool(), MockOptions(suggest_reviewers=False)) + OutputCapture().assert_outputs(self, step.run, [{}]) + + def test_basic(self): + capture = OutputCapture() + step = SuggestReviewers(MockTool(), MockOptions(suggest_reviewers=True, git_commit=None)) + expected_stdout = "The following reviewers have recently modified files in your patch:\nFoo Bar\nWould you like to CC them?\n" + capture.assert_outputs(self, step.run, [{"bug_id": "123"}], expected_stdout=expected_stdout) |