Log Message
Make TestInput immutable https://bugs.webkit.org/show_bug.cgi?id=224989 Reviewed by Jonathan Bedard.
The main point here is moving computing reference_files and should_run_pixel_test to when we initially construct TestInput, as at this point this happens in the some process and thread (since bug 221577), hence there's no real reason for it to happen later. In doing this, I've eliminated Port.should_run_as_pixel_test on the basis that no port actually overrode this to apply any different logic, especially given it seems unlikely that any port would want to use different logic here. (Note that ports still have some control through Port.default_pixel_tests.) With this done, it should then be possible to make TestInput immutable, which should help make things easier to understand. Expect, as it happens, there was a reason for it to happen later: we previously generated all the TestInputs twice, once to find out how many workers we need and then another time to actually run them (plus potentially a third time for retries!). There's no actual reason to do this, so move the creation to Manager.run and pass that list around instead of the Tests. * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py: (LayoutTestRunner.run_tests): Don't update TestInput. (LayoutTestRunner._update_test_input): Deleted. * Scripts/webkitpy/layout_tests/controllers/manager.py: (Manager._test_input_for_file): Moved from _update_test_input and Port.should_run_as_pixel_test. (Manager._get_test_inputs): Deleted. (Manager._multiply_test_inputs): Simplify code used to generated repeated/rerun test inputs. (Manager._update_worker_count): Don't create TestInputs; take test_inputs as arg. (Manager._set_up_run): Rename test_names to test_inputs. (Manager.run): Create TestInput objects here. (Manager._run_test_subset): Take TestInputs not Tests, generate new TestInputs for retry if needed. (Manager._run_tests): Don't create TestInputs; take test_inputs as arg. * Scripts/webkitpy/layout_tests/models/test.py: Fly-by: use __slots__. * Scripts/webkitpy/layout_tests/models/test_input.py: (TestInput): Migrate to attrs. (TestInput.__init__): Deleted. (TestInput.__repr__): Deleted. * Scripts/webkitpy/port/base.py: (Port.should_run_as_pixel_test): Deleted. (Port._should_run_as_pixel_test): Deleted.
Modified Paths
- trunk/Tools/ChangeLog
- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
- trunk/Tools/Scripts/webkitpy/layout_tests/models/test.py
- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_input.py
- trunk/Tools/Scripts/webkitpy/port/base.py
Diff
Modified: trunk/Tools/ChangeLog (276669 => 276670)
--- trunk/Tools/ChangeLog 2021-04-27 22:06:32 UTC (rev 276669)
+++ trunk/Tools/ChangeLog 2021-04-27 22:09:45 UTC (rev 276670)
@@ -1,5 +1,58 @@
2021-04-27 Sam Sneddon <[email protected]>
+ Make TestInput immutable
+ https://bugs.webkit.org/show_bug.cgi?id=224989
+
+ Reviewed by Jonathan Bedard.
+
+ The main point here is moving computing reference_files and
+ should_run_pixel_test to when we initially construct TestInput, as at
+ this point this happens in the some process and thread (since bug
+ 221577), hence there's no real reason for it to happen later.
+
+ In doing this, I've eliminated Port.should_run_as_pixel_test on the
+ basis that no port actually overrode this to apply any different logic,
+ especially given it seems unlikely that any port would want to use
+ different logic here. (Note that ports still have some control through
+ Port.default_pixel_tests.)
+
+ With this done, it should then be possible to make TestInput immutable,
+ which should help make things easier to understand.
+
+ Expect, as it happens, there was a reason for it to happen later: we
+ previously generated all the TestInputs twice, once to find out how
+ many workers we need and then another time to actually run them (plus
+ potentially a third time for retries!). There's no actual reason to do
+ this, so move the creation to Manager.run and pass that list around
+ instead of the Tests.
+
+ * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:
+ (LayoutTestRunner.run_tests): Don't update TestInput.
+ (LayoutTestRunner._update_test_input): Deleted.
+ * Scripts/webkitpy/layout_tests/controllers/manager.py:
+ (Manager._test_input_for_file): Moved from _update_test_input and
+ Port.should_run_as_pixel_test.
+ (Manager._get_test_inputs): Deleted.
+ (Manager._multiply_test_inputs): Simplify code used to generated
+ repeated/rerun test inputs.
+ (Manager._update_worker_count): Don't create TestInputs; take
+ test_inputs as arg.
+ (Manager._set_up_run): Rename test_names to test_inputs.
+ (Manager.run): Create TestInput objects here.
+ (Manager._run_test_subset): Take TestInputs not Tests, generate new
+ TestInputs for retry if needed.
+ (Manager._run_tests): Don't create TestInputs; take test_inputs as arg.
+ * Scripts/webkitpy/layout_tests/models/test.py: Fly-by: use __slots__.
+ * Scripts/webkitpy/layout_tests/models/test_input.py:
+ (TestInput): Migrate to attrs.
+ (TestInput.__init__): Deleted.
+ (TestInput.__repr__): Deleted.
+ * Scripts/webkitpy/port/base.py:
+ (Port.should_run_as_pixel_test): Deleted.
+ (Port._should_run_as_pixel_test): Deleted.
+
+2021-04-27 Sam Sneddon <[email protected]>
+
Optimize Port._expected_baselines_for_suffixes
https://bugs.webkit.org/show_bug.cgi?id=225115
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (276669 => 276670)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py 2021-04-27 22:06:32 UTC (rev 276669)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py 2021-04-27 22:09:45 UTC (rev 276670)
@@ -94,10 +94,7 @@
def run_tests(self, expectations, test_inputs, num_workers, retrying, device_type=None):
self._expectations = expectations
- self._test_inputs = []
- for test_input in test_inputs:
- self._update_test_input(test_input, device_type)
- self._test_inputs.append(test_input)
+ self._test_inputs = list(test_inputs)
self._retrying = retrying
@@ -136,15 +133,6 @@
return run_results
- def _update_test_input(self, test_input, device_type=None):
- if test_input.reference_files is None:
- # Lazy initialization.
- test_input.reference_files = self._port.reference_files(test_input.test_name, device_type=device_type)
- if test_input.reference_files:
- test_input.should_run_pixel_test = True
- else:
- test_input.should_run_pixel_test = self._port.should_run_as_pixel_test(test_input)
-
def _worker_factory(self, worker_connection):
results_directory = self._results_directory
if self._retrying:
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (276669 => 276670)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py 2021-04-27 22:06:32 UTC (rev 276669)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py 2021-04-27 22:09:45 UTC (rev 276670)
@@ -215,11 +215,40 @@
return tests_to_run
def _test_input_for_file(self, test_file, device_type):
+ reference_files = self._port.reference_files(
+ test_file.test_path, device_type=device_type
+ )
+ timeout = (
+ self._options.slow_time_out_ms
+ if self._test_is_slow(test_file.test_path, device_type=device_type)
+ else self._options.time_out_ms
+ )
+ should_dump_jsconsolelog_in_stderr = (
+ self._test_should_dump_jsconsolelog_in_stderr(
+ test_file.test_path, device_type=device_type
+ )
+ )
+
+ if reference_files:
+ should_run_pixel_test = True
+ elif not self._options.pixel_tests:
+ should_run_pixel_test = False
+ elif self._options.pixel_test_directories:
+ should_run_pixel_test = any(
+ test_file.test_path.startswith(directory)
+ for directory in self._options.pixel_test_directories
+ )
+ else:
+ should_run_pixel_test = True
+
return TestInput(
test_file,
- self._options.slow_time_out_ms if self._test_is_slow(test_file.test_path, device_type=device_type) else self._options.time_out_ms,
- test_file.needs_any_server,
- should_dump_jsconsolelog_in_stderr=self._test_should_dump_jsconsolelog_in_stderr(test_file.test_path, device_type=device_type))
+ timeout=timeout,
+ needs_servers=test_file.needs_any_server,
+ should_dump_jsconsolelog_in_stderr=should_dump_jsconsolelog_in_stderr,
+ reference_files=reference_files,
+ should_run_pixel_test=should_run_pixel_test,
+ )
def _test_is_slow(self, test_file, device_type):
if self._expectations[device_type].model().has_modifier(test_file, test_expectations.SLOW):
@@ -229,20 +258,22 @@
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)
- def _get_test_inputs(self, tests_to_run, repeat_each, iterations, device_type):
- test_inputs = []
- for _ in range(iterations):
- for test in tests_to_run:
- for _ in range(repeat_each):
- test_inputs.append(self._test_input_for_file(test, device_type=device_type))
- return test_inputs
+ def _multiply_test_inputs(self, test_inputs, repeat_each, iterations):
+ if repeat_each == 1:
+ per_iteration = list(test_inputs)[:]
+ else:
+ per_iteration = []
+ for test_input in test_inputs:
+ per_iteration.extend([test_input] * repeat_each)
- def _update_worker_count(self, test_names, device_type):
- test_inputs = self._get_test_inputs(test_names, self._options.repeat_each, self._options.iterations, device_type=device_type)
- worker_count = self._runner.get_worker_count(test_inputs, int(self._options.child_processes))
+ return per_iteration * iterations
+
+ def _update_worker_count(self, test_inputs):
+ new_test_inputs = self._multiply_test_inputs(test_inputs, self._options.repeat_each, self._options.iterations)
+ worker_count = self._runner.get_worker_count(new_test_inputs, int(self._options.child_processes))
self._options.child_processes = worker_count
- def _set_up_run(self, test_names, device_type):
+ def _set_up_run(self, test_inputs, device_type):
# This must be started before we check the system dependencies,
# since the helper may do things to make the setup correct.
self._printer.write_update("Starting helper ...")
@@ -249,7 +280,7 @@
if not self._port.start_helper(pixel_tests=self._options.pixel_tests, prefer_integrated_gpu=self._options.prefer_integrated_gpu):
return False
- self._update_worker_count(test_names, device_type=device_type)
+ self._update_worker_count(test_inputs)
self._port.reset_preferences()
# Check that the system dependencies (themes, fonts, ...) are correct.
@@ -353,13 +384,17 @@
start_time_for_device = time.time()
if not tests_to_run_by_device[device_type]:
continue
- if not self._set_up_run(tests_to_run_by_device[device_type], device_type=device_type):
+
+ test_inputs = [self._test_input_for_file(test, device_type=device_type)
+ for test in tests_to_run_by_device[device_type]]
+
+ if not self._set_up_run(test_inputs, device_type=device_type):
return test_run_results.RunDetails(exit_code=-1)
configuration = self._port.configuration_for_upload(self._port.target_host(0))
if not configuration.get('flavor', None): # The --result-report-flavor argument should override wk1/wk2
configuration['flavor'] = 'wk2' if self._options.webkit_test_runner else 'wk1'
- temp_initial_results, temp_retry_results, temp_enabled_pixel_tests_in_retry = self._run_test_subset(tests_to_run_by_device[device_type], device_type=device_type)
+ temp_initial_results, temp_retry_results, temp_enabled_pixel_tests_in_retry = self._run_test_subset(test_inputs, device_type=device_type)
skipped_results = TestRunResults(self._expectations[device_type], len(aggregate_tests_to_skip))
for skipped_test in set(aggregate_tests_to_skip):
@@ -428,12 +463,12 @@
return result
def _run_test_subset(self,
- tests_to_run, # type: List[Test]
+ test_inputs, # type: List[TestInput]
device_type, # type: Optional[DeviceType]
):
try:
enabled_pixel_tests_in_retry = False
- initial_results = self._run_tests(tests_to_run, self._options.repeat_each, self._options.iterations, int(self._options.child_processes), retrying=False, device_type=device_type)
+ initial_results = self._run_tests(test_inputs, self._options.repeat_each, self._options.iterations, int(self._options.child_processes), retrying=False, device_type=device_type)
tests_to_retry = self._tests_to_retry(initial_results, include_crashes=self._port.should_retry_crashes())
# Don't retry failures when interrupted by user or failures limit exception.
@@ -440,11 +475,19 @@
retry_failures = self._options.retry_failures and not (initial_results.interrupted or initial_results.keyboard_interrupted)
if retry_failures and tests_to_retry:
enabled_pixel_tests_in_retry = self._force_pixel_tests_if_needed()
+ if enabled_pixel_tests_in_retry:
+ retry_test_inputs = [self._test_input_for_file(test_input.test, device_type=device_type)
+ for test_input in test_inputs
+ if test_input.test.test_path in tests_to_retry]
+ else:
+ retry_test_inputs = [test_input
+ for test_input in test_inputs
+ if test_input.test.test_path in tests_to_retry]
_log.info('')
_log.info("Retrying %s ..." % pluralize(len(tests_to_retry), "unexpected failure"))
_log.info('')
- retry_results = self._run_tests([test for test in tests_to_run if test.test_path in tests_to_retry],
+ retry_results = self._run_tests(retry_test_inputs,
repeat_each=1,
iterations=1,
num_workers=1,
@@ -494,7 +537,7 @@
return test_run_results.RunDetails(exit_code, summarized_results, initial_results, retry_results, enabled_pixel_tests_in_retry)
def _run_tests(self,
- tests_to_run, # type: List[Test]
+ test_inputs, # type: List[TestInput]
repeat_each, # type: int
iterations, # type: int
num_workers, # type: int
@@ -501,10 +544,10 @@
retrying, # type: bool
device_type, # type: Optional[DeviceType]
):
- test_inputs = self._get_test_inputs(tests_to_run, repeat_each, iterations, device_type=device_type)
+ new_test_inputs = self._multiply_test_inputs(test_inputs, repeat_each, iterations)
assert self._runner is not None
- return self._runner.run_tests(self._expectations[device_type], test_inputs, num_workers, retrying, device_type)
+ return self._runner.run_tests(self._expectations[device_type], new_test_inputs, num_workers, retrying, device_type)
def _clean_up_run(self):
_log.debug("Flushing stdout")
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test.py (276669 => 276670)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test.py 2021-04-27 22:06:32 UTC (rev 276669)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test.py 2021-04-27 22:09:45 UTC (rev 276670)
@@ -30,7 +30,7 @@
import attr
[email protected](frozen=True)
[email protected](frozen=True, slots=True)
class Test(object):
"""Data about a test and its expectations.
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test_input.py (276669 => 276670)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_input.py 2021-04-27 22:06:32 UTC (rev 276669)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_input.py 2021-04-27 22:09:45 UTC (rev 276670)
@@ -1,5 +1,6 @@
# Copyright (C) 2010 Google Inc. All rights reserved.
# Copyright (C) 2010 Gabor Rapcsanyi ([email protected]), University of Szeged
+# Copyright (C) 2021 Apple Inc. All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -28,8 +29,12 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+import attr
+
from .test import Test
+
[email protected](frozen=True, slots=True)
class TestInput(object):
"""Information about a test needed to run it.
@@ -36,25 +41,13 @@
This differs from a Test object insofar as it contains metadata not specific to the test,
derived from TestExpectations/test execution options (e.g., timeout).
"""
+ test = attr.ib(type=Test)
+ timeout = attr.ib(default=None) # type: Union[None, int, str]
+ needs_servers = attr.ib(default=None) # type: Optional[bool]
+ should_dump_jsconsolelog_in_stderr = attr.ib(default=None) # type: Optional[bool]
+ reference_files = attr.ib(default=None) # type: Optional[List[Tuple[str str]]]
+ should_run_pixel_test = attr.ib(default=None) # type: Optional[bool]
- def __init__(self,
- test, # type: Test
- timeout=None, # type: Union[None, int, str]
- needs_servers=None, # type: Optional[bool]
- should_dump_jsconsolelog_in_stderr=None, # type: Optional[bool]
- ):
- # TestInput objects are normally constructed by the manager and passed
- # to the workers, but these some fields are set lazily in the workers where possible
- # because they require us to look at the filesystem and we want to be able to do that in parallel.
- self.test = test
- self.timeout = timeout # in msecs; should rename this for consistency
- self.needs_servers = needs_servers
- self.should_dump_jsconsolelog_in_stderr = should_dump_jsconsolelog_in_stderr
- self.reference_files = None
-
@property
def test_name(self):
return self.test.test_path
-
- def __repr__(self):
- return "TestInput('%s', timeout=%s, needs_servers=%s, reference_files=%s, should_dump_jsconsolelog_in_stderr=%s)" % (self.test_name, self.timeout, self.needs_servers, self.reference_files, self.should_dump_jsconsolelog_in_stderr)
Modified: trunk/Tools/Scripts/webkitpy/port/base.py (276669 => 276670)
--- trunk/Tools/Scripts/webkitpy/port/base.py 2021-04-27 22:06:32 UTC (rev 276669)
+++ trunk/Tools/Scripts/webkitpy/port/base.py 2021-04-27 22:09:45 UTC (rev 276670)
@@ -1361,18 +1361,6 @@
def sample_process(self, name, pid, target_host=None):
pass
- def should_run_as_pixel_test(self, test_input):
- if not self._options.pixel_tests:
- return False
- if self._options.pixel_test_directories:
- return any(test_input.test_name.startswith(directory) for directory in self._options.pixel_test_directories)
- return self._should_run_as_pixel_test(test_input)
-
- def _should_run_as_pixel_test(self, test_input):
- # Default behavior is to allow all test to run as pixel tests if --pixel-tests is on and
- # --pixel-test-directory is not specified.
- return True
-
def _in_flatpak_sandbox(self):
return self._filesystem.exists("/.flatpak-info")
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
