Bug 174390

Summary: Subclass Benchmark Runner script for future Web Driver support
Product: WebKit Reporter: Matthew Stewart <matthew_r_stewart>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, buildbot, commit-queue, dean_johnson, dewei_zhu, glenn, slewis
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 175186    
Bug Blocks: 174056, 174443    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Subclass Benchmark Runner script for WebDriver support
slewis: review+, commit-queue: commit-queue-
patch
slewis: review+, commit-queue: commit-queue-
Patch none

Description Matthew Stewart 2017-07-11 13:59:10 PDT
Subclass Benchmark Runner script for future Web Driver support
Comment 1 Matthew Stewart 2017-07-11 14:12:43 PDT
Created attachment 315164 [details]
Patch
Comment 2 Stephanie Lewis 2017-07-11 15:58:23 PDT
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
Comment 3 Matthew Stewart 2017-07-12 15:17:04 PDT
Created attachment 315286 [details]
Patch
Comment 4 dewei_zhu 2017-07-17 17:28:19 PDT
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.
Comment 5 Matthew Stewart 2017-07-18 18:56:20 PDT
Created attachment 315873 [details]
Patch
Comment 6 Matthew Stewart 2017-07-18 19:38:33 PDT
Created attachment 315879 [details]
Patch
Comment 7 dewei_zhu 2017-07-18 19:44:58 PDT
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.
Comment 8 Matthew Stewart 2017-07-19 16:07:53 PDT
Created attachment 315958 [details]
Subclass Benchmark Runner script for WebDriver support
Comment 9 WebKit Commit Bot 2017-07-24 17:14:52 PDT
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
Comment 10 Matthew Stewart 2017-07-24 18:01:38 PDT
Created attachment 316334 [details]
patch
Comment 11 WebKit Commit Bot 2017-07-24 18:42:15 PDT
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
Comment 12 Matthew Stewart 2017-07-24 20:27:33 PDT
Created attachment 316344 [details]
Patch
Comment 13 WebKit Commit Bot 2017-07-24 21:10:03 PDT
Comment on attachment 316344 [details]
Patch

Clearing flags on attachment: 316344

Committed r219857: <http://trac.webkit.org/changeset/219857>
Comment 14 WebKit Commit Bot 2017-07-24 21:10:05 PDT
All reviewed patches have been landed.  Closing bug.