Bug 73079 – Web Inspector: chromium: I'd like to add a script for running perf tests for WebInspector.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ilya Tikhonovsky]

The idea is to have performance tests for WebInspector.
I was suggested to put these tests into PerformanceTests/inspector.
They produce output in a format that is suitable for chromium perf bot drawing scripts.

I'd like to reuse code for scanning folders for test files.
Because of the fact that original test_files.py has common and layout specific parts
I decided to extract the generic part and put it into webkitpy/common
I think the two packages with name test_files look strange but I have no idea about a better name.

[Ilya Tikhonovsky]

Created attachment 116505 [details]
[patch] initial version

[Adam Barth]

Comment on attachment 116505 [details]
[patch] initial version

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

> Tools/Scripts/run-devtools-perf-test.py:73
> +def main(regular_output=sys.stderr, buildbot_output=sys.stdout):

This main is way too big.  Main should be a tiny function that calls into a class that has tests.

[Adam Barth]

Comment on attachment 116505 [details]
[patch] initial version

The wrapper script should be small and should call into a class in webkitpy to do the real work.  The class in webkitpy should probably be in a new top-level package for performance tests.  We'll also need unit tests for the new class.

[Ilya Tikhonovsky]

Created attachment 116654 [details]
[patch] second iteration

comments addressed

[Eric Seidel (no email)]

Comment on attachment 116654 [details]
[patch] second iteration

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

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:56
> +def parse_args(args=None):
> +    print_options = printing.print_options()

Why not just put this on PerfTestsRunner?  Free functions are much harder to mock.

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests_unittest.py:43
> +def create_driver():
> +    class TestDriver:
> +

Why not put these on the TestCase?

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests_unittest.py:100
> +    def test_run_passing_test(self):
> +        runner = create_runner()
> +        driver = create_driver()
> +        test_failed, driver_need_restart = runner._run_single_test('pass.html', driver)
> +        self.assertFalse(test_failed)
> +        self.assertFalse(driver_need_restart)
> +
> +    def test_run_silent_test(self):
> +        runner = create_runner()
> +        driver = create_driver()
> +        test_failed, driver_need_restart = runner._run_single_test('silent.html', driver)
> +        self.assertTrue(test_failed)
> +        self.assertFalse(driver_need_restart)

Looks like copy/paste code.  Can we use a helper?

[Ilya Tikhonovsky]

Created attachment 116725 [details]
[patch] third version

comments addressed

[WebKit Review Bot]

Attachment 116725 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/run-insp..." exit_code: 1

Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:82:  too many blank lines (2)  [pep8/E303] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Tikhonovsky]

Created attachment 116727 [details]
[patch] third version. style fix.

[Adam Barth]

Comment on attachment 116727 [details]
[patch] third version. style fix.

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

This generally looks good.  We should like Eric take a look though.  We're not super happy with some of the decisions NRWT makes in how it sets up its harness.  Hopefully Eric will comment on whether you've avoided the problems we're trying to fix in that aspect of NRWT.

Thanks!

> Tools/Scripts/webkitpy/performance_tests/__init__.py:13
> +# Keep this file free of any code or import statements that could
> +# cause either an error to occur or a log message to be logged.
> +# This ensures that calling code can import initialization code from
> +# webkitpy before any errors or log messages due to code in this file.
> +# Initialization code can include things like version-checking code and
> +# logging configuration code.
> +#
> +# We do not execute any version-checking code or logging configuration
> +# 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.

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

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

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

[Eric Seidel (no email)]

Comment on attachment 116727 [details]
[patch] third version. style fix.

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.

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

> 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):"

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.

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

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

Leading space?

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

Or _printer?  Any time we're grabbing at sys.* directly we're making our code less-mockable.

[Ilya Tikhonovsky]

Created attachment 116801 [details]
[patch] forth version

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

[Dirk Pranke]

Comment on attachment 116801 [details]
[patch] forth version

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

The patch looks close to me, just a few comments ...

