Bug 178059 – Improve --help documentation and add --list-plans to show available benchmarks.

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

[Maciej Stachowiak]

Improve --help documentation and add --list-plans to show available benchmarks.

[Maciej Stachowiak]

Created attachment 323121 [details]
New output for --help and --list-plans

For ease of review, attachment showing what the new --help and --list-plams output looks like.

[Maciej Stachowiak]

Created attachment 323122 [details]
Patch

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Dewei, please take a second look at Maciej's patch to make sure landing this patch won't break bots.

[Maciej Stachowiak]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 323122 [details]
> 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.

I added it here because benchmark_runner already knows where plan files are located via  _find_plan_file(). I agree the knowledge should only be in one place, it seemed easier to put it in BenchmarkRunner than in run_benchmark.py. If you think it's a better direction for run_benchmark.py be the only place that knows about plan files instead, I can change things towards that direction instead. I put the code in benchmark_runner before I noticed that run_benchmark was already enumerating all plans.

> 
> > 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".

Will do.

> 
> > 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.

You're right, that makes no sense. Will revert.

[Ryosuke Niwa]

(In reply to Maciej Stachowiak from comment #5)
> (In reply to Ryosuke Niwa from comment #3)
> > Comment on attachment 323122 [details]
> > 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.
> 
> I added it here because benchmark_runner already knows where plan files are
> located via  _find_plan_file(). I agree the knowledge should only be in one
> place, it seemed easier to put it in BenchmarkRunner than in
> run_benchmark.py. If you think it's a better direction for run_benchmark.py
> be the only place that knows about plan files instead, I can change things
> towards that direction instead. I put the code in benchmark_runner before I
> noticed that run_benchmark was already enumerating all plans.

Oh, okay. I guess keeping it in benchmark_runner would be okay then.

[Maciej Stachowiak]

Committed r223341: <https://trac.webkit.org/changeset/223341>

[Radar WebKit Bug Importer]

<rdar://problem/35003097>