diff options
Diffstat (limited to 'Tools/Scripts/webkitpy')
61 files changed, 1453 insertions, 206 deletions
diff --git a/Tools/Scripts/webkitpy/common/checkout/__init__.py b/Tools/Scripts/webkitpy/common/checkout/__init__.py index 597dcbd..ef65bee 100644 --- a/Tools/Scripts/webkitpy/common/checkout/__init__.py +++ b/Tools/Scripts/webkitpy/common/checkout/__init__.py @@ -1,3 +1 @@ # Required for Python to search this directory for module files - -from api import Checkout diff --git a/Tools/Scripts/webkitpy/common/checkout/api.py b/Tools/Scripts/webkitpy/common/checkout/api.py index 6357982..29e43d3 100644 --- a/Tools/Scripts/webkitpy/common/checkout/api.py +++ b/Tools/Scripts/webkitpy/common/checkout/api.py @@ -39,14 +39,14 @@ from webkitpy.common.system.executive import Executive, run_command, ScriptError from webkitpy.common.system.deprecated_logging import log -# This class represents the WebKit-specific parts of the checkout (like -# ChangeLogs). +# This class represents the WebKit-specific parts of the checkout (like ChangeLogs). # FIXME: Move a bunch of ChangeLog-specific processing from SCM to this object. +# NOTE: All paths returned from this class should be absolute. class Checkout(object): def __init__(self, scm): self._scm = scm - def _is_path_to_changelog(self, path): + def is_path_to_changelog(self, path): return os.path.basename(path) == "ChangeLog" def _latest_entry_for_changelog_at_revision(self, changelog_path, revision): @@ -59,7 +59,7 @@ class Checkout(object): def changelog_entries_for_revision(self, revision): 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)] + 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): @@ -96,10 +96,10 @@ class Checkout(object): return [path for path in absolute_paths if predicate(path)] def modified_changelogs(self, git_commit, changed_files=None): - return self._modified_files_matching_predicate(git_commit, self._is_path_to_changelog, changed_files=changed_files) + return self._modified_files_matching_predicate(git_commit, self.is_path_to_changelog, changed_files=changed_files) 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) + 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, changed_files=None): changelog_paths = self.modified_changelogs(git_commit, changed_files) diff --git a/Tools/Scripts/webkitpy/common/checkout/diff_parser.py b/Tools/Scripts/webkitpy/common/checkout/diff_parser.py index a6ea756..5a5546c 100644 --- a/Tools/Scripts/webkitpy/common/checkout/diff_parser.py +++ b/Tools/Scripts/webkitpy/common/checkout/diff_parser.py @@ -118,6 +118,8 @@ class DiffFile(object): self.lines.append((deleted_line_number, new_line_number, line)) +# If this is going to be called DiffParser, it should be a re-useable parser. +# Otherwise we should rename it to ParsedDiff or just Diff. class DiffParser(object): """A parser for a patch file. @@ -125,16 +127,18 @@ class DiffParser(object): a DiffFile object. """ - # FIXME: This function is way too long and needs to be broken up. def __init__(self, diff_input): """Parses a diff. Args: diff_input: An iterable object. """ - state = _INITIAL_STATE + self.files = self._parse_into_diff_files(diff_input) - self.files = {} + # FIXME: This function is way too long and needs to be broken up. + def _parse_into_diff_files(self, diff_input): + files = {} + state = _INITIAL_STATE current_file = None old_diff_line = None new_diff_line = None @@ -148,7 +152,7 @@ class DiffParser(object): if file_declaration: filename = file_declaration.group('FilePath') current_file = DiffFile(filename) - self.files[filename] = current_file + files[filename] = current_file state = _DECLARED_FILE_PATH continue @@ -179,3 +183,4 @@ class DiffParser(object): else: _log.error('Unexpected diff format when parsing a ' 'chunk: %r' % line) + return files diff --git a/Tools/Scripts/webkitpy/common/checkout/scm.py b/Tools/Scripts/webkitpy/common/checkout/scm.py index c54fb42..3f77043 100644 --- a/Tools/Scripts/webkitpy/common/checkout/scm.py +++ b/Tools/Scripts/webkitpy/common/checkout/scm.py @@ -172,14 +172,15 @@ class SCM: return os.path.join(self.scripts_directory(), script_name) def ensure_clean_working_directory(self, force_clean): - if not force_clean and not self.working_directory_is_clean(): + if self.working_directory_is_clean(): + return + if not force_clean: # FIXME: Shouldn't this use cwd=self.checkout_root? print self.run(self.status_command(), error_handler=Executive.ignore_error) raise ScriptError(message="Working directory has modifications, pass --force-clean or --no-clean to continue.") - log("Cleaning working directory") self.clean_working_directory() - + def ensure_no_local_commits(self, force): if not self.supports_local_commits(): return diff --git a/Tools/Scripts/webkitpy/common/config/build.py b/Tools/Scripts/webkitpy/common/config/build.py index 2a432ce..2b27c85 100644 --- a/Tools/Scripts/webkitpy/common/config/build.py +++ b/Tools/Scripts/webkitpy/common/config/build.py @@ -41,8 +41,8 @@ def _should_file_trigger_build(target_platform, file): directories = [ # Directories that shouldn't trigger builds on any bots. - ("PageLoadTests", []), - ("WebCore/manual-tests", []), + ("PerformanceTests", []), + ("Source/WebCore/manual-tests", []), ("Examples", []), ("Websites", []), ("android", []), @@ -57,10 +57,10 @@ def _should_file_trigger_build(target_platform, file): ("wince", []), # Directories that should trigger builds on only some bots. - ("JavaScriptGlue", ["mac"]), + ("Source/JavaScriptGlue", ["mac"]), ("LayoutTests/platform/mac", ["mac", "win"]), ("LayoutTests/platform/mac-snowleopard", ["mac-snowleopard", "win"]), - ("WebCore/image-decoders", ["chromium"]), + ("Source/WebCore/image-decoders", ["chromium"]), ("cairo", ["gtk", "wincairo"]), ("cf", ["chromium-mac", "mac", "qt", "win"]), ("chromium", ["chromium"]), diff --git a/Tools/Scripts/webkitpy/common/config/build_unittest.py b/Tools/Scripts/webkitpy/common/config/build_unittest.py index d833464..e07d42f 100644 --- a/Tools/Scripts/webkitpy/common/config/build_unittest.py +++ b/Tools/Scripts/webkitpy/common/config/build_unittest.py @@ -27,11 +27,11 @@ from webkitpy.common.config import build class ShouldBuildTest(unittest.TestCase): _should_build_tests = [ - (["Websites/bugs.webkit.org/foo", "WebCore/bar"], ["*"]), + (["Websites/bugs.webkit.org/foo", "Source/WebCore/bar"], ["*"]), (["Websites/bugs.webkit.org/foo"], []), - (["JavaScriptCore/JavaScriptCore.xcodeproj/foo"], ["mac-leopard", "mac-snowleopard"]), - (["JavaScriptGlue/foo", "WebCore/bar"], ["*"]), - (["JavaScriptGlue/foo"], ["mac-leopard", "mac-snowleopard"]), + (["Source/JavaScriptCore/JavaScriptCore.xcodeproj/foo"], ["mac-leopard", "mac-snowleopard"]), + (["Source/JavaScriptGlue/foo", "Source/WebCore/bar"], ["*"]), + (["Source/JavaScriptGlue/foo"], ["mac-leopard", "mac-snowleopard"]), (["LayoutTests/foo"], ["*"]), (["LayoutTests/platform/chromium-linux/foo"], ["chromium-linux"]), (["LayoutTests/platform/chromium-win/fast/compact/001-expected.txt"], ["chromium-win"]), @@ -42,13 +42,13 @@ class ShouldBuildTest(unittest.TestCase): (["LayoutTests/platform/win-xp/foo"], ["win"]), (["LayoutTests/platform/win-wk2/foo"], ["win"]), (["LayoutTests/platform/win/foo"], ["win"]), - (["WebCore/mac/foo"], ["chromium-mac", "mac-leopard", "mac-snowleopard"]), - (["WebCore/win/foo"], ["chromium-win", "win"]), - (["WebCore/platform/graphics/gpu/foo"], ["mac-leopard", "mac-snowleopard"]), - (["WebCore/platform/wx/wxcode/win/foo"], []), - (["WebCore/rendering/RenderThemeMac.mm", "WebCore/rendering/RenderThemeMac.h"], ["mac-leopard", "mac-snowleopard"]), - (["WebCore/rendering/RenderThemeChromiumLinux.h"], ["chromium-linux"]), - (["WebCore/rendering/RenderThemeWinCE.h"], []), + (["Source/WebCore/mac/foo"], ["chromium-mac", "mac-leopard", "mac-snowleopard"]), + (["Source/WebCore/win/foo"], ["chromium-win", "win"]), + (["Source/WebCore/platform/graphics/gpu/foo"], ["mac-leopard", "mac-snowleopard"]), + (["Source/WebCore/platform/wx/wxcode/win/foo"], []), + (["Source/WebCore/rendering/RenderThemeMac.mm", "Source/WebCore/rendering/RenderThemeMac.h"], ["mac-leopard", "mac-snowleopard"]), + (["Source/WebCore/rendering/RenderThemeChromiumLinux.h"], ["chromium-linux"]), + (["Source/WebCore/rendering/RenderThemeWinCE.h"], []), ] def test_should_build(self): diff --git a/Tools/Scripts/webkitpy/common/config/committers.py b/Tools/Scripts/webkitpy/common/config/committers.py index 7c5bf8b..c7c741b 100644 --- a/Tools/Scripts/webkitpy/common/config/committers.py +++ b/Tools/Scripts/webkitpy/common/config/committers.py @@ -89,6 +89,7 @@ committers_unable_to_review = [ Committer("Brett Wilson", "brettw@chromium.org", "brettx"), Committer("Brian Weinstein", "bweinstein@apple.com", "bweinstein"), Committer("Cameron McCormack", "cam@webkit.org", "heycam"), + Committer("Carlos Garcia Campos", ["cgarcia@igalia.com", "carlosgc@gnome.org", "carlosgc@webkit.org"], "KaL"), Committer("Carol Szabo", "carol.szabo@nokia.com"), Committer("Chang Shu", "Chang.Shu@nokia.com"), Committer("Chris Evans", "cevans@google.com"), @@ -122,6 +123,7 @@ committers_unable_to_review = [ Committer("Jakob Petsovits", ["jpetsovits@rim.com", "jpetso@gmx.at"], "jpetso"), Committer("Jakub Wieczorek", "jwieczorek@webkit.org", "fawek"), Committer("James Hawkins", ["jhawkins@chromium.org", "jhawkins@google.com"], "jhawkins"), + Committer("James Simonsen", "simonjam@chromium.org", "simonjam"), Committer("Jay Civelli", "jcivelli@chromium.org", "jcivelli"), Committer("Jens Alfke", ["snej@chromium.org", "jens@apple.com"]), Committer("Jer Noble", "jer.noble@apple.com", "jernoble"), @@ -160,7 +162,6 @@ committers_unable_to_review = [ Committer("Michael Nordman", "michaeln@google.com", "michaeln"), Committer("Michael Saboff", "msaboff@apple.com"), Committer("Michelangelo De Simone", "michelangelo@webkit.org", "michelangelo"), - Committer("Mihai Parparita", "mihaip@chromium.org", "mihaip"), Committer("Mike Belshe", ["mbelshe@chromium.org", "mike@belshe.com"]), Committer("Mike Fenton", ["mifenton@rim.com", "mike.fenton@torchmobile.com"], "mfenton"), Committer("Mike Thole", ["mthole@mikethole.com", "mthole@apple.com"]), @@ -175,6 +176,7 @@ committers_unable_to_review = [ Committer("Philippe Normand", ["pnormand@igalia.com", "philn@webkit.org"], "philn-tp"), Committer("Pierre d'Herbemont", ["pdherbemont@free.fr", "pdherbemont@apple.com"], "pdherbemont"), Committer("Pierre-Olivier Latour", "pol@apple.com", "pol"), + Committer("Pratik Solanki", "psolanki@apple.com", "psolanki"), Committer("Renata Hodovan", "reni@webkit.org", "reni"), Committer("Robert Hogan", ["robert@webkit.org", "robert@roberthogan.net", "lists@roberthogan.net"], "mwenge"), Committer("Roland Steiner", "rolandsteiner@chromium.org"), @@ -269,6 +271,7 @@ reviewers_list = [ Reviewer("Maciej Stachowiak", "mjs@apple.com", "othermaciej"), Reviewer("Mark Rowe", "mrowe@apple.com", "bdash"), Reviewer("Martin Robinson", ["mrobinson@webkit.org", "mrobinson@igalia.com", "martin.james.robinson@gmail.com"], "mrobinson"), + Reviewer("Mihai Parparita", "mihaip@chromium.org", "mihaip"), Reviewer("Nate Chapin", "japhet@chromium.org", "japhet"), Reviewer("Nikolas Zimmermann", ["zimmermann@kde.org", "zimmermann@physik.rwth-aachen.de", "zimmermann@webkit.org"], "wildfox"), Reviewer("Ojan Vafai", "ojan@chromium.org", "ojan"), diff --git a/Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py b/Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py index 3cb6da5..98e2fae 100644 --- a/Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py +++ b/Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py @@ -270,6 +270,7 @@ class BuildBot(object): "Leopard", "Tiger", "Windows.*Build", + "EFL", "GTK.*32", "GTK.*64.*Debug", # Disallow the 64-bit Release bot which is broken. "Qt", diff --git a/Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py b/Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py index a10e432..ba898ec 100644 --- a/Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py +++ b/Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py @@ -30,10 +30,15 @@ import unittest from webkitpy.common.net.layouttestresults import LayoutTestResults from webkitpy.common.net.buildbot import BuildBot, Builder, Build +from webkitpy.layout_tests.layout_package import test_results +from webkitpy.layout_tests.layout_package import test_failures from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup class BuilderTest(unittest.TestCase): + def _mock_test_result(self, testname): + return test_results.TestResult(testname, [test_failures.FailureTextMismatch()]) + def _install_fetch_build(self, failure): def _mock_fetch_build(build_number): build = Build( @@ -42,8 +47,8 @@ class BuilderTest(unittest.TestCase): revision=build_number + 1000, is_green=build_number < 4 ) - parsed_results = {LayoutTestResults.fail_key: failure(build_number)} - build._layout_test_results = LayoutTestResults(parsed_results) + results = [self._mock_test_result(testname) for testname in failure(build_number)] + build._layout_test_results = LayoutTestResults(results) return build self.builder._fetch_build = _mock_fetch_build @@ -254,6 +259,7 @@ class BuildBotTest(unittest.TestCase): "Leopard", "Tiger", "Windows.*Build", + "EFL", "GTK.*32", "GTK.*64.*Debug", # Disallow the 64-bit Release bot which is broken. "Qt", diff --git a/Tools/Scripts/webkitpy/common/net/layouttestresults.py b/Tools/Scripts/webkitpy/common/net/layouttestresults.py index 15e95ce..28caad4 100644 --- a/Tools/Scripts/webkitpy/common/net/layouttestresults.py +++ b/Tools/Scripts/webkitpy/common/net/layouttestresults.py @@ -27,8 +27,12 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. # # A module for parsing results.html files generated by old-run-webkit-tests +# This class is one big hack and only needs to exist until we transition to new-run-webkit-tests. +from webkitpy.common.system.deprecated_logging import log from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup, SoupStrainer +from webkitpy.layout_tests.layout_package import test_results +from webkitpy.layout_tests.layout_package import test_failures # FIXME: This should be unified with all the layout test results code in the layout_tests package @@ -57,36 +61,89 @@ class LayoutTestResults(object): ] @classmethod + def _failures_from_fail_row(self, row): + # Look at all anchors in this row, and guess what type + # of new-run-webkit-test failures they equate to. + failures = set() + test_name = None + for anchor in row.findAll("a"): + anchor_text = unicode(anchor.string) + if not test_name: + test_name = anchor_text + continue + if anchor_text in ["expected image", "image diffs"] or '%' in anchor_text: + failures.add(test_failures.FailureImageHashMismatch()) + elif anchor_text in ["expected", "actual", "diff", "pretty diff"]: + failures.add(test_failures.FailureTextMismatch()) + else: + log("Unhandled link text in results.html parsing: %s. Please file a bug against webkitpy." % anchor_text) + # FIXME: Its possible the row contained no links due to ORWT brokeness. + # We should probably assume some type of failure anyway. + return failures + + @classmethod + def _failures_from_row(cls, row, table_title): + if table_title == cls.fail_key: + return cls._failures_from_fail_row(row) + if table_title == cls.crash_key: + return [test_failures.FailureCrash()] + if table_title == cls.timeout_key: + return [test_failures.FailureTimeout()] + if table_title == cls.missing_key: + return [test_failures.FailureMissingResult(), test_failures.FailureMissingImageHash(), test_failures.FailureMissingImage()] + return None + + @classmethod + def _test_result_from_row(cls, row, table_title): + test_name = unicode(row.find("a").string) + failures = cls._failures_from_row(row, table_title) + # TestResult is a class designed to work with new-run-webkit-tests. + # old-run-webkit-tests does not save quite enough information in results.html for us to parse. + # FIXME: It's unclear if test_name should include LayoutTests or not. + return test_results.TestResult(test_name, failures) + + @classmethod + def _parse_results_table(cls, table): + table_title = unicode(table.findPreviousSibling("p").string) + if table_title not in cls.expected_keys: + # This Exception should only ever be hit if run-webkit-tests changes its results.html format. + raise Exception("Unhandled title: %s" % table_title) + # Ignore stderr failures. Everyone ignores them anyway. + if table_title == cls.stderr_key: + return [] + # FIXME: We might end with two TestResults object for the same test if it appears in more than one row. + return [cls._test_result_from_row(row, table_title) for row in table.findAll("tr")] + + @classmethod def _parse_results_html(cls, page): - if not page: - return None - parsed_results = {} tables = BeautifulSoup(page).findAll("table") - for table in tables: - table_title = unicode(table.findPreviousSibling("p").string) - if table_title not in cls.expected_keys: - # This Exception should only ever be hit if run-webkit-tests changes its results.html format. - raise Exception("Unhandled title: %s" % table_title) - # We might want to translate table titles into identifiers before storing. - parsed_results[table_title] = [unicode(row.find("a").string) for row in table.findAll("tr")] - - return parsed_results + return sum([cls._parse_results_table(table) for table in tables], []) @classmethod def results_from_string(cls, string): - parsed_results = cls._parse_results_html(string) - if not parsed_results: + if not string: return None - return cls(parsed_results) + test_results = cls._parse_results_html(string) + if not test_results: + return None + return cls(test_results) + + def __init__(self, test_results): + self._test_results = test_results + + def test_results(self): + return self._test_results - def __init__(self, parsed_results): - self._parsed_results = parsed_results + def results_matching_failure_types(self, failure_types): + return [result for result in self._test_results if result.has_failure_matching_types(failure_types)] - def parsed_results(self): - return self._parsed_results + def tests_matching_failure_types(self, failure_types): + return [result.filename for result in self.results_matching_failure_types(failure_types)] - def results_matching_keys(self, result_keys): - return sorted(sum([tests for key, tests in self._parsed_results.items() if key in result_keys], [])) + def failing_test_results(self): + # These should match the "fail", "crash", and "timeout" keys. + failure_types = [test_failures.FailureTextMismatch, test_failures.FailureImageHashMismatch, test_failures.FailureCrash, test_failures.FailureTimeout] + return self.results_matching_failure_types(failure_types) def failing_tests(self): - return self.results_matching_keys([self.fail_key, self.crash_key, self.timeout_key]) + return [result.filename for result in self.failing_test_results()] diff --git a/Tools/Scripts/webkitpy/common/net/layouttestresults_unittest.py b/Tools/Scripts/webkitpy/common/net/layouttestresults_unittest.py index 8490eae..01b91b8 100644 --- a/Tools/Scripts/webkitpy/common/net/layouttestresults_unittest.py +++ b/Tools/Scripts/webkitpy/common/net/layouttestresults_unittest.py @@ -29,6 +29,10 @@ import unittest from webkitpy.common.net.layouttestresults import LayoutTestResults +from webkitpy.common.system.outputcapture import OutputCapture +from webkitpy.layout_tests.layout_package import test_results +from webkitpy.layout_tests.layout_package import test_failures +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup class LayoutTestResultsTest(unittest.TestCase): @@ -57,21 +61,28 @@ class LayoutTestResultsTest(unittest.TestCase): </html> """ - _expected_layout_test_results = { - 'Tests that had stderr output:': [ - 'accessibility/aria-activedescendant-crash.html', - ], - 'Tests that had no expected results (probably new):': [ - 'fast/repaint/no-caret-repaint-in-non-content-editable-element.html', - ], - } - def test_parse_layout_test_results(self): + failures = [test_failures.FailureMissingResult(), test_failures.FailureMissingImageHash(), test_failures.FailureMissingImage()] + testname = 'fast/repaint/no-caret-repaint-in-non-content-editable-element.html' + expected_results = [test_results.TestResult(testname, failures)] + results = LayoutTestResults._parse_results_html(self._example_results_html) - self.assertEqual(self._expected_layout_test_results, results) + self.assertEqual(expected_results, results) def test_results_from_string(self): self.assertEqual(LayoutTestResults.results_from_string(None), None) self.assertEqual(LayoutTestResults.results_from_string(""), None) results = LayoutTestResults.results_from_string(self._example_results_html) self.assertEqual(len(results.failing_tests()), 0) + + def test_failures_from_fail_row(self): + row = BeautifulSoup("<tr><td><a>test.hml</a></td><td><a>expected image</a></td><td><a>25%</a></td></tr>") + test_name = unicode(row.find("a").string) + # Even if the caller has already found the test name, findAll inside _failures_from_fail_row will see it again. + failures = OutputCapture().assert_outputs(self, LayoutTestResults._failures_from_fail_row, [row]) + self.assertEqual(len(failures), 1) + self.assertEqual(type(sorted(failures)[0]), test_failures.FailureImageHashMismatch) + + row = BeautifulSoup("<tr><td><a>test.hml</a><a>foo</a></td></tr>") + expected_stderr = "Unhandled link text in results.html parsing: foo. Please file a bug against webkitpy.\n" + OutputCapture().assert_outputs(self, LayoutTestResults._failures_from_fail_row, [row], expected_stderr=expected_stderr) diff --git a/Tools/Scripts/webkitpy/common/net/networktransaction.py b/Tools/Scripts/webkitpy/common/net/networktransaction.py index de19e94..21cc71f 100644 --- a/Tools/Scripts/webkitpy/common/net/networktransaction.py +++ b/Tools/Scripts/webkitpy/common/net/networktransaction.py @@ -58,8 +58,7 @@ class NetworkTransaction(object): if self._convert_404_to_None and e.code == 404: return None self._check_for_timeout() - _log.warn("Received HTTP status %s from server. Retrying in " - "%s seconds..." % (e.code, self._backoff_seconds)) + _log.warn("Received HTTP status %s loading \"%s\". Retrying in %s seconds..." % (e.code, e.filename, self._backoff_seconds)) self._sleep() def _check_for_timeout(self): diff --git a/Tools/Scripts/webkitpy/common/net/networktransaction_unittest.py b/Tools/Scripts/webkitpy/common/net/networktransaction_unittest.py index 49aaeed..c4cd4e0 100644 --- a/Tools/Scripts/webkitpy/common/net/networktransaction_unittest.py +++ b/Tools/Scripts/webkitpy/common/net/networktransaction_unittest.py @@ -70,9 +70,9 @@ class NetworkTransactionTest(LoggingTestCase): transaction = NetworkTransaction(initial_backoff_seconds=0) self.assertEqual(transaction.run(lambda: self._raise_500_error()), 42) self.assertEqual(self._run_count, 3) - self.assertLog(['WARNING: Received HTTP status 500 from server. ' + self.assertLog(['WARNING: Received HTTP status 500 loading "http://example.com/". ' 'Retrying in 0 seconds...\n', - 'WARNING: Received HTTP status 500 from server. ' + 'WARNING: Received HTTP status 500 loading "http://example.com/". ' 'Retrying in 0.0 seconds...\n']) def test_convert_404_to_None(self): diff --git a/Tools/Scripts/webkitpy/common/net/statusserver.py b/Tools/Scripts/webkitpy/common/net/statusserver.py index 64dd77b..abd298a 100644 --- a/Tools/Scripts/webkitpy/common/net/statusserver.py +++ b/Tools/Scripts/webkitpy/common/net/statusserver.py @@ -69,6 +69,13 @@ class StatusServer: return self._browser.add_file(results_file, "text/plain", "results.txt", 'results_file') + # 500 is the AppEngine limit for TEXT fields (which most of our fields are). + # Exceeding the limit will result in a 500 error from the server. + def _set_field(self, field_name, value, limit=500): + if len(value) > limit: + _log.warn("Attempted to set %s to value exceeding %s characters, truncating." % (field_name, limit)) + self._browser[field_name] = value[:limit] + def _post_status_to_server(self, queue_name, status, patch, results_file): if results_file: # We might need to re-wind the file if we've already tried to post it. @@ -81,7 +88,7 @@ class StatusServer: if self.bot_id: self._browser["bot_id"] = self.bot_id self._add_patch(patch) - self._browser["status"] = status + self._set_field("status", status, limit=500) self._add_results_file(results_file) return self._browser.submit().read() # This is the id of the newly created status object. diff --git a/Tools/Scripts/webkitpy/common/system/directoryfileset.py b/Tools/Scripts/webkitpy/common/system/directoryfileset.py new file mode 100644 index 0000000..11acaf4 --- /dev/null +++ b/Tools/Scripts/webkitpy/common/system/directoryfileset.py @@ -0,0 +1,77 @@ +# 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: +# +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. 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. +# +# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS 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 APPLE OR ITS 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. + +from __future__ import with_statement + +import os +import shutil + +from webkitpy.common.system.fileset import FileSetFileHandle +from webkitpy.common.system.filesystem import FileSystem +import webkitpy.common.system.ospath as ospath + + +class DirectoryFileSet(object): + """The set of files under a local directory.""" + def __init__(self, path, filesystem=None): + self._path = path + self._filesystem = filesystem or FileSystem() + if not self._path.endswith(os.path.sep): + self._path += os.path.sep + + def _full_path(self, filename): + assert self._is_under(self._path, filename) + return self._filesystem.join(self._path, filename) + + def _drop_directory_prefix(self, path): + return path[len(self._path):] + + def _files_in_directory(self): + """Returns a list of all the files in the directory, including the path + to the directory""" + return self._filesystem.files_under(self._path) + + def _is_under(self, dir, filename): + return bool(ospath.relpath(self._filesystem.join(dir, filename), dir)) + + def open(self, filename): + return FileSetFileHandle(self, filename, self._filesystem) + + def namelist(self): + return map(self._drop_directory_prefix, self._files_in_directory()) + + def read(self, filename): + return self._filesystem.read_text_file(self._full_path(filename)) + + def extract(self, filename, path): + """Extracts a file from this file set to the specified directory.""" + src = self._full_path(filename) + dest = self._filesystem.join(path, filename) + # As filename may have slashes in it, we must ensure that the same + # directory hierarchy exists at the output path. + self._filesystem.maybe_make_directory(os.path.split(dest)[0]) + self._filesystem.copyfile(src, dest) + + def delete(self, filename): + filename = self._full_path(filename) + self._filesystem.remove(filename) diff --git a/Tools/Scripts/webkitpy/common/system/directoryfileset_unittest.py b/Tools/Scripts/webkitpy/common/system/directoryfileset_unittest.py new file mode 100644 index 0000000..a3850ee --- /dev/null +++ b/Tools/Scripts/webkitpy/common/system/directoryfileset_unittest.py @@ -0,0 +1,70 @@ +# 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: +# +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. 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. +# +# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS 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 APPLE OR ITS 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. + +from __future__ import with_statement + +import unittest + +from webkitpy.common.system.directoryfileset import DirectoryFileSet +from webkitpy.common.system.filesystem_mock import MockFileSystem + + +class DirectoryFileSetTest(unittest.TestCase): + def setUp(self): + files = {} + files['/test/some-file'] = 'contents' + files['/test/some-other-file'] = 'other contents' + files['/test/b/c'] = 'c' + self._filesystem = MockFileSystem(files) + self._fileset = DirectoryFileSet('/test', self._filesystem) + + def test_files_in_namelist(self): + self.assertTrue('some-file' in self._fileset.namelist()) + self.assertTrue('some-other-file' in self._fileset.namelist()) + self.assertTrue('b/c' in self._fileset.namelist()) + + def test_read(self): + self.assertEquals('contents', self._fileset.read('some-file')) + + def test_open(self): + file = self._fileset.open('some-file') + self.assertEquals('some-file', file.name()) + self.assertEquals('contents', file.contents()) + + def test_extract(self): + self._fileset.extract('some-file', '/test-directory') + contents = self._filesystem.read_text_file('/test-directory/some-file') + self.assertEquals('contents', contents) + + def test_extract_deep_file(self): + self._fileset.extract('b/c', '/test-directory') + self.assertTrue(self._filesystem.exists('/test-directory/b/c')) + + def test_delete(self): + self.assertTrue(self._filesystem.exists('/test/some-file')) + self._fileset.delete('some-file') + self.assertFalse(self._filesystem.exists('/test/some-file')) + + +if __name__ == '__main__': + unittest.main() diff --git a/Tools/Scripts/webkitpy/common/system/fileset.py b/Tools/Scripts/webkitpy/common/system/fileset.py new file mode 100644 index 0000000..22f7c4d --- /dev/null +++ b/Tools/Scripts/webkitpy/common/system/fileset.py @@ -0,0 +1,64 @@ +# 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: +# +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. 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. +# +# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS 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 APPLE OR ITS 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. + +from __future__ import with_statement +import os + +from webkitpy.common.system.filesystem import FileSystem + + +class FileSetFileHandle(object): + """Points to a file that resides in a file set""" + def __init__(self, fileset, filename, filesystem=None): + self._filename = filename + self._fileset = fileset + self._contents = None + self._filesystem = filesystem or FileSystem() + + def __str__(self): + return "%s:%s" % (self._fileset, self._filename) + + def contents(self): + if self._contents is None: + self._contents = self._fileset.read(self._filename) + return self._contents + + def save_to(self, path, filename=None): + if filename is None: + self._fileset.extract(self._filename, path) + return + with self._filesystem.mkdtemp() as temp_dir: + self._fileset.extract(self._filename, temp_dir) + + src = self._filesystem.join(temp_dir, self._filename) + dest = self._filesystem.join(path, filename) + self._filesystem.copyfile(src, dest) + + def delete(self): + self._fileset.delete(self._filename) + + def name(self): + return self._filename + + def splitext(self): + return os.path.splitext(self.name()) diff --git a/Tools/Scripts/webkitpy/common/system/filesystem.py b/Tools/Scripts/webkitpy/common/system/filesystem.py index f0b5e44..8830ea8 100644 --- a/Tools/Scripts/webkitpy/common/system/filesystem.py +++ b/Tools/Scripts/webkitpy/common/system/filesystem.py @@ -93,7 +93,7 @@ class FileSystem(object): def maybe_make_directory(self, *path): """Create the specified directory if it doesn't already exist.""" try: - os.makedirs(os.path.join(*path)) + os.makedirs(self.join(*path)) except OSError, e: if e.errno != errno.EEXIST: raise @@ -152,3 +152,9 @@ class FileSystem(object): """Copies the contents of the file at the given path to the destination path.""" shutil.copyfile(source, destination) + + def files_under(self, path): + """Return the list of all files under the given path.""" + return [self.join(path_to_file, filename) + for (path_to_file, _, filenames) in os.walk(path) + for filename in filenames] diff --git a/Tools/Scripts/webkitpy/common/system/filesystem_mock.py b/Tools/Scripts/webkitpy/common/system/filesystem_mock.py index ea0f3f9..c605cb2 100644 --- a/Tools/Scripts/webkitpy/common/system/filesystem_mock.py +++ b/Tools/Scripts/webkitpy/common/system/filesystem_mock.py @@ -29,6 +29,7 @@ import errno import os import path +import re class MockFileSystem(object): @@ -57,7 +58,7 @@ class MockFileSystem(object): return any(f.startswith(path) for f in self.files) def join(self, *comps): - return '/'.join(comps) + return re.sub(re.escape(os.path.sep), '/', os.path.join(*comps)) def listdir(self, path): if not self.isdir(path): @@ -107,3 +108,11 @@ class MockFileSystem(object): raise IOError(errno.EISDIR, destination, os.strerror(errno.ISDIR)) self.files[destination] = self.files[source] + + def files_under(self, path): + if not path.endswith('/'): + path += '/' + return [file for file in self.files if file.startswith(path)] + + def remove(self, path): + del self.files[path] diff --git a/Tools/Scripts/webkitpy/common/system/zipfileset.py b/Tools/Scripts/webkitpy/common/system/zipfileset.py new file mode 100644 index 0000000..fa2b762 --- /dev/null +++ b/Tools/Scripts/webkitpy/common/system/zipfileset.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: +# +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. 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. +# +# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS 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 APPLE OR ITS 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 urllib +import zipfile + +from webkitpy.common.net.networktransaction import NetworkTransaction +from webkitpy.common.system.fileset import FileSetFileHandle +from webkitpy.common.system.filesystem import FileSystem + + +class ZipFileSet(object): + """The set of files in a zip file that resides at a URL (local or remote)""" + def __init__(self, zip_url, filesystem=None, zip_factory=None): + self._zip_url = zip_url + self._zip_file = None + self._filesystem = filesystem or FileSystem() + self._zip_factory = zip_factory or self._retrieve_zip_file + + def _retrieve_zip_file(self, zip_url): + temp_file = NetworkTransaction().run(lambda: urllib.urlretrieve(zip_url)[0]) + return zipfile.ZipFile(temp_file) + + def _load(self): + if self._zip_file is None: + self._zip_file = self._zip_factory(self._zip_url) + + def open(self, filename): + self._load() + return FileSetFileHandle(self, filename, self._filesystem) + + def namelist(self): + self._load() + return self._zip_file.namelist() + + def read(self, filename): + self._load() + return self._zip_file.read(filename) + + def extract(self, filename, path): + self._load() + self._zip_file.extract(filename, path) + + def delete(self, filename): + raise Exception("Can't delete from a ZipFileSet.") diff --git a/Tools/Scripts/webkitpy/common/system/zipfileset_unittest.py b/Tools/Scripts/webkitpy/common/system/zipfileset_unittest.py new file mode 100644 index 0000000..a9ba5ad --- /dev/null +++ b/Tools/Scripts/webkitpy/common/system/zipfileset_unittest.py @@ -0,0 +1,95 @@ +# 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: +# +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. 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. +# +# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS 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 APPLE OR ITS 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 os +import shutil +import tempfile +import unittest +import zipfile + +from webkitpy.common.system.filesystem_mock import MockFileSystem +from webkitpy.common.system.zipfileset import ZipFileSet + + +class FakeZip(object): + def __init__(self, filesystem): + self._filesystem = filesystem + self._files = {} + + def add_file(self, filename, contents): + self._files[filename] = contents + + def open(self, filename): + return FileSetFileHandle(self, filename, self._filesystem) + + def namelist(self): + return self._files.keys() + + def read(self, filename): + return self._files[filename] + + def extract(self, filename, path): + self._filesystem.write_text_file(self._filesystem.join(path, filename), self.read(filename)) + + def delete(self, filename): + raise Exception("Can't delete from a ZipFileSet.") + + +class ZipFileSetTest(unittest.TestCase): + def setUp(self): + self._filesystem = MockFileSystem() + self._zip = ZipFileSet('blah', self._filesystem, self.make_fake_zip) + + def make_fake_zip(self, zip_url): + result = FakeZip(self._filesystem) + result.add_file('some-file', 'contents') + result.add_file('a/b/some-other-file', 'other contents') + return result + + def test_open(self): + file = self._zip.open('a/b/some-other-file') + self.assertEquals('a/b/some-other-file', file.name()) + self.assertEquals('other contents', file.contents()) + + def test_read(self): + self.assertEquals('contents', self._zip.read('some-file')) + + def test_extract(self): + self._filesystem.maybe_make_directory('/some-dir') + self._zip.extract('some-file', '/some-dir') + self.assertTrue(self._filesystem.isfile('/some-dir/some-file')) + + def test_deep_extract(self): + self._filesystem.maybe_make_directory('/some-dir') + self._zip.extract('a/b/some-other-file', '/some-dir') + self.assertTrue(self._filesystem.isfile('/some-dir/a/b/some-other-file')) + + def test_cant_delete(self): + self.assertRaises(Exception, self._zip.delete, 'some-file') + + def test_namelist(self): + self.assertTrue('some-file' in self._zip.namelist()) + + +if __name__ == '__main__': + unittest.main() diff --git a/Tools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py b/Tools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py index 309bf8d..47dc8a2 100644 --- a/Tools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py +++ b/Tools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py @@ -196,7 +196,7 @@ class ListDuplicatesTest(unittest.TestCase): ('LayoutTests/platform/mac/test.html', ('platform/mac/test.html', checkout_root)), (None, - ('platform/mac/test.html', os.path.join(checkout_root, 'WebCore'))), + ('platform/mac/test.html', os.path.join(checkout_root, 'Source', 'WebCore'))), ('test.html', ('platform/mac/test.html', os.path.join(layout_test_dir, 'platform/mac'))), (None, diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py index fdb8da6..2bb2d02 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py @@ -500,11 +500,9 @@ class TestShellThread(WatchableThread): result = worker.get_test_result() except AttributeError, e: # This gets raised if the worker thread has already exited. - failures = [] - _log.error('Cannot get results of test: %s' % - test_input.filename) - result = test_results.TestResult(test_input.filename, failures=[], - test_run_time=0, total_time_for_all_diffs=0, time_for_diffs={}) + _log.error('Cannot get results of test: %s' % test_input.filename) + # FIXME: Seems we want a unique failure type here. + result = test_results.TestResult(test_input.filename) return result diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py index 54d129b..12e65b2 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py @@ -46,6 +46,7 @@ import webkitpy.thirdparty.simplejson as simplejson _log = logging.getLogger("webkitpy.layout_tests.layout_package.json_results_generator") +# FIXME: We already have a TestResult class in test_results.py class TestResult(object): """A simple class that represents a single test result.""" diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py index 0e478c8..9280b02 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py @@ -139,9 +139,7 @@ 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 test_results.TestResult(path, failures, run_time, - total_time_for_all_diffs=0, - time_for_diffs=0) + return test_results.TestResult(path, failures=failures, test_run_time=run_time) def get_result_summary(self, tests, expectations_str): test_paths = [os.path.join(self._port.layout_tests_dir(), test) for diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py index 6d55761..5dd0114 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py @@ -35,6 +35,9 @@ import test_expectations import cPickle +# FIXME: This is backwards. Each TestFailure subclass should know what +# test_expectation type it corresponds too. Then this method just +# collects them all from the failure list and returns the worst one. def determine_result_type(failure_list): """Takes a set of test_failures and returns which result type best fits the list of failures. "Best fits" means we use the worst type of failure. @@ -88,6 +91,9 @@ class TestFailure(object): def __ne__(self, other): return self.__class__.__name__ != other.__class__.__name__ + def __hash__(self): + return hash(self.__class__.__name__) + def dumps(self): """Returns the string/JSON representation of a TestFailure.""" return cPickle.dumps(self) @@ -125,9 +131,6 @@ class FailureWithType(TestFailure): use the standard OutputLinks. """ - def __init__(self): - TestFailure.__init__(self) - # Filename suffixes used by ResultHtmlOutput. OUT_FILENAMES = () diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py index 3e3528d..b2698d1 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py @@ -80,5 +80,15 @@ class Test(unittest.TestCase): for c in ALL_FAILURE_CLASSES: self.assert_loads(c) + def test_equals(self): + self.assertEqual(FailureCrash(), FailureCrash()) + self.assertNotEqual(FailureCrash(), FailureTimeout()) + crash_set = set([FailureCrash(), FailureCrash()]) + self.assertEqual(len(crash_set), 1) + # The hash happens to be the name of the class, but sets still work: + crash_set = set([FailureCrash(), "FailureCrash"]) + self.assertEqual(len(crash_set), 2) + + if __name__ == '__main__': unittest.main() diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results.py index 2417fb7..055f65b 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results.py @@ -38,13 +38,14 @@ class TestResult(object): 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 + def __init__(self, filename, failures=None, test_run_time=None, total_time_for_all_diffs=None, time_for_diffs=None): 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.failures = failures or [] + self.test_run_time = test_run_time or 0 + self.total_time_for_all_diffs = total_time_for_all_diffs or 0 + self.time_for_diffs = time_for_diffs or {} # FIXME: Why is this a dictionary? + + # FIXME: Setting this in the constructor makes this class hard to mutate. self.type = test_failures.determine_result_type(failures) def __eq__(self, other): @@ -57,5 +58,11 @@ class TestResult(object): def __ne__(self, other): return not (self == other) + def has_failure_matching_types(self, types): + for failure in self.failures: + if type(failure) in types: + return True + return False + def dumps(self): return cPickle.dumps(self) diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results_unittest.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results_unittest.py index 5921666..c8fcf64 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results_unittest.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_results_unittest.py @@ -32,12 +32,20 @@ from test_results import TestResult class Test(unittest.TestCase): + def test_defaults(self): + result = TestResult("foo") + self.assertEqual(result.filename, 'foo') + self.assertEqual(result.failures, []) + self.assertEqual(result.test_run_time, 0) + self.assertEqual(result.total_time_for_all_diffs, 0) + self.assertEqual(result.time_for_diffs, {}) + 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) + time_for_diffs={}) s = result.dumps() new_result = TestResult.loads(s) self.assertTrue(isinstance(new_result, TestResult)) diff --git a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py index 24d04ca..5b02a00 100644 --- a/Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py +++ b/Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py @@ -251,6 +251,7 @@ class TestRunner: overrides=overrides_str) return self._expectations + # FIXME: This method is way too long and needs to be broken into pieces. def prepare_lists_and_print_output(self): """Create appropriate subsets of test lists and returns a ResultSummary object. Also prints expected test counts. @@ -384,9 +385,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 = test_results.TestResult(test, - failures=[], test_run_time=0, total_time_for_all_diffs=0, - time_for_diffs=0) + result = test_results.TestResult(test) result.type = test_expectations.SKIP result_summary.add(result, expected=True) self._printer.print_expected('') diff --git a/Tools/Scripts/webkitpy/layout_tests/port/chromium.py b/Tools/Scripts/webkitpy/layout_tests/port/chromium.py index 012e9cc..b90421a 100644 --- a/Tools/Scripts/webkitpy/layout_tests/port/chromium.py +++ b/Tools/Scripts/webkitpy/layout_tests/port/chromium.py @@ -380,8 +380,11 @@ class ChromiumDriver(base.Driver): if self._port.get_option('js_flags') is not None: cmd.append('--js-flags="' + self._port.get_option('js_flags') + '"') - if self._port.get_option('multiple_loads') > 0: - cmd.append('--multiple-loads=' + str(self._port.get_option('multiple_loads'))) + if self._port.get_option('stress_opt'): + cmd.append('--stress-opt') + + if self._port.get_option('stress_deopt'): + cmd.append('--stress-deopt') # test_shell does not support accelerated compositing. if not self._port.get_option("use_test_shell"): diff --git a/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py index f7e5330..c431765 100755 --- a/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py +++ b/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py @@ -215,10 +215,14 @@ 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("--stress-opt", action="store_true", + default=False, + help="Enable additional stress test to JavaScript optimization"), + optparse.make_option("--stress-deopt", action="store_true", + default=False, + help="Enable additional stress test to JavaScript optimization"), optparse.make_option("--nocheck-sys-deps", action="store_true", default=False, help="Don't check the system dependencies (themes)"), diff --git a/Tools/Scripts/webkitpy/style/checker.py b/Tools/Scripts/webkitpy/style/checker.py index 6f1beb0..3cfa1c9 100644 --- a/Tools/Scripts/webkitpy/style/checker.py +++ b/Tools/Scripts/webkitpy/style/checker.py @@ -131,10 +131,10 @@ _PATH_RULES_SPECIFIER = [ # values. "WebKit/efl/ewk/", # There is no clean way to avoid "yy_*" names used by flex. - "WebCore/css/CSSParser.cpp", + "Source/WebCore/css/CSSParser.cpp", # Qt code uses '_' in some places (such as private slots # and on test xxx_data methos on tests) - "JavaScriptCore/qt/api/", + "Source/JavaScriptCore/qt/", "WebKit/qt/Api/", "WebKit/qt/tests/", "WebKit/qt/declarative/", @@ -143,7 +143,7 @@ _PATH_RULES_SPECIFIER = [ ([# The GTK+ APIs use GTK+ naming style, which includes # lower-cased, underscore-separated values. # Also, GTK+ allows the use of NULL. - "WebCore/bindings/scripts/test/GObject", + "Source/WebCore/bindings/scripts/test/GObject", "WebKit/gtk/webkit/", "Tools/DumpRenderTree/gtk/"], ["-readability/naming", @@ -154,8 +154,12 @@ _PATH_RULES_SPECIFIER = [ ["-build/header_guard"]), ([# assembler has lots of opcodes that use underscores, so # we don't check for underscores in that directory. - "/JavaScriptCore/assembler/"], + "/Source/JavaScriptCore/assembler/"], ["-readability/naming"]), + ([# JITStubs has an usual syntax which causes false alarms for a few checks. + "JavaScriptCore/jit/JITStubs.cpp"], + ["-readability/parameter_name", + "-whitespace/parens"]), # WebKit2 rules: # WebKit2 doesn't use config.h, and certain directories have other @@ -165,11 +169,13 @@ _PATH_RULES_SPECIFIER = [ ["-build/include_order", "-readability/naming"]), ([# The WebKit2 C API has names with underscores and whitespace-aligned - # struct members. + # struct members. Also, we allow unnecessary parameter names in + # WebKit2 APIs because we're matching CF's header style. "WebKit2/UIProcess/API/C/", "WebKit2/WebProcess/InjectedBundle/API/c/"], ["-build/include_order", "-readability/naming", + "-readability/parameter_name", "-whitespace/declaration"]), ([# Nothing in WebKit2 uses config.h. "WebKit2/"], @@ -241,7 +247,7 @@ _SKIPPED_FILES_WITH_WARNING = [ # Soup API that is still being cooked, will be removed from WebKit # in a few months when it is merged into soup proper. The style # follows the libsoup style completely. - "WebCore/platform/network/soup/cache/", + "Source/WebCore/platform/network/soup/cache/", ] diff --git a/Tools/Scripts/webkitpy/style/checker_unittest.py b/Tools/Scripts/webkitpy/style/checker_unittest.py index d9057a8..a4649d2 100755 --- a/Tools/Scripts/webkitpy/style/checker_unittest.py +++ b/Tools/Scripts/webkitpy/style/checker_unittest.py @@ -222,11 +222,11 @@ class GlobalVariablesTest(unittest.TestCase): "readability/null") assertNoCheck("WebKit/efl/ewk/ewk_view.h", "readability/naming") - assertNoCheck("WebCore/css/CSSParser.cpp", + assertNoCheck("Source/WebCore/css/CSSParser.cpp", "readability/naming") # Test if Qt exceptions are indeed working - assertCheck("JavaScriptCore/qt/api/qscriptengine.cpp", + assertCheck("Source/JavaScriptCore/qt/api/qscriptengine.cpp", "readability/braces") assertCheck("WebKit/qt/Api/qwebpage.cpp", "readability/braces") @@ -236,7 +236,10 @@ class GlobalVariablesTest(unittest.TestCase): "readability/braces") assertCheck("WebKit/qt/examples/platformplugin/WebPlugin.cpp", "readability/braces") - assertNoCheck("JavaScriptCore/qt/api/qscriptengine.cpp", + assertNoCheck("Source/JavaScriptCore/qt/api/qscriptengine.cpp", + "readability/naming") + assertNoCheck("Source/JavaScriptCore/qt/benchmarks" + "/qscriptengine/tst_qscriptengine.cpp", "readability/naming") assertNoCheck("WebKit/qt/Api/qwebpage.cpp", "readability/naming") @@ -247,7 +250,7 @@ class GlobalVariablesTest(unittest.TestCase): assertNoCheck("WebKit/qt/examples/platformplugin/WebPlugin.cpp", "readability/naming") - assertNoCheck("WebCore/ForwardingHeaders/debugger/Debugger.h", + assertNoCheck("Source/WebCore/ForwardingHeaders/debugger/Debugger.h", "build/header_guard") # Third-party Python code: webkitpy/thirdparty @@ -296,8 +299,8 @@ class CheckerDispatcherSkipTest(unittest.TestCase): paths_to_skip = [ "gtk2drawing.c", "gtkdrawing.h", - "WebCore/platform/gtk/gtk2drawing.c", - "WebCore/platform/gtk/gtkdrawing.h", + "Source/WebCore/platform/gtk/gtk2drawing.c", + "Source/WebCore/platform/gtk/gtkdrawing.h", "WebKit/gtk/tests/testatk.c", ] @@ -485,8 +488,8 @@ class CheckerDispatcherDispatchTest(unittest.TestCase): "foo.wm", "foo.xhtml", "foo.y", - os.path.join("WebCore", "ChangeLog"), - os.path.join("WebCore", "inspector", "front-end", "inspector.js"), + os.path.join("Source", "WebCore", "ChangeLog"), + os.path.join("Source", "WebCore", "inspector", "front-end", "inspector.js"), os.path.join("Tools", "Scripts", "check-webkit-style"), ] @@ -505,7 +508,7 @@ class CheckerDispatcherDispatchTest(unittest.TestCase): def test_xml_paths(self): """Test paths that should be checked as XML.""" paths = [ - "WebCore/WebCore.vcproj/WebCore.vcproj", + "Source/WebCore/WebCore.vcproj/WebCore.vcproj", "WebKitLibraries/win/tools/vsprops/common.vsprops", ] diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp.py b/Tools/Scripts/webkitpy/style/checkers/cpp.py index 94e5bdd..4ea7d69 100644 --- a/Tools/Scripts/webkitpy/style/checkers/cpp.py +++ b/Tools/Scripts/webkitpy/style/checkers/cpp.py @@ -1,7 +1,7 @@ #!/usr/bin/python # -*- coding: utf-8 -*- # -# Copyright (C) 2009 Google Inc. All rights reserved. +# Copyright (C) 2009, 2010 Google Inc. All rights reserved. # Copyright (C) 2009 Torch Mobile Inc. # Copyright (C) 2009 Apple Inc. All rights reserved. # Copyright (C) 2010 Chris Jerdonek (cjerdonek@webkit.org) @@ -47,6 +47,8 @@ import string import sys import unicodedata +from webkitpy.common.memoized import memoized + # The key to use to provide a class to fake loading a header file. INCLUDE_IO_INJECTION_KEY = 'include_header_io' @@ -192,6 +194,34 @@ def iteratively_replace_matches_with_char(pattern, char_replacement, s): s = s[:start_match_index] + char_replacement * match_length + s[end_match_index:] +def _convert_to_lower_with_underscores(text): + """Converts all text strings in camelCase or PascalCase to lowers with underscores.""" + + # First add underscores before any capital letter followed by a lower case letter + # as long as it is in a word. + # (This put an underscore before Password but not P and A in WPAPassword). + text = sub(r'(?<=[A-Za-z0-9])([A-Z])(?=[a-z])', r'_\1', text) + + # Next add underscores before capitals at the end of words if it was + # preceeded by lower case letter or number. + # (This puts an underscore before A in isA but not A in CBA). + text = sub(r'(?<=[a-z0-9])([A-Z])(?=\b)', r'_\1', text) + + # Next add underscores when you have a captial letter which is followed by a capital letter + # but is not proceeded by one. (This puts an underscore before A in 'WordADay'). + text = sub(r'(?<=[a-z0-9])([A-Z][A-Z_])', r'_\1', text) + + return text.lower() + + + +def _create_acronym(text): + """Creates an acronym for the given text.""" + # Removes all lower case letters except those starting words. + text = sub(r'(?<!\b)[a-z]', '', text) + return text.upper() + + def up_to_unmatched_closing_paren(s): """Splits a string into two parts up to first unmatched ')'. @@ -308,6 +338,138 @@ class _IncludeState(dict): return error_message +class Position(object): + """Holds the position of something.""" + def __init__(self, row, column): + self.row = row + self.column = column + + +class Parameter(object): + """Information about one function parameter.""" + def __init__(self, parameter, parameter_name_index, row): + self.type = parameter[:parameter_name_index].strip() + # Remove any initializers from the parameter name (e.g. int i = 5). + self.name = sub(r'=.*', '', parameter[parameter_name_index:]).strip() + self.row = row + + @memoized + def lower_with_underscores_name(self): + """Returns the parameter name in the lower with underscores format.""" + return _convert_to_lower_with_underscores(self.name) + + +class SingleLineView(object): + """Converts multiple lines into a single line (with line breaks replaced by a + space) to allow for easier searching.""" + def __init__(self, lines, start_position, end_position): + """Create a SingleLineView instance. + + Args: + lines: a list of multiple lines to combine into a single line. + start_position: offset within lines of where to start the single line. + end_position: just after where to end (like a slice operation). + """ + # Get the rows of interest. + trimmed_lines = lines[start_position.row:end_position.row + 1] + + # Remove the columns on the last line that aren't included. + trimmed_lines[-1] = trimmed_lines[-1][:end_position.column] + + # Remove the columns on the first line that aren't included. + trimmed_lines[0] = trimmed_lines[0][start_position.column:] + + # Create a single line with all of the parameters. + self.single_line = ' '.join(trimmed_lines) + + # Keep the row lengths, so we can calculate the original row number + # given a column in the single line (adding 1 due to the space added + # during the join). + self._row_lengths = [len(line) + 1 for line in trimmed_lines] + self._starting_row = start_position.row + + def convert_column_to_row(self, single_line_column_number): + """Convert the column number from the single line into the original + line number. + + Special cases: + * Columns in the added spaces are considered part of the previous line. + * Columns beyond the end of the line are consider part the last line + in the view.""" + total_columns = 0 + row_offset = 0 + while row_offset < len(self._row_lengths) - 1 and single_line_column_number >= total_columns + self._row_lengths[row_offset]: + total_columns += self._row_lengths[row_offset] + row_offset += 1 + return self._starting_row + row_offset + + +def create_skeleton_parameters(all_parameters): + """Converts a parameter list to a skeleton version. + + The skeleton only has one word for the parameter name, one word for the type, + and commas after each parameter and only there. Everything in the skeleton + remains in the same columns as the original.""" + all_simplifications = ( + # Remove template parameters, function declaration parameters, etc. + r'(<[^<>]*?>)|(\([^\(\)]*?\))|(\{[^\{\}]*?\})', + # Remove all initializers. + r'=[^,]*', + # Remove :: and everything before it. + r'[^,]*::', + # Remove modifiers like &, *. + r'[&*]', + # Remove const modifiers. + r'\bconst\s+(?=[A-Za-z])', + # Remove numerical modifiers like long. + r'\b(unsigned|long|short)\s+(?=unsigned|long|short|int|char|double|float)') + + skeleton_parameters = all_parameters + for simplification in all_simplifications: + skeleton_parameters = iteratively_replace_matches_with_char(simplification, ' ', skeleton_parameters) + # If there are any parameters, then add a , after the last one to + # make a regular pattern of a , following every parameter. + if skeleton_parameters.strip(): + skeleton_parameters += ',' + return skeleton_parameters + + +def find_parameter_name_index(skeleton_parameter): + """Determines where the parametere name starts given the skeleton parameter.""" + # The first space from the right in the simplified parameter is where the parameter + # name starts unless the first space is before any content in the simplified parameter. + before_name_index = skeleton_parameter.rstrip().rfind(' ') + if before_name_index != -1 and skeleton_parameter[:before_name_index].strip(): + return before_name_index + 1 + return len(skeleton_parameter) + + +def parameter_list(elided_lines, start_position, end_position): + """Generator for a function's parameters.""" + # Create new positions that omit the outer parenthesis of the parameters. + start_position = Position(row=start_position.row, column=start_position.column + 1) + end_position = Position(row=end_position.row, column=end_position.column - 1) + single_line_view = SingleLineView(elided_lines, start_position, end_position) + skeleton_parameters = create_skeleton_parameters(single_line_view.single_line) + end_index = -1 + + while True: + # Find the end of the next parameter. + start_index = end_index + 1 + end_index = skeleton_parameters.find(',', start_index) + + # No comma means that all parameters have been parsed. + if end_index == -1: + return + row = single_line_view.convert_column_to_row(end_index) + + # Parse the parameter into a type and parameter name. + skeleton_parameter = skeleton_parameters[start_index:end_index] + name_offset = find_parameter_name_index(skeleton_parameter) + parameter = single_line_view.single_line[start_index:end_index] + yield Parameter(parameter, name_offset, row) + + class _FunctionState(object): """Tracks current function name and the number of lines in its body. @@ -329,7 +491,8 @@ class _FunctionState(object): self.body_start_line_number = -1000 self.ending_line_number = -1000 - def begin(self, function_name, body_start_line_number, ending_line_number, is_declaration): + def begin(self, function_name, body_start_line_number, ending_line_number, is_declaration, + parameter_start_position, parameter_end_position, clean_lines): """Start analyzing function body. Args: @@ -337,6 +500,9 @@ class _FunctionState(object): body_start_line_number: The line number of the { or the ; for a protoype. ending_line_number: The line number where the function ends. is_declaration: True if this is a prototype. + parameter_start_position: position in elided of the '(' for the parameters. + parameter_end_position: position in elided of the ')' for the parameters. + clean_lines: A CleansedLines instance containing the file. """ self.in_a_function = True self.lines_in_function = -1 # Don't count the open brace line. @@ -344,6 +510,17 @@ class _FunctionState(object): self.body_start_line_number = body_start_line_number self.ending_line_number = ending_line_number self.is_declaration = is_declaration + self.parameter_start_position = parameter_start_position + self.parameter_end_position = parameter_end_position + self._clean_lines = clean_lines + self._parameter_list = None + + def parameter_list(self): + if not self._parameter_list: + # Store the final result as a tuple since that is immutable. + self._parameter_list = tuple(parameter_list(self._clean_lines.elided, self.parameter_start_position, self.parameter_end_position)) + + return self._parameter_list def count(self, line_number): """Count line in current function body.""" @@ -1161,7 +1338,7 @@ def check_spacing_for_function_call(line, line_number, error): error(line_number, 'whitespace/parens', 2, 'Extra space after (') if (search(r'\w\s+\(', function_call) - and not search(r'#\s*define|typedef', function_call)): + and not match(r'\s*(#|typedef)', function_call)): error(line_number, 'whitespace/parens', 4, 'Extra space before ( in function call') # If the ) is followed only by a newline or a { + newline, assume it's @@ -1213,7 +1390,11 @@ def detect_functions(clean_lines, line_number, function_state, error): raw = clean_lines.raw_lines raw_line = raw[line_number] - regexp = r'\s*(\w(\w|::|\*|\&|\s|<|>|,|~)*)\(' # decls * & space::name( ... + # Lines ending with a \ indicate a macro. Don't try to check them. + if raw_line.endswith('\\'): + return + + regexp = r'\s*(\w(\w|::|\*|\&|\s|<|>|,|~|(operator\s*(/|-|=|!|\+)+))*)\(' # decls * & space::name( ... match_result = match(regexp, line) if not match_result: return @@ -1232,7 +1413,7 @@ def detect_functions(clean_lines, line_number, function_state, error): # Replace template constructs with _ so that no spaces remain in the function name, # while keeping the column numbers of other characters the same as "line". line_with_no_templates = iteratively_replace_matches_with_char(r'<[^<>]*>', '_', line) - match_function = search(r'((\w|:|<|>|,|~)*)\(', line_with_no_templates) + match_function = search(r'((\w|:|<|>|,|~|(operator\s*(/|-|=|!|\+)+))*)\(', line_with_no_templates) if not match_function: return # The '(' must have been inside of a template. @@ -1246,13 +1427,22 @@ def detect_functions(clean_lines, line_number, function_state, error): function += parameter_regexp.group(1) else: function += '()' + + parameter_start_position = Position(line_number, match_function.end(1)) + close_result = close_expression(clean_lines, line_number, parameter_start_position.column) + if close_result[1] == len(clean_lines.elided): + # No end was found. + return + parameter_end_position = Position(close_result[1], close_result[2]) + is_declaration = bool(search(r'^[^{]*;', start_line)) if is_declaration: ending_line_number = start_line_number else: open_brace_index = start_line.find('{') ending_line_number = close_expression(clean_lines, start_line_number, open_brace_index)[1] - function_state.begin(function, start_line_number, ending_line_number, is_declaration) + function_state.begin(function, start_line_number, ending_line_number, is_declaration, + parameter_start_position, parameter_end_position, clean_lines) return # No body for the function (or evidence of a non-function) was found. @@ -1288,6 +1478,64 @@ def check_for_function_lengths(clean_lines, line_number, function_state, error): function_state.count(line_number) # Count non-blank/non-comment lines. +def _check_parameter_name_against_text(parameter, text, error): + """Checks to see if the parameter name is contained within the text. + + Return false if the check failed (i.e. an error was produced). + """ + + # Treat 'lower with underscores' as a canonical form because it is + # case insensitive while still retaining word breaks. (This ensures that + # 'elate' doesn't look like it is duplicating of 'NateLate'.) + canonical_parameter_name = parameter.lower_with_underscores_name() + + # Appends "object" to all text to catch variables that did the same (but only + # do this when the parameter name is more than a single character to avoid + # flagging 'b' which may be an ok variable when used in an rgba function). + if len(canonical_parameter_name) > 1: + text = sub(r'(\w)\b', r'\1Object', text) + canonical_text = _convert_to_lower_with_underscores(text) + + # Used to detect cases like ec for ExceptionCode. + acronym = _create_acronym(text).lower() + if canonical_text.find(canonical_parameter_name) != -1 or acronym.find(canonical_parameter_name) != -1: + error(parameter.row, 'readability/parameter_name', 5, + 'The parameter name "%s" adds no information, so it should be removed.' % parameter.name) + return False + return True + + +def check_function_definition(clean_lines, line_number, function_state, error): + """Check that function definitions for style issues. + + Specifically, check that parameter names in declarations add information. + + Args: + clean_lines: A CleansedLines instance containing the file. + line_number: The number of the line to check. + function_state: Current function name and lines in body so far. + error: The function to call with any errors found. + """ + # Only do checks when we have a function declaration. + if line_number != function_state.body_start_line_number or not function_state.is_declaration: + return + + parameter_list = function_state.parameter_list() + for parameter in parameter_list: + if not parameter.name: + continue + + # Check the parameter name against the function name for single parameter set functions. + if len(parameter_list) == 1 and match('set[A-Z]', function_state.current_function): + trimmed_function_name = function_state.current_function[len('set'):] + if not _check_parameter_name_against_text(parameter, trimmed_function_name, error): + continue # Since an error was noted for this name, move to the next parameter. + + # Check the parameter name against the type. + if not _check_parameter_name_against_text(parameter, parameter.type, error): + continue # Since an error was noted for this name, move to the next parameter. + + def check_pass_ptr_usage(clean_lines, line_number, function_state, error): """Check for proper usage of Pass*Ptr. @@ -2028,6 +2276,10 @@ def check_for_null(clean_lines, line_number, file_state, error): if search(r'\bgdk_pixbuf_save_to\w+\b', line): return + # Don't warn about NULL usage in gtk_widget_style_get(). See Bug 51758. + if search(r'\bgtk_widget_style_get\(\w+\b', line): + return + if search(r'\bNULL\b', line): error(line_number, 'readability/null', 5, 'Use 0 instead of NULL.') return @@ -3001,6 +3253,7 @@ def process_line(filename, file_extension, check_for_function_lengths(clean_lines, line, function_state, error) if search(r'\bNOLINT\b', raw_lines[line]): # ignore nolint lines return + check_function_definition(clean_lines, line, function_state, error) check_pass_ptr_usage(clean_lines, line, function_state, error) check_for_multiline_comments_and_strings(clean_lines, line, error) check_style(clean_lines, line, file_extension, class_state, file_state, error) @@ -3084,6 +3337,7 @@ class CppChecker(object): 'readability/function', 'readability/multiline_comment', 'readability/multiline_string', + 'readability/parameter_name', 'readability/naming', 'readability/null', 'readability/pass_ptr', diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py index 70df1ea..d39d2ba 100644 --- a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py +++ b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py @@ -1,7 +1,7 @@ #!/usr/bin/python # -*- coding: utf-8; -*- # -# Copyright (C) 2009 Google Inc. All rights reserved. +# Copyright (C) 2009, 2010 Google Inc. All rights reserved. # Copyright (C) 2009 Torch Mobile Inc. # Copyright (C) 2009 Apple Inc. All rights reserved. # Copyright (C) 2010 Chris Jerdonek (cjerdonek@webkit.org) @@ -109,6 +109,19 @@ class CppFunctionsTest(unittest.TestCase): """Supports testing functions that do not need CppStyleTestBase.""" + def test_convert_to_lower_with_underscores(self): + self.assertEquals(cpp_style._convert_to_lower_with_underscores('ABC'), 'abc') + self.assertEquals(cpp_style._convert_to_lower_with_underscores('aB'), 'a_b') + self.assertEquals(cpp_style._convert_to_lower_with_underscores('isAName'), 'is_a_name') + self.assertEquals(cpp_style._convert_to_lower_with_underscores('AnotherTest'), 'another_test') + self.assertEquals(cpp_style._convert_to_lower_with_underscores('PassRefPtr<MyClass>'), 'pass_ref_ptr<my_class>') + self.assertEquals(cpp_style._convert_to_lower_with_underscores('_ABC'), '_abc') + + def test_create_acronym(self): + self.assertEquals(cpp_style._create_acronym('ABC'), 'ABC') + self.assertEquals(cpp_style._create_acronym('IsAName'), 'IAN') + self.assertEquals(cpp_style._create_acronym('PassRefPtr<MyClass>'), 'PRP<MC>') + def test_is_c_or_objective_c(self): clean_lines = cpp_style.CleansedLines(['']) clean_objc_lines = cpp_style.CleansedLines(['#import "header.h"']) @@ -119,6 +132,99 @@ class CppFunctionsTest(unittest.TestCase): self.assertFalse(cpp_style._FileState(clean_lines, 'h').is_c_or_objective_c()) self.assertTrue(cpp_style._FileState(clean_objc_lines, 'h').is_c_or_objective_c()) + def test_parameter(self): + # Test type. + parameter = cpp_style.Parameter('ExceptionCode', 13, 1) + self.assertEquals(parameter.type, 'ExceptionCode') + self.assertEquals(parameter.name, '') + self.assertEquals(parameter.row, 1) + + # Test type and name. + parameter = cpp_style.Parameter('PassRefPtr<MyClass> parent', 19, 1) + self.assertEquals(parameter.type, 'PassRefPtr<MyClass>') + self.assertEquals(parameter.name, 'parent') + self.assertEquals(parameter.row, 1) + + # Test type, no name, with default value. + parameter = cpp_style.Parameter('MyClass = 0', 7, 0) + self.assertEquals(parameter.type, 'MyClass') + self.assertEquals(parameter.name, '') + self.assertEquals(parameter.row, 0) + + # Test type, name, and default value. + parameter = cpp_style.Parameter('MyClass a = 0', 7, 0) + self.assertEquals(parameter.type, 'MyClass') + self.assertEquals(parameter.name, 'a') + self.assertEquals(parameter.row, 0) + + def test_single_line_view(self): + start_position = cpp_style.Position(row=1, column=1) + end_position = cpp_style.Position(row=3, column=1) + single_line_view = cpp_style.SingleLineView(['0', 'abcde', 'fgh', 'i'], start_position, end_position) + self.assertEquals(single_line_view.single_line, 'bcde fgh i') + self.assertEquals(single_line_view.convert_column_to_row(0), 1) + self.assertEquals(single_line_view.convert_column_to_row(4), 1) + self.assertEquals(single_line_view.convert_column_to_row(5), 2) + self.assertEquals(single_line_view.convert_column_to_row(8), 2) + self.assertEquals(single_line_view.convert_column_to_row(9), 3) + self.assertEquals(single_line_view.convert_column_to_row(100), 3) + + start_position = cpp_style.Position(row=0, column=3) + end_position = cpp_style.Position(row=0, column=4) + single_line_view = cpp_style.SingleLineView(['abcdef'], start_position, end_position) + self.assertEquals(single_line_view.single_line, 'd') + + def test_create_skeleton_parameters(self): + self.assertEquals(cpp_style.create_skeleton_parameters(''), '') + self.assertEquals(cpp_style.create_skeleton_parameters(' '), ' ') + self.assertEquals(cpp_style.create_skeleton_parameters('long'), 'long,') + self.assertEquals(cpp_style.create_skeleton_parameters('const unsigned long int'), ' int,') + self.assertEquals(cpp_style.create_skeleton_parameters('long int*'), ' int ,') + self.assertEquals(cpp_style.create_skeleton_parameters('PassRefPtr<Foo> a'), 'PassRefPtr a,') + self.assertEquals(cpp_style.create_skeleton_parameters( + 'ComplexTemplate<NestedTemplate1<MyClass1, MyClass2>, NestedTemplate1<MyClass1, MyClass2> > param, int second'), + 'ComplexTemplate param, int second,') + self.assertEquals(cpp_style.create_skeleton_parameters('int = 0, Namespace::Type& a'), 'int , Type a,') + # Create skeleton parameters is a bit too aggressive with function variables, but + # it allows for parsing other parameters and declarations like this are rare. + self.assertEquals(cpp_style.create_skeleton_parameters('void (*fn)(int a, int b), Namespace::Type& a'), + 'void , Type a,') + + # This doesn't look like functions declarations but the simplifications help to eliminate false positives. + self.assertEquals(cpp_style.create_skeleton_parameters('b{d}'), 'b ,') + + def test_find_parameter_name_index(self): + self.assertEquals(cpp_style.find_parameter_name_index(' int a '), 5) + self.assertEquals(cpp_style.find_parameter_name_index(' PassRefPtr '), 16) + self.assertEquals(cpp_style.find_parameter_name_index('double'), 6) + + def test_parameter_list(self): + elided_lines = ['int blah(PassRefPtr<MyClass> paramName,', + 'const Other1Class& foo,', + 'const ComplexTemplate<Class1, NestedTemplate<P1, P2> >* const * param = new ComplexTemplate<Class1, NestedTemplate<P1, P2> >(34, 42),', + 'int* myCount = 0);'] + start_position = cpp_style.Position(row=0, column=8) + end_position = cpp_style.Position(row=3, column=16) + + expected_parameters = ({'type': 'PassRefPtr<MyClass>', 'name': 'paramName', 'row': 0}, + {'type': 'const Other1Class&', 'name': 'foo', 'row': 1}, + {'type': 'const ComplexTemplate<Class1, NestedTemplate<P1, P2> >* const *', 'name': 'param', 'row': 2}, + {'type': 'int*', 'name': 'myCount', 'row': 3}) + index = 0 + for parameter in cpp_style.parameter_list(elided_lines, start_position, end_position): + expected_parameter = expected_parameters[index] + self.assertEquals(parameter.type, expected_parameter['type']) + self.assertEquals(parameter.name, expected_parameter['name']) + self.assertEquals(parameter.row, expected_parameter['row']) + index += 1 + self.assertEquals(index, len(expected_parameters)) + + def test_check_parameter_against_text(self): + error_collector = ErrorCollector(self.assert_) + parameter = cpp_style.Parameter('FooF ooF', 4, 1) + self.assertFalse(cpp_style._check_parameter_name_against_text(parameter, 'FooF', error_collector)) + self.assertEquals(error_collector.results(), + 'The parameter name "ooF" adds no information, so it should be removed. [readability/parameter_name] [5]') class CppStyleTestBase(unittest.TestCase): """Provides some useful helper functions for cpp_style tests. @@ -152,6 +258,7 @@ class CppStyleTestBase(unittest.TestCase): basic_error_rules = ('-build/header_guard', '-legal/copyright', '-readability/fn_size', + '-readability/parameter_name', '-whitespace/ending_newline') return self.perform_lint(code, filename, basic_error_rules) @@ -159,7 +266,7 @@ class CppStyleTestBase(unittest.TestCase): def perform_multi_line_lint(self, code, file_extension): basic_error_rules = ('-build/header_guard', '-legal/copyright', - '-multi_line_filter', + '-readability/parameter_name', '-whitespace/ending_newline') return self.perform_lint(code, 'test.' + file_extension, basic_error_rules) @@ -250,6 +357,16 @@ class FunctionDetectionTest(CppStyleTestBase): self.assertEquals(function_state.body_start_line_number, function_information['body_start_line_number']) self.assertEquals(function_state.ending_line_number, function_information['ending_line_number']) self.assertEquals(function_state.is_declaration, function_information['is_declaration']) + expected_parameters = function_information.get('parameter_list') + if expected_parameters: + actual_parameters = function_state.parameter_list() + self.assertEquals(len(actual_parameters), len(expected_parameters)) + for index in range(len(expected_parameters)): + actual_parameter = actual_parameters[index] + expected_parameter = expected_parameters[index] + self.assertEquals(actual_parameter.type, expected_parameter['type']) + self.assertEquals(actual_parameter.name, expected_parameter['name']) + self.assertEquals(actual_parameter.row, expected_parameter['row']) def test_basic_function_detection(self): self.perform_function_detection( @@ -268,6 +385,37 @@ class FunctionDetectionTest(CppStyleTestBase): 'ending_line_number': 0, 'is_declaration': True}) + self.perform_function_detection( + ['CheckedInt<T> operator /(const CheckedInt<T> &lhs, const CheckedInt<T> &rhs);'], + {'name': 'operator /', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True}) + + self.perform_function_detection( + ['CheckedInt<T> operator -(const CheckedInt<T> &lhs, const CheckedInt<T> &rhs);'], + {'name': 'operator -', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True}) + + self.perform_function_detection( + ['CheckedInt<T> operator !=(const CheckedInt<T> &lhs, const CheckedInt<T> &rhs);'], + {'name': 'operator !=', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True}) + + self.perform_function_detection( + ['CheckedInt<T> operator +(const CheckedInt<T> &lhs, const CheckedInt<T> &rhs);'], + {'name': 'operator +', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True}) + + def test_ignore_macros(self): + self.perform_function_detection(['void aFunctionName(int); \\'], None) + def test_non_functions(self): # This case exposed an error because the open brace was in quotes. self.perform_function_detection( @@ -284,6 +432,70 @@ class FunctionDetectionTest(CppStyleTestBase): # Simple test case with something that is not a function. self.perform_function_detection(['class Stuff;'], None) + def test_parameter_list(self): + # A function with no arguments. + function_state = self.perform_function_detection( + ['void functionName();'], + {'name': 'functionName', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True, + 'parameter_list': ()}) + + # A function with one argument. + function_state = self.perform_function_detection( + ['void functionName(int);'], + {'name': 'functionName', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True, + 'parameter_list': + ({'type': 'int', 'name': '', 'row': 0},)}) + + # A function with unsigned and short arguments + function_state = self.perform_function_detection( + ['void functionName(unsigned a, short b, long c, long long short unsigned int);'], + {'name': 'functionName', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True, + 'parameter_list': + ({'type': 'unsigned', 'name': 'a', 'row': 0}, + {'type': 'short', 'name': 'b', 'row': 0}, + {'type': 'long', 'name': 'c', 'row': 0}, + {'type': 'long long short unsigned int', 'name': '', 'row': 0})}) + + # Some parameter type with modifiers and no parameter names. + function_state = self.perform_function_detection( + ['virtual void determineARIADropEffects(Vector<String>*&, const unsigned long int*&, const MediaPlayer::Preload, Other<Other2, Other3<P1, P2> >, int);'], + {'name': 'determineARIADropEffects', + 'body_start_line_number': 0, + 'ending_line_number': 0, + 'is_declaration': True, + 'parameter_list': + ({'type': 'Vector<String>*&', 'name': '', 'row': 0}, + {'type': 'const unsigned long int*&', 'name': '', 'row': 0}, + {'type': 'const MediaPlayer::Preload', 'name': '', 'row': 0}, + {'type': 'Other<Other2, Other3<P1, P2> >', 'name': '', 'row': 0}, + {'type': 'int', 'name': '', 'row': 0})}) + + # Try parsing a function with a very complex definition. + function_state = self.perform_function_detection( + ['AnotherTemplate<Class1, Class2> aFunctionName(PassRefPtr<MyClass> paramName,', + 'const Other1Class& foo,', + 'const ComplexTemplate<Class1, NestedTemplate<P1, P2> >* const * param = new ComplexTemplate<Class1, NestedTemplate<P1, P2> >(34, 42),', + 'int* myCount = 0);'], + {'name': 'aFunctionName', + 'body_start_line_number': 3, + 'ending_line_number': 3, + 'is_declaration': True, + 'parameter_list': + ({'type': 'PassRefPtr<MyClass>', 'name': 'paramName', 'row': 0}, + {'type': 'const Other1Class&', 'name': 'foo', 'row': 1}, + {'type': 'const ComplexTemplate<Class1, NestedTemplate<P1, P2> >* const *', 'name': 'param', 'row': 2}, + {'type': 'int*', 'name': 'myCount', 'row': 3})}) + + class CppStyleTest(CppStyleTestBase): # Test get line width. @@ -1223,6 +1435,8 @@ class CppStyleTest(CppStyleTestBase): self.assert_lint('((a+b))', '') self.assert_lint('foo (foo)', 'Extra space before ( in function call' ' [whitespace/parens] [4]') + self.assert_lint('#elif (foo(bar))', '') + self.assert_lint('#elif (foo(bar) && foo(baz))', '') self.assert_lint('typedef foo (*foo)(foo)', '') self.assert_lint('typedef foo (*foo12bar_)(foo)', '') self.assert_lint('typedef foo (Foo::*bar)(foo)', '') @@ -1797,7 +2011,7 @@ class CppStyleTest(CppStyleTestBase): # Allow the WTF_ prefix for files in that directory. header_guard_filter = FilterConfiguration(('-', '+build/header_guard')) error_collector = ErrorCollector(self.assert_, header_guard_filter) - self.process_file_data('JavaScriptCore/wtf/TestName.h', 'h', + self.process_file_data('Source/JavaScriptCore/wtf/TestName.h', 'h', ['#ifndef WTF_TestName_h', '#define WTF_TestName_h'], error_collector) self.assertEquals(0, len(error_collector.result_list()), @@ -1805,7 +2019,7 @@ class CppStyleTest(CppStyleTestBase): # Also allow the non WTF_ prefix for files in that directory. error_collector = ErrorCollector(self.assert_, header_guard_filter) - self.process_file_data('JavaScriptCore/wtf/TestName.h', 'h', + self.process_file_data('Source/JavaScriptCore/wtf/TestName.h', 'h', ['#ifndef TestName_h', '#define TestName_h'], error_collector) self.assertEquals(0, len(error_collector.result_list()), @@ -1813,7 +2027,7 @@ class CppStyleTest(CppStyleTestBase): # Verify that we suggest the WTF prefix version. error_collector = ErrorCollector(self.assert_, header_guard_filter) - self.process_file_data('JavaScriptCore/wtf/TestName.h', 'h', + self.process_file_data('Source/JavaScriptCore/wtf/TestName.h', 'h', ['#ifndef BAD_TestName_h', '#define BAD_TestName_h'], error_collector) self.assertEquals( @@ -2549,14 +2763,12 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): error_level = 1 error_lines = self.trigger_test_lines(error_level) + 1 trigger_level = self.trigger_test_lines(self.min_confidence) + # Since the function name isn't valid, the function detection algorithm + # will skip it, so no error is produced. self.assert_function_lengths_check( ('TEST_F(' + self.function_body(error_lines)), - ('Small and focused functions are preferred: ' - 'TEST_F has %d non-comment lines ' - '(error triggered by exceeding %d lines).' - ' [readability/fn_size] [%d]') - % (error_lines, trigger_level, error_level)) + '') def test_function_length_check_definition_severity1_with_embedded_no_lints(self): error_level = 1 @@ -3648,6 +3860,17 @@ class WebKitStyleTest(CppStyleTestBase): self.assert_lint( 'gchar* result = gdk_pixbuf_save_to_stream(pixbuf, function, data, type, error, NULL);', '') + self.assert_lint( + 'gtk_widget_style_get(style, "propertyName", &value, "otherName", &otherValue, NULL);', + '') + self.assert_lint( + 'gtk_widget_style_get_property(style, NULL, NULL);', + 'Use 0 instead of NULL. [readability/null] [5]', + 'foo.cpp') + self.assert_lint( + 'gtk_widget_style_get_valist(style, NULL, NULL);', + 'Use 0 instead of NULL. [readability/null] [5]', + 'foo.cpp') # 2. C++ and C bool values should be written as true and # false. Objective-C BOOL values should be written as YES and NO. @@ -3867,8 +4090,8 @@ class WebKitStyleTest(CppStyleTestBase): 'variable_2' + name_underscore_error_message]) # There is an exception for op code functions but only in the JavaScriptCore directory. - self.assert_lint('void this_op_code(int var1, int var2)', '', 'JavaScriptCore/foo.cpp') - self.assert_lint('void op_code(int var1, int var2)', '', 'JavaScriptCore/foo.cpp') + self.assert_lint('void this_op_code(int var1, int var2)', '', 'Source/JavaScriptCore/foo.cpp') + self.assert_lint('void op_code(int var1, int var2)', '', 'Source/JavaScriptCore/foo.cpp') self.assert_lint('void this_op_code(int var1, int var2)', 'this_op_code' + name_underscore_error_message) # GObject requires certain magical names in class declarations. @@ -3908,6 +4131,55 @@ class WebKitStyleTest(CppStyleTestBase): self.assert_lint('OwnPtr<uint32_t> under_score(new uint32_t);', 'under_score' + name_underscore_error_message) + def test_parameter_names(self): + # Leave meaningless variable names out of function declarations. + meaningless_variable_name_error_message = 'The parameter name "%s" adds no information, so it should be removed. [readability/parameter_name] [5]' + + parameter_error_rules = ('-', + '+readability/parameter_name') + # No variable name, so no error. + self.assertEquals('', + self.perform_lint('void func(int);', 'test.cpp', parameter_error_rules)) + + # Verify that copying the name of the set function causes the error (with some odd casing). + self.assertEquals(meaningless_variable_name_error_message % 'itemCount', + self.perform_lint('void setItemCount(size_t itemCount);', 'test.cpp', parameter_error_rules)) + self.assertEquals(meaningless_variable_name_error_message % 'abcCount', + self.perform_lint('void setABCCount(size_t abcCount);', 'test.cpp', parameter_error_rules)) + + # Verify that copying a type name will trigger the warning (even if the type is a template parameter). + self.assertEquals(meaningless_variable_name_error_message % 'context', + self.perform_lint('void funct(PassRefPtr<ScriptExecutionContext> context);', 'test.cpp', parameter_error_rules)) + + # Verify that acronyms as variable names trigger the error (for both set functions and type names). + self.assertEquals(meaningless_variable_name_error_message % 'ec', + self.perform_lint('void setExceptionCode(int ec);', 'test.cpp', parameter_error_rules)) + self.assertEquals(meaningless_variable_name_error_message % 'ec', + self.perform_lint('void funct(ExceptionCode ec);', 'test.cpp', parameter_error_rules)) + + # 'object' alone, appended, or as part of an acronym is meaningless. + self.assertEquals(meaningless_variable_name_error_message % 'object', + self.perform_lint('void funct(RenderView object);', 'test.cpp', parameter_error_rules)) + self.assertEquals(meaningless_variable_name_error_message % 'viewObject', + self.perform_lint('void funct(RenderView viewObject);', 'test.cpp', parameter_error_rules)) + self.assertEquals(meaningless_variable_name_error_message % 'rvo', + self.perform_lint('void funct(RenderView rvo);', 'test.cpp', parameter_error_rules)) + + # Check that r, g, b, and a are allowed. + self.assertEquals('', + self.perform_lint('void setRGBAValues(int r, int g, int b, int a);', 'test.cpp', parameter_error_rules)) + + # Verify that a simple substring match isn't done which would cause false positives. + self.assertEquals('', + self.perform_lint('void setNateLateCount(size_t elate);', 'test.cpp', parameter_error_rules)) + self.assertEquals('', + self.perform_lint('void funct(NateLate elate);', 'test.cpp', parameter_error_rules)) + + # Don't have generate warnings for functions (only declarations). + self.assertEquals('', + self.perform_lint('void funct(PassRefPtr<ScriptExecutionContext> context)\n' + '{\n' + '}\n', 'test.cpp', parameter_error_rules)) def test_comments(self): # A comment at the beginning of a line is ok. diff --git a/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py b/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py index 1d82ea8..4bdc79b 100644 --- a/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py +++ b/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py @@ -149,11 +149,11 @@ class CommitQueueTask(object): "Able to pass tests without patch", "Unable to pass tests without patch (tree is red?)") - def _failing_tests_from_last_run(self): + def _failing_results_from_last_run(self): results = self._delegate.layout_test_results() if not results: - return None - return results.failing_tests() + return [] # Makes callers slighty cleaner to not have to deal with None + return results.failing_test_results() def _land(self): # Unclear if this should pass --quiet or not. If --parent-command always does the reporting, then it should. @@ -168,8 +168,8 @@ class CommitQueueTask(object): "Landed patch", "Unable to land patch") - def _report_flaky_tests(self, flaky_tests): - self._delegate.report_flaky_tests(self._patch, flaky_tests) + def _report_flaky_tests(self, flaky_test_results): + self._delegate.report_flaky_tests(self._patch, flaky_test_results) def _test_patch(self): if self._patch.is_rollout(): @@ -177,12 +177,14 @@ class CommitQueueTask(object): if self._test(): return True - first_failing_tests = self._failing_tests_from_last_run() + first_failing_results = self._failing_results_from_last_run() + first_failing_tests = [result.filename for result in first_failing_results] if self._test(): - self._report_flaky_tests(first_failing_tests) + self._report_flaky_tests(first_failing_results) return True - second_failing_tests = self._failing_tests_from_last_run() + second_failing_results = self._failing_results_from_last_run() + second_failing_tests = [result.filename for result in second_failing_results] if first_failing_tests != second_failing_tests: # We could report flaky tests here, but since run-webkit-tests # is run with --exit-after-N-failures=1, we would need to @@ -192,9 +194,14 @@ class CommitQueueTask(object): return False if self._build_and_test_without_patch(): - raise self._script_error # The error from the previous ._test() run is real, report it. + return self.report_failure() # The error from the previous ._test() run is real, report it. return False # Tree must be red, just retry later. + def report_failure(self): + if not self._validate(): + return False + raise self._script_error + def run(self): if not self._validate(): return False @@ -203,11 +210,11 @@ class CommitQueueTask(object): if not self._update(): return False if not self._apply(): - raise self._script_error + return self.report_failure() if not self._build(): if not self._build_without_patch(): return False - raise self._script_error + return self.report_failure() if not self._test_patch(): return False # Make sure the patch is still valid before landing (e.g., make sure @@ -216,5 +223,5 @@ class CommitQueueTask(object): return False # FIXME: We should understand why the land failure occured and retry if possible. if not self._land(): - raise self._script_error + return self.report_failure() return True diff --git a/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py b/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py index 376f407..f279cac 100644 --- a/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py +++ b/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py @@ -31,6 +31,8 @@ import unittest from webkitpy.common.system.deprecated_logging import error, log from webkitpy.common.system.outputcapture import OutputCapture +from webkitpy.layout_tests.layout_package import test_results +from webkitpy.layout_tests.layout_package import test_failures from webkitpy.thirdparty.mock import Mock from webkitpy.tool.bot.commitqueuetask import * from webkitpy.tool.mocktool import MockTool @@ -62,11 +64,15 @@ class MockCommitQueue(CommitQueueTaskDelegate): def layout_test_results(self): return None - def report_flaky_tests(self, patch, flaky_tests): + def report_flaky_tests(self, patch, flaky_results): + flaky_tests = [result.filename for result in flaky_results] log("report_flaky_tests: patch='%s' flaky_tests='%s'" % (patch.id(), flaky_tests)) class CommitQueueTaskTest(unittest.TestCase): + def _mock_test_result(self, testname): + return test_results.TestResult(testname, [test_failures.FailureTextMismatch()]) + def _run_through_task(self, commit_queue, expected_stderr, expected_exception=None, expect_retry=False): tool = MockTool(log_executive=True) patch = tool.bugs.fetch_attachment(197) @@ -189,7 +195,7 @@ run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--n 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', '--non-interactive'] command_passed: success_message='Passed tests' patch='197' -report_flaky_tests: patch='197' flaky_tests='None' +report_flaky_tests: patch='197' flaky_tests='[]' run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197] command_passed: success_message='Landed patch' patch='197' """ @@ -227,13 +233,13 @@ command_failed: failure_message='Patch does not pass tests' script_error='MOCK t task = CommitQueueTask(commit_queue, patch) self._double_flaky_test_counter = 0 - def mock_failing_tests_from_last_run(): + def mock_failing_results_from_last_run(): CommitQueueTaskTest._double_flaky_test_counter += 1 if CommitQueueTaskTest._double_flaky_test_counter % 2: - return ['foo.html'] - return ['bar.html'] + return [self._mock_test_result('foo.html')] + return [self._mock_test_result('bar.html')] - task._failing_tests_from_last_run = mock_failing_tests_from_last_run + task._failing_results_from_last_run = mock_failing_results_from_last_run success = OutputCapture().assert_outputs(self, task.run, expected_stderr=expected_stderr) self.assertEqual(success, False) diff --git a/Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py b/Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py index 01cbf39..91fcb85 100644 --- a/Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py +++ b/Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py @@ -102,7 +102,9 @@ class FlakyTestReporter(object): The bots will update this with information from each new failure. -If you would like to track this test fix with another bug, please close this bug as a duplicate. +If you believe this bug to be fixed or invalid, feel free to close. The bots will re-open if the flake re-occurs. + +If you would like to track this test fix with another bug, please close this bug as a duplicate. The bots will follow the duplicate chain when making future comments. """ % format_values master_flake_bug = 50856 # MASTER: Flaky tests found by the commit-queue @@ -124,8 +126,9 @@ If you would like to track this test fix with another bug, please close this bug bot_id_string = "Bot: %s " % (bot_id) if bot_id else "" return "%sPort: %s Platform: %s" % (bot_id_string, self._tool.port().name(), self._tool.platform.display_name()) - def _latest_flake_message(self, flaky_test, patch): - flake_message = "The %s just saw %s flake while processing attachment %s on bug %s." % (self._bot_name, flaky_test, patch.id(), patch.bug_id()) + def _latest_flake_message(self, flaky_result, patch): + failure_messages = [failure.message() for failure in flaky_result.failures] + flake_message = "The %s just saw %s flake (%s) while processing attachment %s on bug %s." % (self._bot_name, flaky_result.filename, ", ".join(failure_messages), patch.id(), patch.bug_id()) return "%s\n%s" % (flake_message, self._bot_information()) def _results_diff_path_for_test(self, flaky_test): @@ -150,11 +153,12 @@ If you would like to track this test fix with another bug, please close this bug else: self._tool.bugs.post_comment_to_bug(bug.id(), latest_flake_message) - def report_flaky_tests(self, flaky_tests, patch): + def report_flaky_tests(self, flaky_test_results, patch): message = "The %s encountered the following flaky tests while processing attachment %s:\n\n" % (self._bot_name, patch.id()) - for flaky_test in flaky_tests: + for flaky_result in flaky_test_results: + flaky_test = flaky_result.filename bug = self._lookup_bug_for_flaky_test(flaky_test) - latest_flake_message = self._latest_flake_message(flaky_test, patch) + latest_flake_message = self._latest_flake_message(flaky_result, patch) author_emails = self._author_emails_for_test(flaky_test) if not bug: _log.info("Bug does not already exist for %s, creating." % flaky_test) diff --git a/Tools/Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py b/Tools/Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py index f72fb28..631f8d1 100644 --- a/Tools/Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py +++ b/Tools/Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py @@ -31,6 +31,8 @@ import unittest from webkitpy.common.config.committers import Committer from webkitpy.common.system.filesystem_mock import MockFileSystem from webkitpy.common.system.outputcapture import OutputCapture +from webkitpy.layout_tests.layout_package import test_results +from webkitpy.layout_tests.layout_package import test_failures from webkitpy.tool.bot.flakytestreporter import FlakyTestReporter from webkitpy.tool.mocktool import MockTool, MockStatusServer @@ -49,6 +51,9 @@ class MockCommitInfo(object): class FlakyTestReporterTest(unittest.TestCase): + def _mock_test_result(self, testname): + return test_results.TestResult(testname, [test_failures.FailureTextMismatch()]) + def _assert_emails_for_test(self, emails): tool = MockTool() reporter = FlakyTestReporter(tool, 'dummy-queue') @@ -75,7 +80,9 @@ FLAKE_MESSAGE The bots will update this with information from each new failure. -If you would like to track this test fix with another bug, please close this bug as a duplicate. +If you believe this bug to be fixed or invalid, feel free to close. The bots will re-open if the flake re-occurs. + +If you would like to track this test fix with another bug, please close this bug as a duplicate. The bots will follow the duplicate chain when making future comments. component: Tools / Tests cc: test@test.com @@ -110,12 +117,14 @@ foo/bar.html has been flaky on the dummy-queue. foo/bar.html was authored by abarth@webkit.org. http://trac.webkit.org/browser/trunk/LayoutTests/foo/bar.html -The dummy-queue just saw foo/bar.html flake while processing attachment 197 on bug 42. +The dummy-queue just saw foo/bar.html flake (Text diff mismatch) while processing attachment 197 on bug 42. Bot: mock-bot-id Port: MockPort Platform: MockPlatform 1.0 The bots will update this with information from each new failure. -If you would like to track this test fix with another bug, please close this bug as a duplicate. +If you believe this bug to be fixed or invalid, feel free to close. The bots will re-open if the flake re-occurs. + +If you would like to track this test fix with another bug, please close this bug as a duplicate. The bots will follow the duplicate chain when making future comments. component: Tools / Tests cc: abarth@webkit.org @@ -130,7 +139,8 @@ The dummy-queue is continuing to process your patch. --- End comment --- """ - OutputCapture().assert_outputs(self, reporter.report_flaky_tests, [['foo/bar.html'], patch], expected_stderr=expected_stderr) + test_results = [self._mock_test_result('foo/bar.html')] + OutputCapture().assert_outputs(self, reporter.report_flaky_tests, [test_results, patch], expected_stderr=expected_stderr) def test_optional_author_string(self): reporter = FlakyTestReporter(MockTool(), 'dummy-queue') diff --git a/Tools/Scripts/webkitpy/tool/bot/irc_command.py b/Tools/Scripts/webkitpy/tool/bot/irc_command.py index 0c17c9f..265974e 100644 --- a/Tools/Scripts/webkitpy/tool/bot/irc_command.py +++ b/Tools/Scripts/webkitpy/tool/bot/irc_command.py @@ -30,9 +30,10 @@ import random import webkitpy.common.config.irc as config_irc from webkitpy.common.config import urls -from webkitpy.tool.bot.queueengine import TerminateQueue from webkitpy.common.net.bugzilla import parse_bug_id from webkitpy.common.system.executive import ScriptError +from webkitpy.tool.bot.queueengine import TerminateQueue +from webkitpy.tool.grammar import join_with_separators # FIXME: Merge with Command? class IRCCommand(object): @@ -53,17 +54,38 @@ class Restart(IRCCommand): class Rollout(IRCCommand): + def _parse_args(self, args): + read_revision = True + rollout_reason = [] + # the first argument must be a revision number + svn_revision_list = [args[0].lstrip("r")] + if not svn_revision_list[0].isdigit(): + read_revision = False + + for arg in args[1:]: + if arg.lstrip("r").isdigit() and read_revision: + svn_revision_list.append(arg.lstrip("r")) + else: + read_revision = False + rollout_reason.append(arg) + + return svn_revision_list, rollout_reason + def execute(self, nick, args, tool, sheriff): - if len(args) < 2: - tool.irc().post("%s: Usage: SVN_REVISION REASON" % nick) + svn_revision_list, rollout_reason = self._parse_args(args) + + if (len(svn_revision_list) == 0) or (len(rollout_reason) == 0): + tool.irc().post("%s: Usage: SVN_REVISION [SVN_REVISIONS] REASON" % nick) return - svn_revision = args[0].lstrip("r") - rollout_reason = " ".join(args[1:]) - tool.irc().post("Preparing rollout for r%s..." % svn_revision) + + rollout_reason = " ".join(rollout_reason) + + tool.irc().post("Preparing rollout for %s..." % + join_with_separators(["r" + str(revision) for revision in svn_revision_list])) try: complete_reason = "%s (Requested by %s on %s)." % ( rollout_reason, nick, config_irc.channel) - bug_id = sheriff.post_rollout_patch(svn_revision, complete_reason) + bug_id = sheriff.post_rollout_patch(svn_revision_list, complete_reason) bug_url = tool.bugs.bug_url_for_bug_id(bug_id) tool.irc().post("%s: Created rollout: %s" % (nick, bug_url)) except ScriptError, e: diff --git a/Tools/Scripts/webkitpy/tool/bot/sheriff.py b/Tools/Scripts/webkitpy/tool/bot/sheriff.py index 43f3221..a5edceb 100644 --- a/Tools/Scripts/webkitpy/tool/bot/sheriff.py +++ b/Tools/Scripts/webkitpy/tool/bot/sheriff.py @@ -51,14 +51,14 @@ class Sheriff(object): self._tool.irc().post(irc_message) - def post_rollout_patch(self, svn_revision, rollout_reason): - # Ensure that svn_revision is a number (and not an option to + def post_rollout_patch(self, svn_revision_list, rollout_reason): + # Ensure that svn revisions are numbers (and not options to # create-rollout). try: - svn_revision = int(svn_revision) + svn_revisions = " ".join([str(int(revision)) for revision in svn_revision_list]) except: raise ScriptError(message="Invalid svn revision number \"%s\"." - % svn_revision) + % " ".join(svn_revision_list)) if rollout_reason.startswith("-"): raise ScriptError(message="The rollout reason may not begin " @@ -72,7 +72,7 @@ class Sheriff(object): # pass it prophylactically because we reject unrecognized command # line switches. "--parent-command=sheriff-bot", - svn_revision, + svn_revisions, rollout_reason, ]) return parse_bug_id(output) diff --git a/Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py b/Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py index 08023bd..ccb8ac2 100644 --- a/Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py +++ b/Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py @@ -62,12 +62,20 @@ class SheriffIRCBotTest(unittest.TestCase): expected_stderr = "MOCK: irc.post: Preparing rollout for r21654...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n" OutputCapture().assert_outputs(self, run, args=["rollout 21654 This patch broke the world"], expected_stderr=expected_stderr) + def test_multi_rollout(self): + expected_stderr = "MOCK: irc.post: Preparing rollout for r21654, r21655, and r21656...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n" + OutputCapture().assert_outputs(self, run, args=["rollout 21654 21655 21656 This 21654 patch broke the world"], expected_stderr=expected_stderr) + def test_rollout_with_r_in_svn_revision(self): expected_stderr = "MOCK: irc.post: Preparing rollout for r21654...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n" OutputCapture().assert_outputs(self, run, args=["rollout r21654 This patch broke the world"], expected_stderr=expected_stderr) + def test_multi_rollout_with_r_in_svn_revision(self): + expected_stderr = "MOCK: irc.post: Preparing rollout for r21654, r21655, and r21656...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n" + OutputCapture().assert_outputs(self, run, args=["rollout r21654 21655 r21656 This r21654 patch broke the world"], expected_stderr=expected_stderr) + def test_rollout_bananas(self): - expected_stderr = "MOCK: irc.post: mock_nick: Usage: SVN_REVISION REASON\n" + expected_stderr = "MOCK: irc.post: mock_nick: Usage: SVN_REVISION [SVN_REVISIONS] REASON\n" OutputCapture().assert_outputs(self, run, args=["rollout bananas"], expected_stderr=expected_stderr) def test_rollout_invalidate_revision(self): @@ -90,6 +98,21 @@ class SheriffIRCBotTest(unittest.TestCase): "21654 -bad"], expected_stderr=expected_stderr) + def test_multi_rollout_invalidate_reason(self): + expected_stderr = ("MOCK: irc.post: Preparing rollout for " + "r21654, r21655, and r21656...\nMOCK: irc.post: mock_nick: Failed to " + "create rollout patch:\nMOCK: irc.post: The rollout" + " reason may not begin with - (\"-bad (Requested " + "by mock_nick on #webkit).\").\n") + OutputCapture().assert_outputs(self, run, + args=["rollout " + "21654 21655 r21656 -bad"], + expected_stderr=expected_stderr) + def test_rollout_no_reason(self): - expected_stderr = "MOCK: irc.post: mock_nick: Usage: SVN_REVISION REASON\n" + expected_stderr = "MOCK: irc.post: mock_nick: Usage: SVN_REVISION [SVN_REVISIONS] REASON\n" OutputCapture().assert_outputs(self, run, args=["rollout 21654"], expected_stderr=expected_stderr) + + def test_multi_rollout_no_reason(self): + expected_stderr = "MOCK: irc.post: mock_nick: Usage: SVN_REVISION [SVN_REVISIONS] REASON\n" + OutputCapture().assert_outputs(self, run, args=["rollout 21654 21655 r21656"], expected_stderr=expected_stderr) diff --git a/Tools/Scripts/webkitpy/tool/commands/download.py b/Tools/Scripts/webkitpy/tool/commands/download.py index 020f339..1b478bf 100644 --- a/Tools/Scripts/webkitpy/tool/commands/download.py +++ b/Tools/Scripts/webkitpy/tool/commands/download.py @@ -95,6 +95,7 @@ class Land(AbstractSequencedCommand): steps.EnsureBuildersAreGreen, steps.UpdateChangeLogsWithReviewer, steps.ValidateReviewer, + steps.ValidateChangeLogs, # We do this after UpdateChangeLogsWithReviewer to avoid not having to cache the diff twice. steps.Build, steps.RunTests, steps.Commit, @@ -257,6 +258,7 @@ class AbstractPatchLandingCommand(AbstractPatchSequencingCommand): steps.CleanWorkingDirectory, steps.Update, steps.ApplyPatch, + steps.ValidateChangeLogs, steps.ValidateReviewer, steps.Build, steps.RunTests, diff --git a/Tools/Scripts/webkitpy/tool/commands/download_unittest.py b/Tools/Scripts/webkitpy/tool/commands/download_unittest.py index 3748a8f..ba23ab9 100644 --- a/Tools/Scripts/webkitpy/tool/commands/download_unittest.py +++ b/Tools/Scripts/webkitpy/tool/commands/download_unittest.py @@ -109,11 +109,11 @@ class DownloadCommandsTest(CommandsTest): def test_land_diff(self): expected_stderr = "Building WebKit\nRunning Python unit tests\nRunning Perl unit tests\nRunning JavaScriptCore tests\nRunning run-webkit-tests\nCommitted r49824: <http://trac.webkit.org/changeset/49824>\nUpdating bug 42\n" mock_tool = MockTool() - mock_tool.scm().create_patch = Mock() + mock_tool.scm().create_patch = Mock(return_value="Patch1\nMockPatch\n") mock_tool.checkout().modified_changelogs = Mock(return_value=[]) self.assert_execute_outputs(Land(), [42], options=self._default_options(), expected_stderr=expected_stderr, tool=mock_tool) # Make sure we're not calling expensive calls too often. - self.assertEqual(mock_tool.scm().create_patch.call_count, 0) + self.assertEqual(mock_tool.scm().create_patch.call_count, 1) self.assertEqual(mock_tool.checkout().modified_changelogs.call_count, 1) def test_land_red_builders(self): diff --git a/Tools/Scripts/webkitpy/tool/commands/queries.py b/Tools/Scripts/webkitpy/tool/commands/queries.py index f04f384..733751e 100644 --- a/Tools/Scripts/webkitpy/tool/commands/queries.py +++ b/Tools/Scripts/webkitpy/tool/commands/queries.py @@ -272,7 +272,7 @@ class FailureReason(AbstractDeclarativeCommand): print "%s failing" % (pluralize("builder", len(red_statuses))) builder_choices = [status["name"] for status in red_statuses] # We could offer an "All" choice here. - chosen_name = User.prompt_with_list("Which builder to diagnose:", builder_choices) + chosen_name = self._tool.user.prompt_with_list("Which builder to diagnose:", builder_choices) # FIXME: prompt_with_list should really take a set of objects and a set of names and then return the object. for status in red_statuses: if status["name"] == chosen_name: @@ -345,7 +345,7 @@ class FindFlakyTests(AbstractDeclarativeCommand): def _builder_to_analyze(self): statuses = self._tool.buildbot.builder_statuses() choices = [status["name"] for status in statuses] - chosen_name = User.prompt_with_list("Which builder to analyze:", choices) + chosen_name = self._tool.user.prompt_with_list("Which builder to analyze:", choices) for status in statuses: if status["name"] == chosen_name: return (self._tool.buildbot.builder_with_name(chosen_name), status["built_revision"]) diff --git a/Tools/Scripts/webkitpy/tool/commands/queues.py b/Tools/Scripts/webkitpy/tool/commands/queues.py index e15555f..5628543 100644 --- a/Tools/Scripts/webkitpy/tool/commands/queues.py +++ b/Tools/Scripts/webkitpy/tool/commands/queues.py @@ -96,7 +96,7 @@ class AbstractQueue(Command, QueueEngineDelegate): return self._tool.executive.run_and_throw_if_fail(webkit_patch_args) def _log_directory(self): - return "%s-logs" % self.name + return os.path.join("..", "%s-logs" % self.name) # QueueEngineDelegate methods @@ -312,9 +312,9 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD def refetch_patch(self, patch): return self._tool.bugs.fetch_attachment(patch.id()) - def report_flaky_tests(self, patch, flaky_tests): + def report_flaky_tests(self, patch, flaky_test_results): reporter = FlakyTestReporter(self._tool, self.name) - reporter.report_flaky_tests(flaky_tests, patch) + reporter.report_flaky_tests(flaky_test_results, patch) # StepSequenceErrorHandler methods diff --git a/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py b/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py index d793213..34a6a64 100644 --- a/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py +++ b/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py @@ -31,6 +31,8 @@ import os from webkitpy.common.checkout.scm import CheckoutNeedsUpdate from webkitpy.common.net.bugzilla import Attachment from webkitpy.common.system.outputcapture import OutputCapture +from webkitpy.layout_tests.layout_package import test_results +from webkitpy.layout_tests.layout_package import test_failures from webkitpy.thirdparty.mock import Mock from webkitpy.tool.commands.commandtest import CommandsTest from webkitpy.tool.commands.queues import * @@ -53,7 +55,7 @@ class TestFeederQueue(FeederQueue): class AbstractQueueTest(CommandsTest): def test_log_directory(self): - self.assertEquals(TestQueue()._log_directory(), "test-queue-logs") + self.assertEquals(TestQueue()._log_directory(), os.path.join("..", "test-queue-logs")) def _assert_run_webkit_patch(self, run_args, port=None): queue = TestQueue() @@ -198,6 +200,9 @@ class SecondThoughtsCommitQueue(CommitQueue): class CommitQueueTest(QueuesTest): + def _mock_test_result(self, testname): + return test_results.TestResult(testname, [test_failures.FailureTextMismatch()]) + def test_commit_queue(self): expected_stderr = { "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root), @@ -333,13 +338,13 @@ MOCK: release_work_item: commit-queue 197 queue.bind_to_tool(MockTool()) expected_stderr = """MOCK bug comment: bug_id=76, cc=None --- Begin comment --- -The commit-queue just saw foo/bar.html flake while processing attachment 197 on bug 42. +The commit-queue just saw foo/bar.html flake (Text diff mismatch) while processing attachment 197 on bug 42. Port: MockPort Platform: MockPlatform 1.0 --- End comment --- MOCK bug comment: bug_id=76, cc=None --- Begin comment --- -The commit-queue just saw bar/baz.html flake while processing attachment 197 on bug 42. +The commit-queue just saw bar/baz.html flake (Text diff mismatch) while processing attachment 197 on bug 42. Port: MockPort Platform: MockPlatform 1.0 --- End comment --- @@ -353,7 +358,9 @@ The commit-queue is continuing to process your patch. --- End comment --- """ - OutputCapture().assert_outputs(self, queue.report_flaky_tests, [QueuesTest.mock_work_item, ["foo/bar.html", "bar/baz.html"]], expected_stderr=expected_stderr) + test_names = ["foo/bar.html", "bar/baz.html"] + test_results = [self._mock_test_result(name) for name in test_names] + OutputCapture().assert_outputs(self, queue.report_flaky_tests, [QueuesTest.mock_work_item, test_results], expected_stderr=expected_stderr) def test_layout_test_results(self): queue = CommitQueue() diff --git a/Tools/Scripts/webkitpy/tool/commands/rebaseline.py b/Tools/Scripts/webkitpy/tool/commands/rebaseline.py index 8c4b997..34a398a 100644 --- a/Tools/Scripts/webkitpy/tool/commands/rebaseline.py +++ b/Tools/Scripts/webkitpy/tool/commands/rebaseline.py @@ -34,6 +34,7 @@ import urllib from webkitpy.common.net.buildbot import BuildBot from webkitpy.common.net.layouttestresults import LayoutTestResults from webkitpy.common.system.user import User +from webkitpy.layout_tests.layout_package import test_failures from webkitpy.layout_tests.port import factory from webkitpy.tool.grammar import pluralize from webkitpy.tool.multicommandtool import AbstractDeclarativeCommand @@ -88,7 +89,7 @@ class Rebaseline(AbstractDeclarativeCommand): shutil.move(downloaded_file, local_file) def _tests_to_update(self, build): - failing_tests = build.layout_test_results().results_matching_keys([LayoutTestResults.fail_key]) + failing_tests = build.layout_test_results().tests_matching_failure_types([test_failures.FailureTextMismatch]) return self._tool.user.prompt_with_list("Which test(s) to rebaseline:", failing_tests, can_choose_multiple=True) def _results_url_for_test(self, build, test): diff --git a/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py b/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py index d6582a7..79e4cf4 100644 --- a/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py +++ b/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py @@ -28,7 +28,19 @@ import unittest -from webkitpy.tool.commands.rebaseline import BuilderToPort +from webkitpy.common.system.outputcapture import OutputCapture +from webkitpy.thirdparty.mock import Mock +from webkitpy.tool.commands.rebaseline import BuilderToPort, Rebaseline +from webkitpy.tool.mocktool import MockTool + + +class RebaselineTest(unittest.TestCase): + # This just makes sure the code runs without exceptions. + def test_tests_to_update(self): + command = Rebaseline() + command.bind_to_tool(MockTool()) + build = Mock() + OutputCapture().assert_outputs(self, command._tests_to_update, [build]) class BuilderToPortTest(unittest.TestCase): diff --git a/Tools/Scripts/webkitpy/tool/commands/upload.py b/Tools/Scripts/webkitpy/tool/commands/upload.py index e12c8e2..6617b4f 100644 --- a/Tools/Scripts/webkitpy/tool/commands/upload.py +++ b/Tools/Scripts/webkitpy/tool/commands/upload.py @@ -196,6 +196,7 @@ class Post(AbstractPatchUploadingCommand): help_text = "Attach the current working directory diff to a bug as a patch file" argument_names = "[BUGID]" steps = [ + steps.ValidateChangeLogs, steps.CheckStyle, steps.ConfirmDiff, steps.ObsoletePatches, @@ -215,6 +216,7 @@ class LandSafely(AbstractPatchUploadingCommand): show_in_main_help = True steps = [ steps.UpdateChangeLogsWithReviewer, + steps.ValidateChangeLogs, steps.ObsoletePatches, steps.PostDiffForCommit, ] @@ -241,6 +243,7 @@ class Upload(AbstractPatchUploadingCommand): argument_names = "[BUGID]" show_in_main_help = True steps = [ + steps.ValidateChangeLogs, steps.CheckStyle, steps.PromptForBugOrTitle, steps.CreateBug, diff --git a/Tools/Scripts/webkitpy/tool/mocktool.py b/Tools/Scripts/webkitpy/tool/mocktool.py index 30a4bc3..eb7c248 100644 --- a/Tools/Scripts/webkitpy/tool/mocktool.py +++ b/Tools/Scripts/webkitpy/tool/mocktool.py @@ -540,10 +540,14 @@ class MockCheckout(object): class MockUser(object): - @staticmethod - def prompt(message, repeat=1, raw_input=raw_input): + @classmethod + def prompt(cls, message, repeat=1, raw_input=raw_input): return "Mock user response" + @classmethod + def prompt_with_list(cls, list_title, list_items, can_choose_multiple=False, raw_input=raw_input): + pass + def edit(self, files): pass diff --git a/Tools/Scripts/webkitpy/tool/steps/__init__.py b/Tools/Scripts/webkitpy/tool/steps/__init__.py index 64d9d05..f426f17 100644 --- a/Tools/Scripts/webkitpy/tool/steps/__init__.py +++ b/Tools/Scripts/webkitpy/tool/steps/__init__.py @@ -56,4 +56,5 @@ 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.validatechangelogs import ValidateChangeLogs from webkitpy.tool.steps.validatereviewer import ValidateReviewer diff --git a/Tools/Scripts/webkitpy/tool/steps/abstractstep.py b/Tools/Scripts/webkitpy/tool/steps/abstractstep.py index 5525ea0..1c93d5b 100644 --- a/Tools/Scripts/webkitpy/tool/steps/abstractstep.py +++ b/Tools/Scripts/webkitpy/tool/steps/abstractstep.py @@ -52,6 +52,7 @@ class AbstractStep(object): "bug_title": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]).title(), "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit), "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, changed_files=self._changed_files(state)), + # Absolute path to ChangeLog files. "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit, changed_files=self._changed_files(state)), } diff --git a/Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py b/Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py index 9c16242..a165dd2 100644 --- a/Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py +++ b/Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py @@ -47,7 +47,9 @@ class CleanWorkingDirectory(AbstractStep): def run(self, state): if not self._options.clean: return - # FIXME: This chdir should not be necessary. + # FIXME: This chdir should not be necessary and can be removed as + # soon as ensure_no_local_commits and ensure_clean_working_directory + # are known to set the CWD to checkout_root when calling run_command. os.chdir(self._tool.scm().checkout_root) if not self._allow_local_commits: self._tool.scm().ensure_no_local_commits(self._options.force_clean) diff --git a/Tools/Scripts/webkitpy/tool/steps/commit.py b/Tools/Scripts/webkitpy/tool/steps/commit.py index 5aa6b51..859acbf 100644 --- a/Tools/Scripts/webkitpy/tool/steps/commit.py +++ b/Tools/Scripts/webkitpy/tool/steps/commit.py @@ -36,12 +36,6 @@ from webkitpy.tool.steps.options import Options class Commit(AbstractStep): - @classmethod - def options(cls): - return AbstractStep.options() + [ - Options.git_commit, - ] - def _commit_warning(self, error): working_directory_message = "" if error.working_directory_is_clean else " and working copy changes" return ('There are %s local commits%s. Everything will be committed as a single commit. ' diff --git a/Tools/Scripts/webkitpy/tool/steps/update.py b/Tools/Scripts/webkitpy/tool/steps/update.py index cd1d4d8..036c46d 100644 --- a/Tools/Scripts/webkitpy/tool/steps/update.py +++ b/Tools/Scripts/webkitpy/tool/steps/update.py @@ -36,10 +36,11 @@ class Update(AbstractStep): def options(cls): return AbstractStep.options() + [ Options.update, + Options.quiet, ] def run(self, state): if not self._options.update: return log("Updating working directory") - self._tool.executive.run_and_throw_if_fail(self._tool.port().update_webkit_command(), quiet=True) + self._tool.executive.run_and_throw_if_fail(self._tool.port().update_webkit_command(), quiet=self._options.quiet) diff --git a/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py b/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py index e46b790..2bf41b3 100644 --- a/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py +++ b/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py @@ -67,6 +67,9 @@ class UpdateChangeLogsWithReviewer(AbstractStep): log("Failed to guess reviewer from bug %s and --reviewer= not provided. Not updating ChangeLogs with reviewer." % bug_id) return - os.chdir(self._tool.scm().checkout_root) + # cached_lookup("changelogs") is always absolute paths. for changelog_path in self.cached_lookup(state, "changelogs"): ChangeLog(changelog_path).set_reviewer(reviewer) + + # Tell the world that we just changed something on disk so that the cached diff is invalidated. + self.did_modify_checkout(state) diff --git a/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py b/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py new file mode 100644 index 0000000..e812f94 --- /dev/null +++ b/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py @@ -0,0 +1,58 @@ +# 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. + +from webkitpy.tool.steps.abstractstep import AbstractStep +from webkitpy.tool.steps.options import Options +from webkitpy.common.checkout.diff_parser import DiffParser +from webkitpy.common.system.deprecated_logging import error, log + + +# This is closely related to the ValidateReviewer step and the CommitterValidator class. +# We may want to unify more of this code in one place. +class ValidateChangeLogs(AbstractStep): + def _check_changelog_diff(self, diff_file): + if not self._tool.checkout().is_path_to_changelog(diff_file.filename): + return True + # Each line is a tuple, the first value is the deleted line number + # Date, reviewer, bug title, bug url, and empty lines could all be + # identical in the most recent entries. If the diff starts any + # later than that, assume that the entry is wrong. + if diff_file.lines[0][0] < 8: + return True + log("The diff to %s looks wrong. Are you sure your ChangeLog entry is at the top of the file?" % (diff_file.filename)) + # FIXME: Do we need to make the file path absolute? + self._tool.scm().diff_for_file(diff_file.filename) + if self._tool.user.confirm("OK to continue?", default='n'): + return True + return False + + def run(self, state): + parsed_diff = DiffParser(self.cached_lookup(state, "diff").splitlines()) + for filename, diff_file in parsed_diff.files.items(): + if not self._check_changelog_diff(diff_file): + error("ChangeLog entry in %s is not at the top of the file." % diff_file.filename) diff --git a/Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py b/Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py new file mode 100644 index 0000000..66db793 --- /dev/null +++ b/Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.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. + +import unittest + +from webkitpy.common.system.outputcapture import OutputCapture +from webkitpy.thirdparty.mock import Mock +from webkitpy.tool.mocktool import MockOptions, MockTool +from webkitpy.tool.steps.validatechangelogs import ValidateChangeLogs + + +class ValidateChangeLogsTest(unittest.TestCase): + + def _assert_start_line_produces_output(self, start_line, should_prompt_user=False): + tool = MockTool() + tool._checkout.is_path_to_changelog = lambda path: True + step = ValidateChangeLogs(tool, MockOptions(git_commit=None)) + diff_file = Mock() + diff_file.filename = "mock/ChangeLog" + diff_file.lines = [(start_line, start_line, "foo")] + expected_stdout = expected_stderr = "" + if should_prompt_user: + expected_stdout = "OK to continue?\n" + expected_stderr = "The diff to mock/ChangeLog looks wrong. Are you sure your ChangeLog entry is at the top of the file?\n" + OutputCapture().assert_outputs(self, step._check_changelog_diff, [diff_file], expected_stdout=expected_stdout, expected_stderr=expected_stderr) + + def test_check_changelog_diff(self): + self._assert_start_line_produces_output(1, should_prompt_user=False) + self._assert_start_line_produces_output(7, should_prompt_user=False) + self._assert_start_line_produces_output(8, should_prompt_user=True) diff --git a/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py b/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py index bdf729e..c2a27b9 100644 --- a/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py +++ b/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py @@ -37,12 +37,6 @@ from webkitpy.common.system.deprecated_logging import error, log # FIXME: Some of this logic should probably be unified with CommitterValidator? class ValidateReviewer(AbstractStep): - @classmethod - def options(cls): - return AbstractStep.options() + [ - Options.git_commit, - ] - # FIXME: This should probably move onto ChangeLogEntry def _has_valid_reviewer(self, changelog_entry): if changelog_entry.reviewer(): @@ -58,9 +52,6 @@ class ValidateReviewer(AbstractStep): # this check is too draconian (and too poorly tested) to foist upon users. if not self._options.non_interactive: return - # FIXME: We should figure out how to handle the current working - # directory issue more globally. - os.chdir(self._tool.scm().checkout_root) for changelog_path in self.cached_lookup(state, "changelogs"): changelog_entry = ChangeLog(changelog_path).latest_entry() if self._has_valid_reviewer(changelog_entry): |