Bug 52048

Summary: commit-queue should know how to upload archived results (for test flakes or general failures)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, commit-queue, koz, levin, mihaip, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 51371    
Attachments:
Description Flags
wip
none
Patch
none
Now with workspace.py and much better testing
none
Patch for landing none

Description Eric Seidel (no email) 2011-01-07 01:16:43 PST
commit-queue should know how to upload archived results (for test flakes or general failures)
Comment 1 Eric Seidel (no email) 2011-01-07 01:17:19 PST
Created attachment 78215 [details]
wip
Comment 2 Eric Seidel (no email) 2011-01-09 21:09:08 PST
Created attachment 78367 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-01-09 21:20:45 PST
The real benifit of this change is that it fixes normal -diffs.txt uploads.  They broke when we stopped the cq from reporting double-flakes (when two different flakes happened on two different runs), due to over-reporting of constant-failures as flakes.

Right now the cq does this:
1.  runs with patch.  sees failure.
2.  runs again with patch, sees no failure.
3.  reports test as flaky.  -- however the second run *cleared* the layout-test-results, and the upload of the diffs file fails!

With this patch, we archive the results and pull the -diffs.txt from the archive.  Since I'd taught the queue how to archive, If igured we might as well upload the archive too.  Hence this patch.
Comment 4 Adam Barth 2011-01-10 01:54:50 PST
Comment on attachment 78367 [details]
Patch

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

This is fine, but not really your best patch.  You're putting too much detailed code directly in the queue instead of keeping the queue as a controller that orchestrates the process.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:449
> +        if not mimetype:
> +            mimetype = "text/plain"  # Bugzilla might auto-guess for us and we might not need this?

Sad face, but ok.

> Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py:166
> +            results_diff = results_archive.read_binary_file(results_archive.filename)

Lame.  We should be able to use a file object or a file handle without having to assume the file system hasn't changed.

> Tools/Scripts/webkitpy/tool/commands/queues.py:323
> +    # FIXME: This does not really belong on filesystem, because it uses filesystem
> +    # however it doesn't really belong here either.  We need a new abstaction.
> +    def _find_unused_filename(self, directory, name, extension, search_limit=10):

This isn't a great place for this function.  It's way too high-level.  Maybe something like Checkout to wrap filesystem?

> Tools/Scripts/webkitpy/tool/commands/queues.py:343
> +        zip_command = ['zip', zip_path, results_directory]

Does this work on windows?  Probably this whole thing doesn't work on Windows.  I feel like you're just dumping stuff into this class because it's here.  Is it really too much to ask to have a ZipFile class that abstracts this junk?
Comment 5 Eric Seidel (no email) 2011-01-10 02:03:46 PST
Comment on attachment 78367 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:449
>> +            mimetype = "text/plain"  # Bugzilla might auto-guess for us and we might not need this?
> 
> Sad face, but ok.

I'm not sure what you mean.  This code is here to keep compatibility with existing code using this function and assuming that the mimetype was always text/plain.

>> Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py:166
>> +            results_diff = results_archive.read_binary_file(results_archive.filename)
> 
> Lame.  We should be able to use a file object or a file handle without having to assume the file system hasn't changed.

I'm not sure what you mean.

I'm passing around a ZipFile object because that allows for easier mocking than passing around a zip archive path and making a File or ZipFile when needed.

I guess we could pass around a normal file object and make a ZipFile from it only when necessary.
http://docs.python.org/library/zipfile.html

>> Tools/Scripts/webkitpy/tool/commands/queues.py:323
>> +    def _find_unused_filename(self, directory, name, extension, search_limit=10):
> 
> This isn't a great place for this function.  It's way too high-level.  Maybe something like Checkout to wrap filesystem?

Agreed.  That's exactly what I said in my FIXME. :)  On mac/cocoa, this type of thing would go on NSWorkspace instead of NSFileManager (I think that's right).  We probably want some sort of concept like that.  I could build it for this patch, that's not an unreasonable request.

