Bug 49944

Summary: speculative fix for upload errors: stop using mechanize to upload to test-results.appspot.com
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: New BugsAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, tony, victorw
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch tony: review+

Description Ojan Vafai 2010-11-22 15:49:26 PST
speculative fix for upload errors: stop using mechanize to upload to test-results.appspot.com
Comment 1 Ojan Vafai 2010-11-22 15:54:00 PST
Created attachment 74608 [details]
Patch
Comment 2 Adam Barth 2010-11-22 16:12:58 PST
Comment on attachment 74608 [details]
Patch

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

Do we really want to be implementing HTTP in Python?  Surely there's a library to do that for us.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:44
> +def _encode_multipart_form_data(fields, files):
> +    """Encode form fields for multipart/form-data.

Yuck
Comment 3 Ojan Vafai 2010-11-22 16:40:12 PST
(In reply to comment #2)
> (From update of attachment 74608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74608&action=review
> 
> Do we really want to be implementing HTTP in Python?  Surely there's a library to do that for us.

You mean the encoding or using urllib2 directly? If the former, I don't know of a library to do that. If the latter, I'm not really sure what the benefit of a layer of indirection is.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:44
> > +def _encode_multipart_form_data(fields, files):
> > +    """Encode form fields for multipart/form-data.
> 
> Yuck

Open to suggestions. This is what rietveld's upload code uses, so at least it's well tested.
Comment 4 Tony Chang 2010-11-23 13:54:20 PST
Comment on attachment 74608 [details]
Patch

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

Seems OK to try, but I'm skeptical mechanize itself is the problem.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:60
> +    for (key, value) in fields:

Nit: no ()s

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:68
> +    for (key, filename, value) in files:

Nit: no ()s

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:97
> +        for (filename, path) in files:

Nit: no ()s
Comment 5 Ojan Vafai 2010-11-23 15:00:15 PST
Committed r72633: <http://trac.webkit.org/changeset/72633>
Comment 6 Eric Seidel (no email) 2010-11-23 16:50:00 PST
Um... this seems a bad idea. This is mechanizes job. Not ours. Do we actually understand the bug?  It seems it should be fixed in mechanize or gae or a wrapper around mechanize.

I would have r-ed this.
Comment 7 Ojan Vafai 2010-11-24 10:02:36 PST
(In reply to comment #6)
> Um... this seems a bad idea. This is mechanizes job. Not ours.

I don't really understand what mechanize buys us in this case. Here are the downsides of using mechanize:
-It uses a GET and a POST, where a POST is sufficient here.
-It adds a layer of indirection between the application logic and urllib2.

I see the value of using mechanize for bugzilla GETs. It saves us the work of parsing the bugzilla html. I don't see the benefit of using it for POSTs though. To me it just makes for more complicated code that's hard to debug and, in some cases, is slower because you need to do an extra GET.

>Do we actually understand the bug?

No. What I was seeing from appengine logs is that it would think the uploaded files were empty. From the buildbot side, there was definitely a non-empty file on disk. 

FWIW, this did seem to fix the issue. There is still a timeout issue on appengine, but the empty files issue seems to have gone away.

>It seems it should be fixed in mechanize or gae or a wrapper around mechanize.

Maybe.
Comment 8 Adam Barth 2010-11-24 11:20:39 PST
I think the philosophy here is that webkitpy shouldn't know anything about HTTP.  That knowledge should be encapsulated in some library or another.  If we can't find a library that understands enough about HTTP to do this job, then we should write such a library ourselves, but we shouldn't conflate knowledge of HTTP into an object that knows about uploading LayoutTest results.  That's a big layering violation.