⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 276670 in webkit


Ignore:
Timestamp:
Apr 27, 2021, 3:09:45 PM (5 years ago)
Author:
Sam Sneddon
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.

Location:
trunk/Tools
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r276669 r276670  
     12021-04-27  Sam Sneddon  <gsnedders@apple.com>
     2
     3        Make TestInput immutable
     4        https://bugs.webkit.org/show_bug.cgi?id=224989
     5
     6        Reviewed by Jonathan Bedard.
     7
     8        The main point here is moving computing reference_files and
     9        should_run_pixel_test to when we initially construct TestInput, as at
     10        this point this happens in the some process and thread (since bug
     11        221577), hence there's no real reason for it to happen later.
     12
     13        In doing this, I've eliminated Port.should_run_as_pixel_test on the
     14        basis that no port actually overrode this to apply any different logic,
     15        especially given it seems unlikely that any port would want to use
     16        different logic here. (Note that ports still have some control through
     17        Port.default_pixel_tests.)
     18
     19        With this done, it should then be possible to make TestInput immutable,
     20        which should help make things easier to understand.
     21
     22        Expect, as it happens, there was a reason for it to happen later: we
     23        previously generated all the TestInputs twice, once to find out how
     24        many workers we need and then another time to actually run them (plus
     25        potentially a third time for retries!). There's no actual reason to do
     26        this, so move the creation to Manager.run and pass that list around
     27        instead of the Tests.
     28
     29        * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:
     30        (LayoutTestRunner.run_tests): Don't update TestInput.
     31        (LayoutTestRunner._update_test_input): Deleted.
     32        * Scripts/webkitpy/layout_tests/controllers/manager.py:
     33        (Manager._test_input_for_file): Moved from _update_test_input and
     34        Port.should_run_as_pixel_test.
     35        (Manager._get_test_inputs): Deleted.
     36        (Manager._multiply_test_inputs): Simplify code used to generated
     37        repeated/rerun test inputs.
     38        (Manager._update_worker_count): Don't create TestInputs; take
     39        test_inputs as arg.
     40        (Manager._set_up_run): Rename test_names to test_inputs.
     41        (Manager.run): Create TestInput objects here.
     42        (Manager._run_test_subset): Take TestInputs not Tests, generate new
     43        TestInputs for retry if needed.
     44        (Manager._run_tests): Don't create TestInputs; take test_inputs as arg.
     45        * Scripts/webkitpy/layout_tests/models/test.py: Fly-by: use __slots__.
     46        * Scripts/webkitpy/layout_tests/models/test_input.py:
     47        (TestInput): Migrate to attrs.
     48        (TestInput.__init__): Deleted.
     49        (TestInput.__repr__): Deleted.
     50        * Scripts/webkitpy/port/base.py:
     51        (Port.should_run_as_pixel_test): Deleted.
     52        (Port._should_run_as_pixel_test): Deleted.
     53
    1542021-04-27  Sam Sneddon  <gsnedders@apple.com>
    255

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py

    r275773 r276670  
    9595    def run_tests(self, expectations, test_inputs, num_workers, retrying, device_type=None):
    9696        self._expectations = expectations
    97         self._test_inputs = []
    98         for test_input in test_inputs:
    99             self._update_test_input(test_input, device_type)
    100             self._test_inputs.append(test_input)
     97        self._test_inputs = list(test_inputs)
    10198
    10299        self._retrying = retrying
    … …  
    136133
    137134        return run_results
    138 
    139     def _update_test_input(self, test_input, device_type=None):
    140         if test_input.reference_files is None:
    141             # Lazy initialization.
    142             test_input.reference_files = self._port.reference_files(test_input.test_name, device_type=device_type)
    143         if test_input.reference_files:
    144             test_input.should_run_pixel_test = True
    145         else:
    146             test_input.should_run_pixel_test = self._port.should_run_as_pixel_test(test_input)
    147135
    148136    def _worker_factory(self, worker_connection):

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py

    r275888 r276670  
    216216
    217217    def _test_input_for_file(self, test_file, device_type):
     218        reference_files = self._port.reference_files(
     219            test_file.test_path, device_type=device_type
     220        )
     221        timeout = (
     222            self._options.slow_time_out_ms
     223            if self._test_is_slow(test_file.test_path, device_type=device_type)
     224            else self._options.time_out_ms
     225        )
     226        should_dump_jsconsolelog_in_stderr = (
     227            self._test_should_dump_jsconsolelog_in_stderr(
     228                test_file.test_path, device_type=device_type
     229            )
     230        )
     231
     232        if reference_files:
     233            should_run_pixel_test = True
     234        elif not self._options.pixel_tests:
     235            should_run_pixel_test = False
     236        elif self._options.pixel_test_directories:
     237            should_run_pixel_test = any(
     238                test_file.test_path.startswith(directory)
     239                for directory in self._options.pixel_test_directories
     240            )
     241        else:
     242            should_run_pixel_test = True
     243
    218244        return TestInput(
    219245            test_file,
    220             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,
    221             test_file.needs_any_server,
    222             should_dump_jsconsolelog_in_stderr=self._test_should_dump_jsconsolelog_in_stderr(test_file.test_path, device_type=device_type))
     246            timeout=timeout,
     247            needs_servers=test_file.needs_any_server,
     248            should_dump_jsconsolelog_in_stderr=should_dump_jsconsolelog_in_stderr,
     249            reference_files=reference_files,
     250            should_run_pixel_test=should_run_pixel_test,
     251        )
    223252
    224253    def _test_is_slow(self, test_file, device_type):
    … …  
    230259        return self._expectations[device_type].model().has_modifier(test_file, test_expectations.DUMPJSCONSOLELOGINSTDERR)
    231260
    232     def _get_test_inputs(self, tests_to_run, repeat_each, iterations, device_type):
    233         test_inputs = []
    234         for _ in range(iterations):
    235             for test in tests_to_run:
    236                 for _ in range(repeat_each):
    237                     test_inputs.append(self._test_input_for_file(test, device_type=device_type))
    238         return test_inputs
    239 
    240     def _update_worker_count(self, test_names, device_type):
    241         test_inputs = self._get_test_inputs(test_names, self._options.repeat_each, self._options.iterations, device_type=device_type)
    242         worker_count = self._runner.get_worker_count(test_inputs, int(self._options.child_processes))
     261    def _multiply_test_inputs(self, test_inputs, repeat_each, iterations):
     262        if repeat_each == 1:
     263            per_iteration = list(test_inputs)[:]
     264        else:
     265            per_iteration = []
     266            for test_input in test_inputs:
     267                per_iteration.extend([test_input] * repeat_each)
     268
     269        return per_iteration * iterations
     270
     271    def _update_worker_count(self, test_inputs):
     272        new_test_inputs = self._multiply_test_inputs(test_inputs, self._options.repeat_each, self._options.iterations)
     273        worker_count = self._runner.get_worker_count(new_test_inputs, int(self._options.child_processes))
    243274        self._options.child_processes = worker_count
    244275
    245     def _set_up_run(self, test_names, device_type):
     276    def _set_up_run(self, test_inputs, device_type):
    246277        # This must be started before we check the system dependencies,
    247278        # since the helper may do things to make the setup correct.
    … …  
    250281            return False
    251282
    252         self._update_worker_count(test_names, device_type=device_type)
     283        self._update_worker_count(test_inputs)
    253284        self._port.reset_preferences()
    254285
    … …  
    354385            if not tests_to_run_by_device[device_type]:
    355386                continue
    356             if not self._set_up_run(tests_to_run_by_device[device_type], device_type=device_type):
     387
     388            test_inputs = [self._test_input_for_file(test, device_type=device_type)
     389                           for test in tests_to_run_by_device[device_type]]
     390
     391            if not self._set_up_run(test_inputs, device_type=device_type):
    357392                return test_run_results.RunDetails(exit_code=-1)
    358393
    … …  
    360395            if not configuration.get('flavor', None):  # The --result-report-flavor argument should override wk1/wk2
    361396                configuration['flavor'] = 'wk2' if self._options.webkit_test_runner else 'wk1'
    362             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)
     397            temp_initial_results, temp_retry_results, temp_enabled_pixel_tests_in_retry = self._run_test_subset(test_inputs, device_type=device_type)
    363398
    364399            skipped_results = TestRunResults(self._expectations[device_type], len(aggregate_tests_to_skip))
    … …  
    429464
    430465    def _run_test_subset(self,
    431                          tests_to_run,  # type: List[Test]
     466                         test_inputs,  # type: List[TestInput]
    432467                         device_type,  # type: Optional[DeviceType]
    433468                         ):
    434469        try:
    435470            enabled_pixel_tests_in_retry = False
    436             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)
     471            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)
    437472
    438473            tests_to_retry = self._tests_to_retry(initial_results, include_crashes=self._port.should_retry_crashes())
    … …  
    441476            if retry_failures and tests_to_retry:
    442477                enabled_pixel_tests_in_retry = self._force_pixel_tests_if_needed()
     478                if enabled_pixel_tests_in_retry:
     479                    retry_test_inputs = [self._test_input_for_file(test_input.test, device_type=device_type)
     480                                         for test_input in test_inputs
     481                                         if test_input.test.test_path in tests_to_retry]
     482                else:
     483                    retry_test_inputs = [test_input
     484                                         for test_input in test_inputs
     485                                         if test_input.test.test_path in tests_to_retry]
    443486
    444487                _log.info('')
    445488                _log.info("Retrying %s ..." % pluralize(len(tests_to_retry), "unexpected failure"))
    446489                _log.info('')
    447                 retry_results = self._run_tests([test for test in tests_to_run if test.test_path in tests_to_retry],
     490                retry_results = self._run_tests(retry_test_inputs,
    448491                                                repeat_each=1,
    449492                                                iterations=1,
    … …  
    495538
    496539    def _run_tests(self,
    497                    tests_to_run,  # type: List[Test]
     540                   test_inputs,  # type: List[TestInput]
    498541                   repeat_each,  # type: int
    499542                   iterations,  # type: int
    … …  
    502545                   device_type,  # type: Optional[DeviceType]
    503546                   ):
    504         test_inputs = self._get_test_inputs(tests_to_run, repeat_each, iterations, device_type=device_type)
     547        new_test_inputs = self._multiply_test_inputs(test_inputs, repeat_each, iterations)
    505548
    506549        assert self._runner is not None
    507         return self._runner.run_tests(self._expectations[device_type], test_inputs, num_workers, retrying, device_type)
     550        return self._runner.run_tests(self._expectations[device_type], new_test_inputs, num_workers, retrying, device_type)
    508551
    509552    def _clean_up_run(self):

  • trunk/Tools/Scripts/webkitpy/layout_tests/models/test.py

    r275888 r276670  
    3131
    3232
    33 @attr.s(frozen=True)
     33@attr.s(frozen=True, slots=True)
    3434class Test(object):
    3535    """Data about a test and its expectations.

  • trunk/Tools/Scripts/webkitpy/layout_tests/models/test_input.py

    r275773 r276670  
    11# Copyright (C) 2010 Google Inc. All rights reserved.
    22# Copyright (C) 2010 Gabor Rapcsanyi (rgabor@inf.u-szeged.hu), University of Szeged
     3# Copyright (C) 2021 Apple Inc. All rights reserved.
    34#
    45# Redistribution and use in source and binary forms, with or without
    … …  
    2930
    3031
     32import attr
     33
    3134from .test import Test
    3235
     36
     37@attr.s(frozen=True, slots=True)
    3338class TestInput(object):
    3439    """Information about a test needed to run it.
    … …  
    3742    derived from TestExpectations/test execution options (e.g., timeout).
    3843    """
    39 
    40     def __init__(self,
    41                  test,  # type: Test
    42                  timeout=None,  # type: Union[None, int, str]
    43                  needs_servers=None,  # type: Optional[bool]
    44                  should_dump_jsconsolelog_in_stderr=None,  # type: Optional[bool]
    45                  ):
    46         # TestInput objects are normally constructed by the manager and passed
    47         # to the workers, but these some fields are set lazily in the workers where possible
    48         # because they require us to look at the filesystem and we want to be able to do that in parallel.
    49         self.test = test
    50         self.timeout = timeout  # in msecs; should rename this for consistency
    51         self.needs_servers = needs_servers
    52         self.should_dump_jsconsolelog_in_stderr = should_dump_jsconsolelog_in_stderr
    53         self.reference_files = None
     44    test = attr.ib(type=Test)
     45    timeout = attr.ib(default=None)  # type: Union[None, int, str]
     46    needs_servers = attr.ib(default=None)  # type: Optional[bool]
     47    should_dump_jsconsolelog_in_stderr = attr.ib(default=None)  # type: Optional[bool]
     48    reference_files = attr.ib(default=None)  # type: Optional[List[Tuple[str str]]]
     49    should_run_pixel_test = attr.ib(default=None)  # type: Optional[bool]
    5450
    5551    @property
    5652    def test_name(self):
    5753        return self.test.test_path
    58 
    59     def __repr__(self):
    60         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)

  • trunk/Tools/Scripts/webkitpy/port/base.py

    r276669 r276670  
    13621362        pass
    13631363
    1364     def should_run_as_pixel_test(self, test_input):
    1365         if not self._options.pixel_tests:
    1366             return False
    1367         if self._options.pixel_test_directories:
    1368             return any(test_input.test_name.startswith(directory) for directory in self._options.pixel_test_directories)
    1369         return self._should_run_as_pixel_test(test_input)
    1370 
    1371     def _should_run_as_pixel_test(self, test_input):
    1372         # Default behavior is to allow all test to run as pixel tests if --pixel-tests is on and
    1373         # --pixel-test-directory is not specified.
    1374         return True
    1375 
    13761364    def _in_flatpak_sandbox(self):
    13771365        return self._filesystem.exists("/.flatpak-info")
Note: See TracChangeset for help on using the changeset viewer.