Bug 92806 – nrwt: move actual test-running code into layout_test_runner.py

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: move actual test-running code into layout_test_runner.py

[Dirk Pranke]

Created attachment 155665 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 155665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155665&action=review

We've got so much yak!

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:87
> +        """Runs the tests.

This line is pretty much useless.

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:93
> +              typed Ctrl^C

Why are we indenting by 2 space here?

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:99
> +              of each thread with 'name', 'num_tests', 'total_time' properties
> +            test_timings is a list of timings for each sharded subdirectory
> +              of the form [time, directory_name, num_tests]
> +            individual_test_timings is a list of run times for each test
> +              in the form {filename:filename, test_run_time:test_run_time}

Ditto.

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:113
> +        self._all_results = []
> +        self._group_stats = {}
> +        self._worker_stats = {}
> +        self._has_http_lock = False
> +        self._remaining_locked_shards = []

Do we really need to re-initialize these variables again?

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:142
> +        def worker_factory(worker_connection):
> +            results_directory = self._results_directory
> +            if self._retrying:
> +                self._filesystem.maybe_make_directory(self._filesystem.join(self._results_directory, 'retries'))
> +                results_directory = self._filesystem.join(self._results_directory, 'retries')
> +            return Worker(worker_connection, results_directory, self._options)

It's probably better to make this is a private method given that this is a massive function already.

[Dirk Pranke]

Comment on attachment 155665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155665&action=review

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:87
>> +        """Runs the tests.
> 
> This line is pretty much useless.

will delete.

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:93
>> +              typed Ctrl^C
> 
> Why are we indenting by 2 space here?

no particular reason; this has been this way forever (probably going back to when we used two-space indents in the chromium repo). will fix.

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:99
>> +              in the form {filename:filename, test_run_time:test_run_time}
> 
> Ditto.

will fix.

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:113
>> +        self._remaining_locked_shards = []
> 
> Do we really need to re-initialize these variables again?

yes, because this routine gets called twice.

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:142
>> +            return Worker(worker_connection, results_directory, self._options)
> 
> It's probably better to make this is a private method given that this is a massive function already.

will do.

[Dirk Pranke]

Committed r124281: <http://trac.webkit.org/changeset/124281>