Bug 35812 – [new-run-webkit-tests][Chromium] split http_server into a new top level script plus a module

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

[Dirk Pranke]

Chromium's ui_test infrastructure uses the layout tests http_server infrastructure for some HTTP testing. At the moment it calls into webkitpy/layout_tests/port/http_server directly (as a __main__), but this is awkward, buggy on a mac, and has several layering violations.

I'm going to resolve this by creating a new top-level new-run-webkit-httpd script and call that instead, and make http_server just be a module.

The new script only works w/ Chromium's LigHTTP implementation, but should be made generic.

[Dirk Pranke]

Created attachment 50134 [details]
patch to split running http_server as a standalone script vs. an embedded library

[Eric Seidel (no email)]

LGTM, but I thought that Ojan had already made up a python version of run-webkit-httpd which knew how to talk to apache2?

I'm ready to r+ this as soon as we hear from Ojan.

[Eric Seidel (no email)]

Comment on attachment 50134 [details]
patch to split running http_server as a standalone script vs. an embedded library

Or rather, I'll r+ it, but I'd like Ojan to bless the patch before landing. :)

[Dirk Pranke]

(In reply to comment #2)
> LGTM, but I thought that Ojan had already made up a python version of
> run-webkit-httpd which knew how to talk to apache2?
> 
> I'm ready to r+ this as soon as we hear from Ojan.

If he does, I haven't seen it. However, the chromium test code actually has switches hard-coded into it for the LigHTTPd version (unfortunately), so I would like to address the whole apache/lighttpd thing separately from this patch.

[Ojan Vafai]

Comment on attachment 50134 [details]
patch to split running http_server as a standalone script vs. an embedded library

A couple nits on comments. The code looks fine. No need for another review before committing though.

> +++ b/WebKitTools/ChangeLog
> +        the Chromium tree. At some point this script should be made to
> +        work with Apache-based implementations and on other ports.

Could you file a bug for this and link to it in the ChangeLog description?

> +++ b/WebKitTools/Scripts/new-run-webkit-httpd
> +# TODO: currently this code only works with the Chromium ports and LigHTTPd.
s/TODO/FIXME

> +# It should be made to work on all ports.
> +#
> +# This script is also used by Chromium's ui_tests to run http layout tests
> +# in a browser.
I don't think this needs to be documented. It's not an explicit goal of the WebKit project to support Chromium's ui_test infrastructure. I don't think it's an issue in practice, but if WebKit needed this to change in a way that's incompatible with Chromium's needs, then it should be able to.

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 50134 [details])
> A couple nits on comments. The code looks fine. No need for another review
> before committing though.
> 
> > +++ b/WebKitTools/ChangeLog
> > +        the Chromium tree. At some point this script should be made to
> > +        work with Apache-based implementations and on other ports.
> 
> Could you file a bug for this and link to it in the ChangeLog description?
> 

Done. See bug 35820.

> > +++ b/WebKitTools/Scripts/new-run-webkit-httpd
> > +# TODO: currently this code only works with the Chromium ports and LigHTTPd.
> s/TODO/FIXME
> 

Grr. I thought FIXME was Chromium and TODO was WebKit. Guess I got it backwards.

> > +# It should be made to work on all ports.
> > +#
> > +# This script is also used by Chromium's ui_tests to run http layout tests
> > +# in a browser.
> I don't think this needs to be documented. It's not an explicit goal of the
> WebKit project to support Chromium's ui_test infrastructure. I don't think it's
> an issue in practice, but if WebKit needed this to change in a way that's
> incompatible with Chromium's needs, then it should be able to.

I agree that WebKit should be able to change things, but it doesn't hurt to have the dependency documented.

[Dirk Pranke]

Patch landed as r55602: http://trac.webkit.org/changeset/55602