[webkit-reviews] review granted: [Bug 178059] Improve --help documentation and add --list-plans to show available benchmarks. : [Attachment 323122] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Oct 7 22:25:57 PDT 2017


Ryosuke Niwa <rniwa at webkit.org> has granted Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 178059: Improve --help documentation and add --list-plans to show available
benchmarks.
https://bugs.webkit.org/show_bug.cgi?id=178059

Attachment 323122: Patch

https://bugs.webkit.org/attachment.cgi?id=323122&action=review




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 323122
  --> https://bugs.webkit.org/attachment.cgi?id=323122
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64
> +    @staticmethod
> +    def available_plans():

I don't think this function belongs to benchmark_runner class. This class
shouldn't know anything about where plan files are located.
We should keep it in run_benchmark.py.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66
> +	   plans = [plan_file.replace(".plan", "") for plan_file in
os.listdir(plan_directory) if plan_file.endswith(".plan")]

We should continue to use os.path.splitext(plan_file)[0]
This code replaces any string of ".plan" in its name like
"x*.plan*ebench.plan".

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:88
> -	       raise Exception('Cant find any .plan file in directory %s' %
plandir)
> +	       raise Exception('Cant find any .plan file in directory %s' %
BenchmarkRunner.available_plans())

I don't think this change makes sense. This error messages is about not being
able to find any plans.
We should show the directory name instead.


More information about the webkit-reviews mailing list