Bug 174390 – Subclass Benchmark Runner script for future Web Driver support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Matthew Stewart]

Subclass Benchmark Runner script for future Web Driver support

[Matthew Stewart]

Created attachment 315164 [details]
Patch

[Stephanie Lewis]

Comment on attachment 315164 [details]
Patch

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

good job.  Just fix up webdriver and websocket and I think we'll be good.

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

Probably should dump an error message if this version of the function gets called.  Something about instantiating an abstract class

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

ditto

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

webdriver I think should be one word same for websocket

[Matthew Stewart]

Created attachment 315286 [details]
Patch

Comment on attachment 315286 [details]
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.

[Matthew Stewart]

Created attachment 315873 [details]
Patch

[Matthew Stewart]

Created attachment 315879 [details]
Patch

Comment on attachment 315879 [details]
Patch

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

> Tools/ChangeLog:6
> +        Reviewed by Dewei Zhu.

r=me.

[Matthew Stewart]

Created attachment 315958 [details]
Subclass Benchmark Runner script for WebDriver support

[WebKit Commit Bot]

Comment on attachment 315958 [details]
Subclass Benchmark Runner script for WebDriver support

Rejecting attachment 315958 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 315958, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Dewei Zhu found in /Volumes/Data/EWS/WebKit/Tools/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/4181285

[Matthew Stewart]

Created attachment 316334 [details]
patch

[WebKit Commit Bot]

Comment on attachment 316334 [details]
patch

Rejecting attachment 316334 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 316334, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
.webkit.org/git/WebKit
   e1d65cb..c668947  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 219851 = e1d65cbd37dbe81d09d4d06fec21583903fbd13e
r219852 = c66894799395d770494683b1519ab41f7e4b007b
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/4181637

[Matthew Stewart]

Created attachment 316344 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316344 [details]
Patch

Clearing flags on attachment: 316344

Committed r219857: <http://trac.webkit.org/changeset/219857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.