Bug 36063 - Upload test result json files to app engine server
Summary: Upload test result json files to app engine server
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Victor Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-12 10:18 PST by Victor Wang
Modified: 2010-05-08 12:37 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (9.64 KB, patch)
2010-03-12 10:31 PST, Victor Wang
abarth: review-
Details | Formatted Diff | Diff
Proposed patch (8.52 KB, patch)
2010-03-23 15:47 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Minor update to work with new autoinstall.py (9.27 KB, patch)
2010-04-01 09:55 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
update proposed patch (7.30 KB, patch)
2010-04-29 16:53 PDT, Victor Wang
eric: review-
Details | Formatted Diff | Diff
Proposed patch (7.55 KB, patch)
2010-05-04 15:35 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Fix styles (7.55 KB, patch)
2010-05-04 15:50 PDT, Victor Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 2010-03-12 10:18:59 PST
Update run_webkit_test.py to upload generated JSON files to app engine server. This is for Chromium flakiness dashboard.
Comment 1 Victor Wang 2010-03-12 10:31:59 PST
Created attachment 50608 [details]
Proposed patch

Hi Ojan / Nicolas,

Could you take a look at the patch? Thanks!
Comment 2 Adam Barth 2010-03-13 01:53:32 PST
Comment on attachment 50608 [details]
Proposed patch

