Bug 36063 – Upload test result json files to app engine server

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Victor Wang]

Update run_webkit_test.py to upload generated JSON files to app engine server. This is for Chromium flakiness dashboard.

[Victor Wang]

Created attachment 50608 [details]
Proposed patch

Hi Ojan / Nicolas,

Could you take a look at the patch? Thanks!

[Adam Barth]

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

[Chris Jerdonek]

(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.

[Victor Wang]

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.

[Adam Barth]

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?

[Adam Barth]

Frabjous day!

On Fri, Mar 19, 2010 at 9:57 AM, Chris Jerdonek <[e-mail]> wrote:
> Mechanize (and ClientForm on which it depends) does work with Python 2.4:
>
> http://wwwsearch.sourceforge.net/mechanize/
>
> (See the section on compatibility.)

[Victor Wang]

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 <[e-mail]> wrote:
> > Mechanize (and ClientForm on which it depends) does work with Python 2.4:
> >
> > http://wwwsearch.sourceforge.net/mechanize/
> >
> > (See the section on compatibility.)

[Victor Wang]

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 <[e-mail]> wrote:
> > > Mechanize (and ClientForm on which it depends) does work with Python 2.4:
> > >
> > > http://wwwsearch.sourceforge.net/mechanize/
> > >
> > > (See the section on compatibility.)

[Chris Jerdonek]

(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.

[Victor Wang]

(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!

[Ojan Vafai]

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.

[Victor Wang]

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.

[Eric Seidel (no email)]

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.

[Victor Wang]

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.

[Victor Wang]

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.

[Adam Barth]

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.

[Eric Seidel (no email)]

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.

[Victor Wang]

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.

[Eric Seidel (no email)]

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.

[Adam Barth]

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.

[Adam Barth]

Eric, can you look at the unicodeness of this patch?

[Eric Seidel (no email)]

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.

[Victor Wang]

Created attachment 55060 [details]
Proposed patch

[Victor Wang]

(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.

[WebKit Review Bot]

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.

[Victor Wang]

(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.

[Victor Wang]

Created attachment 55065 [details]
Fix styles

[Adam Barth]

Comment on attachment 55065 [details]
Fix styles

Great!  Thanks.

[WebKit Commit Bot]

Comment on attachment 55065 [details]
Fix styles

Clearing flags on attachment: 55065

Committed r59036: <http://trac.webkit.org/changeset/59036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.