Attachment 426022 Details for Bug 224563 – Patch

[patch] Patch

bug-224563-20210414193208.patch (text/plain), 17.52 KB, created by Sam Sneddon [:gsnedders] on 2021-04-14 11:32:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sam Sneddon [:gsnedders]

Created: 2021-04-14 11:32:10 PDT

Size: 17.52 KB

patch

obsolete

>Subversion Revision: 275952
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 395b9b7f1ea1a2fbed092dbfdb7a9c7340b4993c..668d42fd76a09d44822d28c37780447215b8951c 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,31 @@
>+2021-04-14  Sam Sneddon  <gsnedders@apple.com>
>+
>+        Use TestExpectations directly to check whether a test is slow
>+        https://bugs.webkit.org/show_bug.cgi?id=224563
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This removes support for test-objects.json files, as none were ever added and this has thus
>+        been permanently dead code, allowing us to simplify our code to inspect the expectation
>+        object directly from TestRunResults.
>+
>+        * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:
>+        (LayoutTestRunner.__init__): Remove test_is_slow_fn argument
>+        (LayoutTestRunner._mark_interrupted_tests_as_skipped): Stop passing test_is_slow
>+        (LayoutTestRunner._update_summary_with_result): Stop passing test_is_slow
>+        * Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py:
>+        (LayoutTestRunnerTests._runner): Stop passing test_is_slow
>+        * Scripts/webkitpy/layout_tests/controllers/manager.py:
>+        (Manager): Improve docstring
>+        (Manager.__init__): Stop passing test_is_slow
>+        (Manager._test_is_slow): Stop passing test_is_slow
>+        (Manager.run): Stop passing test_is_slow
>+        (Manager._look_for_new_crash_logs): Stop passing test_is_slow
>+        * Scripts/webkitpy/layout_tests/models/test_run_results.py:
>+        (TestRunResults.add): Remove test_is_slow_fn argument; query TestExpectations to check if test is slow
>+        * Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py:
>+        (summarized_results): Stop passing test_is_slow
>+
> 2021-04-14  Kate Cheney  <katherine_cheney@apple.com>
> 
>         Service worker soft-update loads not being marked app-bound
>diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py b/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
>index 2e60b312de6ec8b23b7372a7c725f67158cd66ec..a3ecae3875492dc26b40d6fd366878ac5dc840f0 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
>@@ -63,12 +63,11 @@ class TestRunInterruptedException(Exception):
> 
> 
> class LayoutTestRunner(object):
>-    def __init__(self, options, port, printer, results_directory, test_is_slow_fn, needs_http=False, needs_websockets=False, needs_web_platform_test_server=False):
>+    def __init__(self, options, port, printer, results_directory, needs_http=False, needs_websockets=False, needs_web_platform_test_server=False):
>         self._options = options
>         self._port = port
>         self._printer = printer
>         self._results_directory = results_directory
>-        self._test_is_slow = test_is_slow_fn
>         self._needs_http = needs_http
>         self._needs_websockets = needs_websockets
>         self._needs_web_platform_test_server = needs_web_platform_test_server
>@@ -162,7 +161,7 @@ class LayoutTestRunner(object):
>                 # FIXME: We probably need to loop here if there are multiple iterations.
>                 # FIXME: Also, these results are really neither expected nor unexpected. We probably
>                 # need a third type of result.
>-                run_results.add(result, expected=False, test_is_slow=self._test_is_slow(test_input.test_name))
>+                run_results.add(result, expected=False)
> 
>     def _interrupt_if_at_failure_limits(self, run_results):
>         # Note: The messages in this method are constructed to match old-run-webkit-tests
>@@ -195,7 +194,7 @@ class LayoutTestRunner(object):
>             exp_str = self._expectations.model().expectations_to_string(expectations)
>             got_str = self._expectations.model().expectation_to_string(result.type)
> 
>-        run_results.add(result, expected, self._test_is_slow(result.test_name))
>+        run_results.add(result, expected)
> 
>         self._printer.print_finished_test(result, expected, exp_str, got_str)
> 
>diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py b/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py
>index 0f2721c92e94daa547917c558a63d247a9deed70..afe89369bd344f79848bf64a602456f5b5aa113f 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py
>@@ -79,7 +79,7 @@ class LayoutTestRunnerTests(unittest.TestCase):
> 
>         host = MockHost()
>         port = port or host.port_factory.get(options.platform, options=options)
>-        return LayoutTestRunner(options, port, FakePrinter(), port.results_directory(), lambda test_name: False)
>+        return LayoutTestRunner(options, port, FakePrinter(), port.results_directory())
> 
>     def _run_tests(self, runner, tests):
>         test_inputs = [TestInput(Test(test), 6000) for test in tests]
>diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py b/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
>index 728a16503739accc68a54f40362e707f9218e558..4a3e098baaf664f73de13affc38e4faef1facdae 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
>@@ -66,8 +66,13 @@ TestExpectations = test_expectations.TestExpectations
> 
> 
> class Manager(object):
>-    """A class for managing running a series of tests on a series of layout
>-    test files."""
>+    """Test execution manager
>+
>+    This class has the main entry points for run-webkit-tests; the ..run_webkit_tests module almost
>+    exclusively just handles CLI options. It orchestrates collecting the tests (through
>+    LayoutTestFinder), running them (LayoutTestRunner), and then displaying the results
>+    (TestResultWriter/Printer).
>+    """
> 
>     def __init__(self, port, options, printer):
>         """Initialize test runner data structures.
>@@ -82,14 +87,10 @@ class Manager(object):
>         self._options = options
>         self._printer = printer
>         self._expectations = OrderedDict()
>-        self.LAYOUT_TESTS_DIRECTORY = 'LayoutTests'
>         self._results_directory = self._port.results_directory()
>         self._finder = LayoutTestFinder(self._port, self._options)
>         self._runner = None
> 
>-        test_options_json_path = self._port.path_from_webkit_base(self.LAYOUT_TESTS_DIRECTORY, "tests-options.json")
>-        self._tests_options = json.loads(self._filesystem.read_text_file(test_options_json_path)) if self._filesystem.exists(test_options_json_path) else {}
>-
>     def _collect_tests(self,
>                        paths,  # type: List[str]
>                        device_type_list,  # type: List[Optional[DeviceType]]
>@@ -224,7 +225,7 @@ class Manager(object):
>     def _test_is_slow(self, test_file, device_type):
>         if self._expectations[device_type].model().has_modifier(test_file, test_expectations.SLOW):
>             return True
>-        return "slow" in self._tests_options.get(test_file, [])
>+        return False
> 
>     def _test_should_dump_jsconsolelog_in_stderr(self, test_file, device_type):
>         return self._expectations[device_type].model().has_modifier(test_file, test_expectations.DUMPJSCONSOLELOGINSTDERR)
>@@ -323,7 +324,7 @@ class Manager(object):
>         needs_http = any(test.needs_http_server for tests in itervalues(tests_to_run_by_device) for test in tests)
>         needs_web_platform_test_server = any(test.needs_wpt_server for tests in itervalues(tests_to_run_by_device) for test in tests)
>         needs_websockets = any(test.needs_websocket_server for tests in itervalues(tests_to_run_by_device) for test in tests)
>-        self._runner = LayoutTestRunner(self._options, self._port, self._printer, self._results_directory, self._test_is_slow,
>+        self._runner = LayoutTestRunner(self._options, self._port, self._printer, self._results_directory,
>                                         needs_http=needs_http, needs_web_platform_test_server=needs_web_platform_test_server, needs_websockets=needs_websockets)
> 
>         initial_results = None
>@@ -335,7 +336,6 @@ class Manager(object):
>         uploads = []
> 
>         for device_type in device_type_list:
>-            self._runner._test_is_slow = lambda test_file: self._test_is_slow(test_file, device_type=device_type)
>             self._options.child_processes = min(self._port.max_child_processes(device_type=device_type), int(child_processes_option_value or self._port.default_child_processes(device_type=device_type)))
> 
>             _log.info('')
>@@ -365,7 +365,7 @@ class Manager(object):
>             for skipped_test in set(aggregate_tests_to_skip):
>                 skipped_result = test_results.TestResult(skipped_test.test_path)
>                 skipped_result.type = test_expectations.SKIP
>-                skipped_results.add(skipped_result, expected=True, test_is_slow=self._test_is_slow(skipped_test.test_path, device_type=device_type))
>+                skipped_results.add(skipped_result, expected=True)
>             temp_initial_results = temp_initial_results.merge(skipped_results)
> 
>             if self._options.report_urls:
>@@ -559,7 +559,7 @@ class Manager(object):
>                     result = test_results.TestResult(test)
>                     result.type = test_expectations.CRASH
>                     result.is_other_crash = True
>-                    run_results.add(result, expected=False, test_is_slow=False)
>+                    run_results.add(result, expected=False)
>                     _log.debug("Adding results for other crash: " + str(test))
> 
>     def _clobber_old_results(self):
>diff --git a/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py b/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
>index 8f916d8a7c709423098060b1c388e80aada0c592..4923e3f0f25208e8497ff1e0ff2ae2ca279ddb79 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
>@@ -68,7 +68,7 @@ class TestRunResults(object):
>         self.interrupted = False
>         self.keyboard_interrupted = False
> 
>-    def add(self, test_result, expected, test_is_slow):
>+    def add(self, test_result, expected):
>         self.tests_by_expectation[test_result.type].add(test_result.test_name)
>         self.results_by_name[test_result.test_name] = test_result
>         if test_result.is_other_crash:
>@@ -92,6 +92,7 @@ class TestRunResults(object):
>                 self.unexpected_crashes += 1
>             elif test_result.type == test_expectations.TIMEOUT:
>                 self.unexpected_timeouts += 1
>+        test_is_slow = self.expectations.model().has_modifier(test_result.test_name, test_expectations.SLOW)
>         if test_is_slow:
>             self.slow_tests.add(test_result.test_name)
> 
>diff --git a/Tools/Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py b/Tools/Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py
>index 0e91a32c990f61a759292ef387803008f1889d6b..4305ae1730619a969ed0ab33bdc99fcaaccd8e3a 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py
>@@ -60,58 +60,56 @@ def run_results(port):
> 
> 
> def summarized_results(port, expected, passing, flaky, include_passes=False):
>-    test_is_slow = False
>-
>     initial_results = run_results(port)
>     if expected:
>-        initial_results.add(get_result('passes/text.html', test_expectations.PASS), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/audio.html', test_expectations.AUDIO), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/timeout.html', test_expectations.TIMEOUT), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/crash.html', test_expectations.CRASH), expected, test_is_slow)
>+        initial_results.add(get_result('passes/text.html', test_expectations.PASS), expected)
>+        initial_results.add(get_result('failures/expected/audio.html', test_expectations.AUDIO), expected)
>+        initial_results.add(get_result('failures/expected/timeout.html', test_expectations.TIMEOUT), expected)
>+        initial_results.add(get_result('failures/expected/crash.html', test_expectations.CRASH), expected)
> 
>         if port._options.pixel_tests:
>-            initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.IMAGE), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.IMAGE), expected)
>         else:
>-            initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.PASS), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.PASS), expected)
> 
>         if port._options.world_leaks:
>-            initial_results.add(get_result('failures/expected/leak.html', test_expectations.LEAK), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/leak.html', test_expectations.LEAK), expected)
>         else:
>-            initial_results.add(get_result('failures/expected/leak.html', test_expectations.PASS), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/leak.html', test_expectations.PASS), expected)
> 
>     elif passing:
>-        initial_results.add(get_result('passes/text.html'), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/audio.html'), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/timeout.html'), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/crash.html'), expected, test_is_slow)
>+        initial_results.add(get_result('passes/text.html'), expected)
>+        initial_results.add(get_result('failures/expected/audio.html'), expected)
>+        initial_results.add(get_result('failures/expected/timeout.html'), expected)
>+        initial_results.add(get_result('failures/expected/crash.html'), expected)
> 
>         if port._options.pixel_tests:
>-            initial_results.add(get_result('failures/expected/pixel-fail.html'), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/pixel-fail.html'), expected)
>         else:
>-            initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.IMAGE), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.IMAGE), expected)
> 
>         if port._options.world_leaks:
>-            initial_results.add(get_result('failures/expected/leak.html'), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/leak.html'), expected)
>         else:
>-            initial_results.add(get_result('failures/expected/leak.html', test_expectations.PASS), expected, test_is_slow)
>+            initial_results.add(get_result('failures/expected/leak.html', test_expectations.PASS), expected)
>     else:
>-        initial_results.add(get_result('passes/text.html', test_expectations.TIMEOUT), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/audio.html', test_expectations.AUDIO), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/timeout.html', test_expectations.CRASH), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/crash.html', test_expectations.TIMEOUT), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.TIMEOUT), expected, test_is_slow)
>-        initial_results.add(get_result('failures/expected/leak.html', test_expectations.CRASH), expected, test_is_slow)
>+        initial_results.add(get_result('passes/text.html', test_expectations.TIMEOUT), expected)
>+        initial_results.add(get_result('failures/expected/audio.html', test_expectations.AUDIO), expected)
>+        initial_results.add(get_result('failures/expected/timeout.html', test_expectations.CRASH), expected)
>+        initial_results.add(get_result('failures/expected/crash.html', test_expectations.TIMEOUT), expected)
>+        initial_results.add(get_result('failures/expected/pixel-fail.html', test_expectations.TIMEOUT), expected)
>+        initial_results.add(get_result('failures/expected/leak.html', test_expectations.CRASH), expected)
> 
>         # we only list hang.html here, since normally this is WontFix
>-        initial_results.add(get_result('failures/expected/hang.html', test_expectations.TIMEOUT), expected, test_is_slow)
>+        initial_results.add(get_result('failures/expected/hang.html', test_expectations.TIMEOUT), expected)
> 
>     if flaky:
>         retry_results = run_results(port)
>-        retry_results.add(get_result('passes/text.html'), True, test_is_slow)
>-        retry_results.add(get_result('failures/expected/timeout.html'), True, test_is_slow)
>-        retry_results.add(get_result('failures/expected/crash.html'), True, test_is_slow)
>-        retry_results.add(get_result('failures/expected/pixel-fail.html'), True, test_is_slow)
>-        retry_results.add(get_result('failures/expected/leak.html'), True, test_is_slow)
>+        retry_results.add(get_result('passes/text.html'), True)
>+        retry_results.add(get_result('failures/expected/timeout.html'), True)
>+        retry_results.add(get_result('failures/expected/crash.html'), True)
>+        retry_results.add(get_result('failures/expected/pixel-fail.html'), True)
>+        retry_results.add(get_result('failures/expected/leak.html'), True)
>     else:
>         retry_results = None
>

Actions: View | Formatted Diff | Diff

Attachments on bug 224563: 426022 | 429110