speculative fix for upload errors: stop using mechanize to upload to test-results.appspot.com
Created attachment 74608 [details] Patch
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
(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 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
Committed r72633: <http://trac.webkit.org/changeset/72633>
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.
(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.
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.