Bug 60354 – Add a --skip-integrationtests mode to test-webkitpy

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

[Adam Barth]

Add a --fast mode to test-webkitpy

[Adam Barth]

Created attachment 92559 [details]
Patch

[Eric Seidel (no email)]

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?

[Adam Barth]

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

[Adam Barth]

> There's no python unit tests should be slow.

^^^ "There's no reason python ..."

[Dirk Pranke]

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.

[Dirk Pranke]

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

[Adam Barth]

The goal is to speed up my development of Python.  30s is needlessly long.  4s is much more pleasant.

[Adam Barth]

Created attachment 92620 [details]
Patch

[Adam Barth]

Created attachment 92621 [details]
Patch

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 92621 [details]
Patch

I would argue these could be off by default...

[Adam Barth]

Committed r85969: <http://trac.webkit.org/changeset/85969>

[Dirk Pranke]

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?

[Adam Barth]

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

[Dirk Pranke]

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

[Adam Barth]

> Why do you think it is insufficiently general?

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