> Tools/Scripts/webkitpy/common/test_files.py:35
> +under that directory."""

If this file is going to be re-used by both layout tests and perf tests, you need to update this docstring. I would be tempted to rename it to something other than "test_files.py" (maybe find_files.py? That's not very good, either).

> Tools/Scripts/webkitpy/common/test_files.py:52
> +          everything under the base_dir.

Add skipped_directories, file_filter to the docstring.

> Tools/Scripts/webkitpy/layout_tests/port/test_files.py:36
>  

I would probably merge all of the remaining code in this file back into port/base.py, and delete this file. That will solve your "two files with the same name" problem and make the layering clearer, I think.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:54
> +        self._host._initialize_scm()

Question for Eric ... should this be a public function? Is it supposed to be something that is optional, or should this be called as part of the constructor?

[Dirk Pranke]

(In reply to comment #12)
> 
> > Tools/Scripts/webkitpy/layout_tests/port/test_files.py:36
> >  
> 
> I would probably merge all of the remaining code in this file back into port/base.py, and delete this file. That will solve your "two files with the same name" problem and make the layering clearer, I think.
> 

I should have mentioned that it may be that test_files() shouldn't be a method on port at all, and we should lift this up into manager.py or something else, but I wouldn't do that as part of this change.

[Ilya Tikhonovsky]

Created attachment 116910 [details]
[patch] next iteration.

(In reply to comment #12)
> (From update of attachment 116801 [details])
> 
> > Tools/Scripts/webkitpy/common/test_files.py:35
> > +under that directory."""
> 
> If this file is going to be re-used by both layout tests and perf tests, you need to update this docstring. I would be tempted to rename it to something other than "test_files.py" (maybe find_files.py? That's not very good, either).

renamed to find_files.py

> 
> > Tools/Scripts/webkitpy/common/test_files.py:52
> > +          everything under the base_dir.
> 
> Add skipped_directories, file_filter to the docstring.

done

> 
> > Tools/Scripts/webkitpy/layout_tests/port/test_files.py:36
> >  
> 
> I would probably merge all of the remaining code in this file back into port/base.py, and delete this file. That will solve your "two files with the same name" problem and make the layering clearer, I think.
> 
> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:54
> > +        self._host._initialize_scm()
> 
> Question for Eric ... should this be a public function? Is it supposed to be something that is optional, or should this be called as part of the constructor?

[Dirk Pranke]

Comment on attachment 116910 [details]
[patch] next iteration.

(In reply to comment #14)
> > I would probably merge all of the remaining code in this file back into port/base.py, and delete this file. That will solve your "two files with the same name" problem and make the layering clearer, I think.

I think you missed this comment, maybe? We should delete port/test_files.py and merge whatever code is needed into base/Port.py

[Ilya Tikhonovsky]

(In reply to comment #15)
> (From update of attachment 116910 [details])
> (In reply to comment #14)
> > > I would probably merge all of the remaining code in this file back into port/base.py, and delete this file. That will solve your "two files with the same name" problem and make the layering clearer, I think.
> 
> I think you missed this comment, maybe? We should delete port/test_files.py and merge whatever code is needed into base/Port.py

I thought It'd be simpler do not merge it into port but move it into manager in another patch as you suggested in the last comment. There is no problem with names because I renamed common/test_files to common/find_files.

[Ilya Tikhonovsky]

Created attachment 117174 [details]
[patch] next iteration.

(In reply to comment #15)
> (From update of attachment 116910 [details])
> (In reply to comment #14)
> > > I would probably merge all of the remaining code in this file back into port/base.py, and delete this file. That will solve your "two files with the same name" problem and make the layering clearer, I think.
> 
> I think you missed this comment, maybe? We should delete port/test_files.py and merge whatever code is needed into base/Port.py

done.

webkitpy/layout_test/port/test_files.py content was moved into webkitpy/layout_test/port/base.py
the tests for test_files were moved to base_unittests.py

[Dirk Pranke]

Comment on attachment 117174 [details]
[patch] next iteration.

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

Sorry for the confusion earlier. It's not clear to me that getting rid of port.tests() and just doing this generically in manager.py is actually the right thing to do, but I definitely didn't want a port/test_files.py to keep existing, which is why I wanted you to inline it for now.

> Tools/Scripts/run-inspector-perf-tests.py:42
> +    sys.exit(PerfTestsRunner(_perf_tests_dir).run())

Nit: I'd probably just hard-code 'inspector' for now and remove the variable if it isn't needed elsewhere, but it's up to you.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:470
> +        return find_files.find(self.filesystem, self.layout_tests_dir(), paths, skipped_directories, _is_test_file)

Please rename this to be _find_test_files() to make it clear that this is a protected function, not a public function (see the comment below in rebaseline.py as well; if this has to stay as a public function, that's fine).


> Tools/Scripts/webkitpy/layout_tests/port/base.py:1020
> +    return _has_supported_extension(filesystem, filename) and not is_reference_html_file(filename)

Can you make _supported_file_extensions be a local variable and move all of these functions next to tests() since they're related?

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:121
> +        for test_name in map(self._to_test_name, self._port.find_test_files(args)):

Does this work if it's just self._port.tests(args)? It seems like this shouldn't be calling find_test_files (or, previously, test_files.find()) directly.

[Ilya Tikhonovsky]

Committed r101618: <http://trac.webkit.org/changeset/101618>