Bug 61315 - [nrwt] don't use NetworkTranslation when uploading results
Summary: [nrwt] don't use NetworkTranslation when uploading results
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: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-23 15:59 PDT by Tony Chang
Modified: 2011-05-24 23:53 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.96 KB, patch)
2011-05-23 16:01 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2011-05-23 15:59:34 PDT
[nrwt] don't use NetworkTranslation when uploading results
Comment 1 Tony Chang 2011-05-23 16:01:42 PDT
Created attachment 94509 [details]
Patch
Comment 2 Tony Chang 2011-05-23 16:04:11 PDT
There are a few other possible fixes:
a) Switch test_results_uploader.py to using mechanize (seems like it's not worth the effort)
b) Switch NetworkTransaction to catch urllib errors.  Maybe this would mean making a subclass that catches urllib and mechanize errors so we can uses a mechanize free version.
c) Move mechanize based error catching into the caller since it's a little strange that NetworkTransaction expects the caller to use mechanize.
Comment 3 Ojan Vafai 2011-05-23 16:22:44 PDT
Comment on attachment 94509 [details]
Patch

Seems fine.
Comment 4 Dirk Pranke 2011-05-23 17:26:15 PDT
So, there's a minor change here in that if the uploader gets a 500 from app engine, it will no longer retry the upload, right?

I probably would've gone for some combination of (b) or (c) since it seems like NetworkTransaction shouldn't depend on mechanize but otherwise provides a useful service, but this change is probably good enough as well. Maybe we should file a separate bug to remove the dependency on NetworkTransaction at some point (or add a FIXME)?
Comment 5 WebKit Commit Bot 2011-05-23 20:41:17 PDT
Comment on attachment 94509 [details]
Patch

Clearing flags on attachment: 94509

Committed r87124: <http://trac.webkit.org/changeset/87124>
Comment 6 WebKit Commit Bot 2011-05-23 20:41:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Tony Chang 2011-05-24 10:52:25 PDT
(In reply to comment #4)
> So, there's a minor change here in that if the uploader gets a 500 from app engine, it will no longer retry the upload, right?

How does the retry happen?  AFAICT, NetworkTransaction only catches mechanize.HTTPError and a 500 error from app engine here would raise a urllib2.HTTPError.
Comment 8 Dirk Pranke 2011-05-24 23:53:18 PDT
(In reply to comment #7)
> (In reply to comment #4)
> > So, there's a minor change here in that if the uploader gets a 500 from app engine, it will no longer retry the upload, right?
> 
> How does the retry happen?  AFAICT, NetworkTransaction only catches mechanize.HTTPError and a 500 error from app engine here would raise a urllib2.HTTPError.

Okay, lame. Turns out mechanize.HTTPError *is* (==) urllib2.HTTPError. It's just a renamed symbol (not even a subclass). If you raise mechanize.HTTPError, catch urllib2.HTTPError will catch it.

We should just change NetworkTransaction to catch urllib2.HTTPError. Note that the code is kind of broken anyway since the file isn't importing mechanize at all (unless that's somehow needed for auto_install magic?)