[Webkit-unassigned] [Bug 174390] Subclass Benchmark Runner script for future Web Driver support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 17 17:28:19 PDT 2017
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>
More information about the webkit-unassigned
mailing list