Please use Mechanize to upload files to AppEngine instead of rolling your own HTTP client.  You can find examples in webkitpy/statusserver.py
Comment 3 Chris Jerdonek 2010-03-15 17:51:37 PDT
(In reply to comment #2)
> (From update of attachment 50608 [details])
> Please use Mechanize to upload files to AppEngine instead of rolling your own
> HTTP client.  You can find examples in webkitpy/statusserver.py

I believe this will make the script not work with Python 2.4 any more, which runs into concerns raised here:

https://bugs.webkit.org/show_bug.cgi?id=35584#c20

I'll try to re-post to webkit-dev tonight asking for clarification on whether we need to maintain 2.4 compatibility for scripts like this.  It would be good for everyone to be on the same page.
Comment 4 Victor Wang 2010-03-18 13:41:34 PDT
Chromium webkit test still requires python 2.4, how about use the current implementations for now and move to mechanize once chromium webkit test is ready for higher python version? I can file another bug to track the migration.
Comment 5 Adam Barth 2010-03-18 13:54:43 PDT
I don't think we should be writing our own HTTP client code.  Plenty of libraries exist for that.  Are there really none that work in Python 2.4?
Comment 6 Adam Barth 2010-03-19 10:17:58 PDT
Frabjous day!

On Fri, Mar 19, 2010 at 9:57 AM, Chris Jerdonek <cjerdonek@webkit.org> wrote:
> Mechanize (and ClientForm on which it depends) does work with Python 2.4:
>
> http://wwwsearch.sourceforge.net/mechanize/
>
> (See the section on compatibility.)
Comment 7 Victor Wang 2010-03-19 10:28:35 PDT
I did check the same page yesterday. The issue with mechanize is we are using autoinstall to bind it, and looks to me autoinstall.py is not compatible with python 2.4.

(In reply to comment #6)
> Frabjous day!
> 
> On Fri, Mar 19, 2010 at 9:57 AM, Chris Jerdonek <cjerdonek@webkit.org> wrote:
> > Mechanize (and ClientForm on which it depends) does work with Python 2.4:
> >
> > http://wwwsearch.sourceforge.net/mechanize/
> >
> > (See the section on compatibility.)
Comment 8 Victor Wang 2010-03-19 10:41:32 PDT
I also tried another thirdparty library but got an issue with multiple file uploads with AE and I am still investigating what's wrong. Maybe we should make autoinstall.py compatible with 2.4?

(In reply to comment #7)
> I did check the same page yesterday. The issue with mechanize is we are using
> autoinstall to bind it, and looks to me autoinstall.py is not compatible with
> python 2.4.
> 
> (In reply to comment #6)
> > Frabjous day!
> > 
> > On Fri, Mar 19, 2010 at 9:57 AM, Chris Jerdonek <cjerdonek@webkit.org> wrote:
> > > Mechanize (and ClientForm on which it depends) does work with Python 2.4:
> > >
> > > http://wwwsearch.sourceforge.net/mechanize/
> > >
> > > (See the section on compatibility.)
Comment 9 Chris Jerdonek 2010-03-19 10:43:49 PDT
(In reply to comment #7)
> I did check the same page yesterday. The issue with mechanize is we are using
> autoinstall to bind it, and looks to me autoinstall.py is not compatible with
> python 2.4.

I submitted a patch to get autoinstall.py and the rest of webkitpy working with Python 2.4, but it was rejected as WONTFIX:

https://bugs.webkit.org/show_bug.cgi?id=35584

I'd be happy to resubmit all or part of that patch within the next few days.  Alternatively, you can grab whatever you need from that patch to get this patch working.  From my recollection, the changes to autoinstall itself are minimal.
Comment 10 Victor Wang 2010-03-19 10:50:50 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > I did check the same page yesterday. The issue with mechanize is we are using
> > autoinstall to bind it, and looks to me autoinstall.py is not compatible with
> > python 2.4.
> 
> I submitted a patch to get autoinstall.py and the rest of webkitpy working with
> Python 2.4, but it was rejected as WONTFIX:
> 
> https://bugs.webkit.org/show_bug.cgi?id=35584
> 
> I'd be happy to resubmit all or part of that patch within the next few days. 
> Alternatively, you can grab whatever you need from that patch to get this patch
> working.  From my recollection, the changes to autoinstall itself are minimal.

This sounds good to me. I will grab your change in autoinstall.py. Thanks!
Comment 11 Ojan Vafai 2010-03-19 11:00:27 PDT
Look(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > I did check the same page yesterday. The issue with mechanize is we are using
> > > autoinstall to bind it, and looks to me autoinstall.py is not compatible with
> > > python 2.4.
> > 
> > I submitted a patch to get autoinstall.py and the rest of webkitpy working with
> > Python 2.4, but it was rejected as WONTFIX:
> > 
> > https://bugs.webkit.org/show_bug.cgi?id=35584
> > 
> > I'd be happy to resubmit all or part of that patch within the next few days. 
> > Alternatively, you can grab whatever you need from that patch to get this patch
> > working.  From my recollection, the changes to autoinstall itself are minimal.
> 
> This sounds good to me. I will grab your change in autoinstall.py. Thanks!

It sounds like the Chromium bots should be entirely on python 2.5+ by Monday. Lets just wait a couple days instead of adding more code. In the meantime, you can get a patch up and reviewed that works for python 2.5.
Comment 12 Victor Wang 2010-03-23 15:47:00 PDT
Created attachment 51464 [details]
Proposed patch

Attached is a new patch that uses mechanize.Browser to upload files.
This patch imports mechanize.Browser through autoinstall.py, which requires python 2.5 for now, so this patch CANNOT be committed until all chromium bots and chromium run_webkit_test shell scripts are upgraded. I will track the chromium status and commit when it is ready.
Comment 13 Eric Seidel (no email) 2010-03-26 14:28:20 PDT
Comment on attachment 51464 [details]
Proposed patch

Looks good.  I'm concerned about lack of testing, but I'm not sure how you'd test this all.

WebKit would like to use this sort of infrastructure as soon as it's ready.  I have code in webkit-patch to produce .json files from historical results.html files from http://build.webkit.org/results  Once the test results server is ready to actually process those .json files I'm happy to wire up things to upload them.
Comment 14 Victor Wang 2010-03-26 15:15:13 PDT
Thanks Eric!
test results server has been up and serving the chromium webkit test flakiness dashboard since last week. The app is also ready to host json files and serve them. Feel free to play with it and let me know your feedback. I will also start feed some real files to see how stable it is.

If you like, I could add you to the admin account for this AE. Here are the servers:
http://test-results-test.appspot.com/
TEST server: http://test-results-test.appspot.com/

(In reply to comment #13)
> (From update of attachment 51464 [details])
> Looks good.  I'm concerned about lack of testing, but I'm not sure how you'd
> test this all.
> 
> WebKit would like to use this sort of infrastructure as soon as it's ready.  I
> have code in webkit-patch to produce .json files from historical results.html
> files from http://build.webkit.org/results  Once the test results server is
> ready to actually process those .json files I'm happy to wire up things to
> upload them.
Comment 15 Victor Wang 2010-04-01 09:55:47 PDT
Created attachment 52304 [details]
Minor update to work with new autoinstall.py

A new version of autoinstall.py landed, update this patch to import the right package.

Still cannot land this patch as chromium bots are not ready on python 25 yet.
Comment 16 Adam Barth 2010-04-02 09:47:56 PDT
Comment on attachment 52304 [details]
Minor update to work with new autoinstall.py

Our experience with AppEngine is that you need to re-try writes because AppEngine sometimes responds with 500 errors.  If you look in the webkitpy.common.net package, you'll find a NetworkTransaction class that can help with this.  You can look at example code to see how it works.  In any case, that's a refinement we can do in a future patch.
Comment 17 Eric Seidel (no email) 2010-04-06 23:43:47 PDT
Comment on attachment 51464 [details]
Proposed patch

Cleared Eric Seidel's review+ from obsolete attachment 51464 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 18 Victor Wang 2010-04-29 16:53:05 PDT
Created attachment 54756 [details]
update proposed patch

-. Merge with latest code for preparing patch landing (Chromium bots are ready for python 2.5+).
-. Simplify the upload code as the AE replaces blobstore with multiple datastore entries for file storage. Blobstore is good for large file but not suitable for incremental merging / updating.
Comment 19 Eric Seidel (no email) 2010-05-02 19:17:24 PDT
Comment on attachment 52304 [details]
Minor update to work with new autoinstall.py

Cleared Adam Barth's review+ from obsolete attachment 52304 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 20 Adam Barth 2010-05-04 10:51:08 PDT
Comment on attachment 54756 [details]
update proposed patch

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:47
 +              self.browser.add_file(open(path, "rb"), "text/plain", filename,
I don't think this complies with our new unicode overloads.  Also, I think it leaks a file handle.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:63
 +    + LF
Is this correct?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:56
 +              self._upload_files(params, files)
Did you look at networktransaction?  Our experience has been that we need to retry writes to AppEngine.
Comment 21 Adam Barth 2010-05-04 10:51:35 PDT
Eric, can you look at the unicodeness of this patch?
Comment 22 Eric Seidel (no email) 2010-05-04 13:40:25 PDT
Comment on attachment 54756 [details]
update proposed patch

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1044
 +          for file in json_files:
This is simpler as:
files = [(file, os.path.join(self._options.results_directory, file)) for file in json_files]

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1054
 +              uploader.upload(attrs, files, 120)
The files array being an array of name/path pairs seems slightly strange.  Why not just paths?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:38
 +          self.browser = Browser()
Shouldn't this be self._browser?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:37
 +          self.url = "http://%s" % host
self._url?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:46
 +          for (filename, path) in files:
You can always get the name from os.path.basename(path) instead of having to pass it separately here.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:47
 +              self.browser.add_file(open(path, "rb"), "text/plain", filename,
To address Adam's question, yes this is unicode-safe.  ClientForm (used by Mechanize), excepts a file-like object returning str (byte array) objects.  So this is fine.  Are we sure they're text/plane though?  Do we need to give a more sophisticated mime type including the encoding?  JSON is always utf-8 iirc.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:41
 +          self.browser.open(self.url + "/testfile/uploadform")
%s is always better than +

r- for nits.
Comment 23 Victor Wang 2010-05-04 15:35:58 PDT
Created attachment 55060 [details]
Proposed patch
Comment 24 Victor Wang 2010-05-04 15:39:54 PDT
(In reply to comment #20)
> (From update of attachment 54756 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:47
>  +              self.browser.add_file(open(path, "rb"), "text/plain", filename,
> I don't think this complies with our new unicode overloads.  Also, I think it
> leaks a file handle.
Fixed the file handle issue. See Eric's comments for unicode...

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:63
>  +    + LF
> Is this correct?
I thought we should set svn:eol style for these python files and this should be same as other python files in the same dir, maybe I miss something?

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:56
>  +              self._upload_files(params, files)
> Did you look at networktransaction?  Our experience has been that we need to
> retry writes to AppEngine.
Done, with a shorter timeout.
Comment 25 WebKit Review Bot 2010-05-04 15:40:01 PDT
Attachment 55060 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:36:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:39:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:70:  blank line at end of file  [pep8/W391] [5]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Victor Wang 2010-05-04 15:44:14 PDT
(In reply to comment #22)
> (From update of attachment 54756 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1044
>  +          for file in json_files:
> This is simpler as:
> files = [(file, os.path.join(self._options.results_directory, file)) for file
> in json_files]
done

> 
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1054
>  +              uploader.upload(attrs, files, 120)
> The files array being an array of name/path pairs seems slightly strange.  Why
> not just paths?
I think we should allow the uploader to have filename uploaded to AE different from the actual filename in local machine. In this case, they are the same, but think they could be different. For example, local file could be FileXyz.json, where Xyz could be a build number or time, but when uploading it to AE, we may just want to name it File.json.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:38
>  +          self.browser = Browser()
> Shouldn't this be self._browser?
done

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:37
>  +          self.url = "http://%s" % host
> self._url?
done

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:46
>  +          for (filename, path) in files:
> You can always get the name from os.path.basename(path) instead of having to
> pass it separately here.
see comment above.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:47
>  +              self.browser.add_file(open(path, "rb"), "text/plain", filename,
> To address Adam's question, yes this is unicode-safe.  ClientForm (used by
> Mechanize), excepts a file-like object returning str (byte array) objects.  So
> this is fine.  Are we sure they're text/plane though?  Do we need to give a
> more sophisticated mime type including the encoding?  JSON is always utf-8
> iirc.
done

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:41
>  +          self.browser.open(self.url + "/testfile/uploadform")
> %s is always better than +
done

> 
> r- for nits.
Comment 27 Victor Wang 2010-05-04 15:50:33 PDT
Created attachment 55065 [details]
Fix styles
Comment 28 Adam Barth 2010-05-07 18:38:12 PDT
Comment on attachment 55065 [details]
Fix styles

Great!  Thanks.
Comment 29 WebKit Commit Bot 2010-05-08 12:36:53 PDT
Comment on attachment 55065 [details]
Fix styles

Clearing flags on attachment: 55065

Committed r59036: <http://trac.webkit.org/changeset/59036>
Comment 30 WebKit Commit Bot 2010-05-08 12:37:01 PDT
All reviewed patches have been landed.  Closing bug.