>> Tools/Scripts/webkitpy/tool/commands/queues.py:343
>> +        zip_command = ['zip', zip_path, results_directory]
> 
> Does this work on windows?  Probably this whole thing doesn't work on Windows.  I feel like you're just dumping stuff into this class because it's here.  Is it really too much to ask to have a ZipFile class that abstracts this junk?

So there already is a ZipFile class built-into python. :)  The APIs it has for creating .zip archives are rather low level, and I worry that I'll get it wrong.  You have to os.walk a directory yourself, and add each os.path.join'd path to the archive yourself.  There is also the question of compression methods, as well as file path string encodings.  I figured that "zip" would just do the right thing.

No, this would not work on win32 (but I don't think webkitpy is supposed to anyway?)

I could write some ZipFile-based .zip creation in python code.  Again, I'm just worried we're going to get it wrong. :)  (There is also another question of where that stuff goes.  Probably in this new "Workspace" object or whatever.
Comment 6 Eric Seidel (no email) 2011-01-10 02:07:26 PST
(In reply to comment #5)
> > Lame.  We should be able to use a file object or a file handle without having to assume the file system hasn't changed.

Turns out ZipFile has an (undocumented) public member ".fp" which is the File object.  I can just grab that.
Comment 7 Eric Seidel (no email) 2011-01-10 02:57:39 PST
Created attachment 78388 [details]
Now with workspace.py and much better testing
Comment 8 Eric Seidel (no email) 2011-01-10 02:58:22 PST
My new unit tests caught a whole bunch of typos.

Looking for my gold star.  Happy to go another round if needed.
Comment 9 Mihai Parparita 2011-01-10 11:42:03 PST
Comment on attachment 78388 [details]
Now with workspace.py and much better testing

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

> Tools/Scripts/webkitpy/common/system/workspace.py:50
> +    def create_zip(self, zip_path, source_path):

FWIW, chromium has MakeZip:

https://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/tools/build/scripts/common/chromium_utils.py&q=MakeZip%20file:chromium_utils&exact_package=chromium&l=330

It still uses the zip command-line on Mac/Linux.
Comment 10 Eric Seidel (no email) 2011-01-10 11:46:47 PST
(In reply to comment #9)
> (From update of attachment 78388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78388&action=review
> 
> > Tools/Scripts/webkitpy/common/system/workspace.py:50
> > +    def create_zip(self, zip_path, source_path):
> 
> FWIW, chromium has MakeZip:
> 
> https://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/tools/build/scripts/common/chromium_utils.py&q=MakeZip%20file:chromium_utils&exact_package=chromium&l=330
> 
> It still uses the zip command-line on Mac/Linux.

Thanks.  That's useful (and proves my point that if I wrote this in python I'd get it wrong). :)

Going to just stick with things as-is.  We can add our own MakeZip if needed for win32.
Comment 11 Adam Barth 2011-01-11 00:58:30 PST
Comment on attachment 78388 [details]
Now with workspace.py and much better testing

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

Thanks for iterating on this patch.

> Tools/Scripts/webkitpy/common/system/workspace_unittest.py:37
> +    def test_find_unused_filename(self):

No test for make_zip?
Comment 12 Eric Seidel (no email) 2011-01-11 01:23:18 PST
Created attachment 78503 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2011-01-11 02:37:15 PST
Comment on attachment 78503 [details]
Patch for landing

Clearing flags on attachment: 78503

Committed r75480: <http://trac.webkit.org/changeset/75480>
Comment 14 WebKit Commit Bot 2011-01-11 02:37:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Eric Seidel (no email) 2011-01-11 11:28:51 PST
Committed r75520: <http://trac.webkit.org/changeset/75520>
Comment 16 Eric Seidel (no email) 2011-01-11 19:21:03 PST
Committed r75583: <http://trac.webkit.org/changeset/75583>