[Webkit-unassigned] [Bug 174390] Subclass Benchmark Runner script for future Web Driver support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174390

--- Comment #4 from dewei_zhu at apple.com ---
Comment on attachment 315286
  --> https://bugs.webkit.org/attachment.cgi?id=315286
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:62
> +        raise NotImplementedError('BenchmarkRunner is an abstract class and shouldn\'t be instantiated.')

unnecessary 'pass'?
Or would it make more sense to make it an abstractmethod?

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:102
> +        pass

It maybe cleaner to make execute function to be
with BenchmarkBuilder(self._plan_name, self._plan, self.name) as web_root:
    self._run_benchmark(int(self._plan['count']), web_root)

and add name = 'webdriver' on WebDriverBenchmarkRunner and 'websocket' on WebSocketBenchmarkRunner.

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:40
> +    parser.add_argument('--driver', dest='driver', default='websocket', choices=['webdriver', 'websocket'])

Maybe we could try something like
benchmark_runner_map = {
    WebDriverBenchmarkRunner.name: WebDriverBenchmarkRunner,
    WebSocketBenchmarkRunner.name: WebSocketBenchmarkRunner,
};

choices=benchmark_runner_map.keys()

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:96
> +    if args.driver == 'webdriver':
> +        runner = WebDriverBenchmarkRunner(args.plan, args.localCopy, args.countOverride, args.buildDir, args.output, args.platform, args.browser, args.scale_unit, args.device_id)
> +    else:
> +        runner = WebSocketBenchmarkRunner(args.plan, args.localCopy, args.countOverride, args.buildDir, args.output, args.platform, args.browser, args.scale_unit, args.device_id)

If we adopt 'benchmark_runner_map', then, there hat we can do is:
benchmark_runner_class = benchmark_runner_map[args.driver]
runner = benchmark_runner_class(args.plan, args.localCopy, args.countOverride, args.buildDir, args.output, args.platform, args.browser, args.scale_unit, args.device_id)

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:100
> +

Remove this line

> Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:8
> +from benchmark_runner import BenchmarkRunner
> +from benchmark_builder import BenchmarkBuilder
> +from selenium.webdriver.support.ui import WebDriverWait

nit, import should be alphabetical
Once execute is removed, BenchmarkBuilder is not needed in this file.

> Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:15
> +

name = 'webdriver'

> Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:23
> +            url = 'file://%s/%s/%s' % (web_root, self._plan_name, test_file)

I would recommend to use str.format.

> Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:34
> +    def execute(self):
> +        with BenchmarkBuilder(self._plan_name, self._plan, 'webdriver') as web_root:
> +            self._run_benchmark(int(self._plan['count']), web_root)

Not necessary anymore.

> Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:8
> +from benchmark_builder import BenchmarkBuilder

Once execute is removed, this should be cleaned up as well.

> Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:17
> +

add name = 'websocket'

> Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:21
> +        BenchmarkRunner.__init__(self, plan_file, local_copy, count_override, build_dir, output_file, platform, browser, scale_unit=True, device_id=None)

It would be better to use 'super(WebSocketBenchmarkRunner, self)' instead.

> Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:44
> +    def execute(self):
> +        with BenchmarkBuilder(self._plan_name, self._plan, 'websocket') as web_root:
> +            self._run_benchmark(int(self._plan['count']), web_root)

Not needed anymore.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170718/d3916556/attachment-0001.html>