Bug 60354 - Add a --skip-integrationtests mode to test-webkitpy
Summary: Add a --skip-integrationtests mode to test-webkitpy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-06 01:10 PDT by Adam Barth
Modified: 2011-05-06 13:04 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.56 KB, patch)
2011-05-06 01:13 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (69.64 KB, patch)
2011-05-06 12:11 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (69.63 KB, patch)
2011-05-06 12:12 PDT, Adam Barth
eric: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-05-06 01:10:10 PDT
Add a --fast mode to test-webkitpy
Comment 1 Adam Barth 2011-05-06 01:13:02 PDT
Created attachment 92559 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-05-06 09:25:58 PDT
Comment on attachment 92559 [details]
Patch

I guess.  Why don't we just kill the slow tests?  I guess that's what we're effectively doing here, since if folks start running --fast, the non-fast tests will go south.

This reminds me exactly of the "fast" directory in LayoutTests.  :)  Eventually everything wanted to be a fast/ test...

What constitutes a "slow" module?  How do you get on/off that list?
Comment 3 Adam Barth 2011-05-06 09:31:36 PDT
> I guess.  Why don't we just kill the slow tests?

I support that plan, but I suspect the folks that wrote those tests would be sad.

> I guess that's what we're effectively doing here, since if folks start running --fast, the non-fast tests will go south.

They're still run by the buildbot.

> This reminds me exactly of the "fast" directory in LayoutTests.  :)  Eventually everything wanted to be a fast/ test...
> 
> What constitutes a "slow" module?  How do you get on/off that list?

If test-webkitpy gets too slow (e.g., takes more than 10s), we continue to add things to the list until it is fast again.

Another approach is to refuse crapy tests that are slow.  There's no python unit tests should be slow.
Comment 4 Adam Barth 2011-05-06 09:32:17 PDT
> There's no python unit tests should be slow.

^^^ "There's no reason python ..."
Comment 5 Dirk Pranke 2011-05-06 10:34:22 PDT
I'm not a big fan of this change. I don't like doing anything that discourages people from running tests. We should just fix bug 60350 instead.

Those tests used to complete in a few seconds, so I suspect something simple just slipped in to cause the slowdown.

Additionally, in bug 59985 I added a way to mark tests as integration tests that weren't run by default. Most of the tests that are in this list are actually integration tests and should probably be marked like that anyway, so this patch is kinda redundant with that one.
Comment 6 Dirk Pranke 2011-05-06 11:09:21 PDT
In the grand scheme of things, 30 seconds isn't that long to wait for a set of tests to complete, so turning off "slow" tests to speed things up seems a bit misguided to me.

Some other things ideas:

1. If the goal is to speed up the EWS doing an entire test run, why not be smarter about which test suites need to run at all ? I.e., if there are no python modules modified, maybe skip test-webkitpy. If no perl tests, skip test-webkitperl, etc. If no changes to Source or LayoutTests, skip layout tests, etc.

2. Modify test-wekbitpy to run tests in parallel. This should be easy, since the tests are already designed to be run standalone.

3. Figure out which tests should be run based on the change and dependency analysis (granted, this one is a whole lot harder).
Comment 7 Adam Barth 2011-05-06 11:41:54 PDT
The goal is to speed up my development of Python.  30s is needlessly long.  4s is much more pleasant.
Comment 8 Adam Barth 2011-05-06 12:11:18 PDT
Created attachment 92620 [details]
Patch
Comment 9 Adam Barth 2011-05-06 12:12:11 PDT
Created attachment 92621 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-05-06 12:14:17 PDT
Comment on attachment 92620 [details]
Patch

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

LGTM.

> Tools/Scripts/webkitpy/test/main.py:40
> +    def _find_unittest_files(self, webkitpy_dir, suffix):

I would have renamed this _find_test_files.  Entertainingly python 2.7 has this implemented for us as TestDiscoverer. :)

> Tools/Scripts/webkitpy/test/main.py:127
> +        skip_integration_tests = False
> +        if len(sys_argv) > 1 and sys.argv[1] == "--skip-integrationtests":
> +            sys.argv.remove("--skip-integrationtests")

Some day we'll have real arg parsing... some day.
Comment 11 Eric Seidel (no email) 2011-05-06 12:14:59 PDT
Comment on attachment 92621 [details]
Patch

I would argue these could be off by default...
Comment 12 Adam Barth 2011-05-06 12:17:06 PDT
Committed r85969: <http://trac.webkit.org/changeset/85969>
Comment 13 Dirk Pranke 2011-05-06 12:18:30 PDT
Please do not submit this patch as is. 

run_webkit_tests_unittests contains tests that do a lot of different things. Also, my definition of "unit test" for purposes of distinguishing between unit and integration tests are test that are self contained and will neither affect or be affected by the environment. Not tests that are slow.

You are also recreating a similar but needlessly different mechanism from the thing I implemented in bug 59985. Can we just reuse that mechanism instead?
Comment 14 Adam Barth 2011-05-06 12:20:24 PDT
> You are also recreating a similar but needlessly different mechanism from the thing I implemented in bug 59985. Can we just reuse that mechanism instead?

The mechanism you created in bug 59985 doesn't work for this purpose.  It's insufficiently general and only works when running the tests individually.
Comment 15 Dirk Pranke 2011-05-06 13:00:33 PDT
(In reply to comment #14)
> > You are also recreating a similar but needlessly different mechanism from the thing I implemented in bug 59985. Can we just reuse that mechanism instead?
> 
> The mechanism you created in bug 59985 doesn't work for this purpose.  It's insufficiently general and only works when running the tests individually.

Well, yeah. The idea is to integrate that approach into test-webkitpy so that TestLoader is used for all of the tests.

Why do you think it is insufficiently general?
Comment 16 Adam Barth 2011-05-06 13:04:30 PDT
> Why do you think it is insufficiently general?

Because you didn't do that work and this approach was like four lines of code.