[webkit-reviews] review requested: [Bug 73079] Web Inspector: chromium: I'd like to add a script for running perf tests for WebInspector. : [Attachment 116801] [patch] forth version

[bugzilla-daemon at webkit.org]

Ilya Tikhonovsky <loislo at chromium.org> has asked  for review:
Bug 73079: Web Inspector: chromium: I'd like to add a script for running perf
tests for WebInspector.
https://bugs.webkit.org/show_bug.cgi?id=73079

Attachment 116801: [patch] forth version
https://bugs.webkit.org/attachment.cgi?id=116801&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
(In reply to comment #9)
> (From update of attachment 116727 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=116727&action=review
> > 
> > Tools/Scripts/webkitpy/performance_tests/__init__.py:13
> > +# Keep this file free of any code or import statements that could
> > +# code in this file so that callers can opt-in as they want.  This also
> > +# allows different callers to choose different initialization code,
> > +# as necessary.
> 
> I'm not sure this big comment is necessary.  Most of our __init__.py files
only have the one line comment at the top.

done.

> 
> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:58
> > +	     self._port = port or
Host().port_factory.get(self._options.platform, self._options)
> 
> Eric should comment as to whether he's happy with this use of the Host()
constructor.
> 
> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:101
> > +		 print >> sys.stderr, "Build not up to date for " +
self._port._path_to_driver()
> 
> Why not use _log ?

done.

> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:113
> > +	     return unexpected_result_count
> 
> What is an unexpected result for a performance test?
> 
> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:135
> > +		 test_failed, driver_need_restart = self._run_single_test(test,
driver)
> > +		 if test_failed:
> > +		     unexpected_result_count = unexpected_result_count + 1
> > +		 else:
> > +		     expected_result_count = expected_result_count + 1
> 
> I see.  It's a pass/fail count.  Maybe that would be clearer than saying
expected/unexpected?

done.

> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:165
> > +		     elif not len(line) == 0:
> 
> The "len" isn't needed here, I don't think.

I'm checking that the test's output has nothing but RESULT.* lines and empty
lines.



(In reply to comment #10)
> (From update of attachment 116727 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=116727&action=review
> 
> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:42
> > +_perf_tests_base_dir = 'PerformanceTests'
> 
> Seems this should be moved onto the class too.

done.

> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:44
> > +_result_regex = re.compile('^RESULT .*$')
> 
> This should be moved onto the class.
> 
> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:49
> > +def run(perf_tests_dir, regular_output=sys.stderr,
buildbot_output=sys.stdout, port=None):
> > +	 runner = PerfTestsRunner(perf_tests_dir, regular_output,
buildbot_output, port)
> > +	 return runner.run()
> 
> Free functions are almost never right.

done.

> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:52
> > +class PerfTestsRunner:
> 
> All modern python objects should inherit from object.  So this should read
"class PerfTestsRunner(object):"

done.

> Also, normally files match the name of the class they contain.  Not always. 
But it seems that run_perf_tests is needlessly different from perftestrunner.py

> 
> >> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:58
> >> +	      self._port = port or
Host().port_factory.get(self._options.platform, self._options)
> > 
> > Eric should comment as to whether he's happy with this use of the Host()
constructor.
> 
> This is also an incomplete initialization of a Host object.  You'll end up
w/o a proper SCM.

done.

> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:87
> > +	     filesystem = self._port.filesystem
> 
> port.filesystem is going away.  it woudl be better to have a self.host.

done.

> > Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:94
> > +	 def run(self):
> > +
> 
> Leading space?

done.