Bug 76022 - META: make test-webkitpy more useful
Summary: META: make test-webkitpy more useful
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 76021 76142 76201 76233 76238 76765 77807 78185 78289 89725 91282 91289 91292 91297
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 20:00 PST by Dirk Pranke
Modified: 2012-07-16 14:21 PDT (History)
4 users (show)

See Also:


Attachments
roll-up patch based off of r121304 + att. 149665 on bug 89725 (54.92 KB, patch)
2012-06-26 19:15 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-01-10 20:00:41 PST
I'm planning on adding new features to test-webkitpy over the next week or two, and cleaning up the code to make it more hackable by people other than cjerdonek :).

Roughly, here's the set of features I have in mind:

1) proper command line help
2) better logging control
3) optional profiling info: per-test time, per-suite time, and actual cProfile data
4) optional code coverage info
5) support for directories and files as command line arguments in addition to test names
6) support for marking tests as fast or slow in addition to integration or unit
7) parallel test execution

This is a tracking bug for this work.
Comment 1 Adam Barth 2012-01-10 23:56:49 PST
Sounds great.
Comment 2 Dirk Pranke 2012-01-11 20:03:26 PST
logging control and command line help is in bug 76142.
Comment 3 Dirk Pranke 2012-01-12 12:29:46 PST
bug 76201 adds support for code coverage.
Comment 4 Dirk Pranke 2012-02-02 15:40:43 PST
Note: bug 77687 has a bunch of the stuff I'm planning to add, but it needs to be cleaned up and broken into landable chunks.
Comment 5 Dirk Pranke 2012-02-04 01:44:24 PST
bug 76765 adds support for filenames, directories, and packages as command line args.
Comment 6 Dirk Pranke 2012-02-04 01:45:42 PST
bug 77807 mostly cleans up the logging of individual test execution; the logging patterns now resemble new-run-webkit-tests (but are slightly improved over it, IMO).
Comment 7 Dirk Pranke 2012-02-08 17:06:45 PST
bug 78175 tracks parallel test execution.
Comment 8 Adam Barth 2012-02-08 17:34:23 PST
> 7) parallel test execution

I'm not sure (7) is worth doing.  I'd rather make the tests faster than run them in parallel.  Running the default set of tests takes less than a minute:

Ran 1306 tests in 53.808s

We have 6% of the tests marked as "integration tests", and running test-webkitpy --skip-integrationtests takes less than half the time:

Ran 1228 tests in 23.473s

Rather than add complexity by making the test suite parallel, it seems like breaking down these slow tests into more targeted unit tests has a better return on investment.
Comment 9 Dirk Pranke 2012-02-08 17:48:14 PST
(In reply to comment #8)
> > 7) parallel test execution
> 
> I'm not sure (7) is worth doing.  I'd rather make the tests faster than run them in parallel.  Running the default set of tests takes less than a minute:
> 
> Ran 1306 tests in 53.808s
> 
> We have 6% of the tests marked as "integration tests", and running test-webkitpy --skip-integrationtests takes less than half the time:
> 
> Ran 1228 tests in 23.473s
> 
> Rather than add complexity by making the test suite parallel, it seems like breaking down these slow tests into more targeted unit tests has a better return on investment.

I don't see this as an either/or thing. Making the tests parallelizable should be pretty easy (I'm most of the way there now), and is forcing me to clean up the messaging code. 

I agree that making the other tests faster would be good also. That said, as I noted in the other bug, there are other integration tests that we're currently not running because they are slow; I would like to enable them as well.
Comment 10 Adam Barth 2012-02-08 17:49:34 PST
Parallelizing has a cost, which is complexity.  I'm not sure we get enough of a benefit to justify the cost.
Comment 11 Dirk Pranke 2012-02-08 17:54:37 PST
(In reply to comment #10)
> Parallelizing has a cost, which is complexity.  I'm not sure we get enough of a benefit to justify the cost.

Sure, that is a fair concern. Of course, refactoring the test code to speed it up introduces complexity as well.

My hope is that once this code is cleaned up, adding parallelism will be dirt-simple and it'll be a no-brainer; doing so should only take me a little more time, so I'd prefer to defer further discussion over whether it will be worth it or not until I have something we can all stare at.

As I've said before, the changes I've posted thus far are (IMO) worth it even if we don't end up doing making test-webkitpy parallel, since they're making the code cleaner and easier to understand regardless. I'd like to keep the two issues separate as much as possible.
Comment 12 Adam Barth 2012-02-08 18:04:50 PST
Sounds like a good plan.
Comment 13 Dirk Pranke 2012-02-08 20:15:34 PST
Okay, I've got some preliminary data and patch which I will post to bug 78175 ... let's move all further discussion of the pros and cons of parallelization there.
Comment 14 Dirk Pranke 2012-02-08 20:16:51 PST
whoops, bug 78185, not bug 78175.
Comment 15 Dirk Pranke 2012-02-09 14:29:45 PST
As an aside, what was 53 seconds yesterday is now 61 seconds ... I wonder what the best way to watch for regressions here would be (or when we should care, given that a minute still isn't that long a time).
Comment 16 Dirk Pranke 2012-06-26 19:15:37 PDT
Created attachment 149666 [details]
roll-up patch based off of r121304 + att. 149665 on bug 89725

This is a roll-up patch that adds parallel execution, better logging, and general code cleanup/restructuring to test-webkitpy. It re-uses the manager_worker_broker logic in webkitpy.layout_tests; the overhead in lines of code for doing so is minimal. However, there's also not a huge win to running in parallel; execution time drops from 20s currently to ~5 seconds with 8 children.

Note that ~45 of the 1400 tests account for about half of the execution time, but o test takes longer than a second to complete (you can generate the list with -t/--timing).

The patch needs better tests, and depends on bug 89725. It also needs to be broken up into more reviewable chunks.
Comment 17 Dirk Pranke 2012-07-13 16:27:10 PDT
Okay, all patches posted, I think.
Comment 18 Dirk Pranke 2012-07-16 14:21:25 PDT
All of the work filed in comment #1 is now complete with the exception of per-suite time and cProfile data for profiling, and support for marking tests as fast or slow. test-webkitpy completes in 20 seconds run serially, and in 4 seconds when run w/ 8 threads.

I think this is good enough to close the bug.