Bug 52048

Created attachment 78215 [details] wip

Created attachment 78367 [details] Patch

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

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

Created attachment 78388 [details] Now with workspace.py and much better testing

My new unit tests caught a whole bunch of typos. Looking for my gold star. Happy to go another round if needed.

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.

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

Created attachment 78503 [details] Patch for landing

Comment on attachment 78503 [details] Patch for landing Clearing flags on attachment: 78503 Committed r75480: <http://trac.webkit.org/changeset/75480>

All reviewed patches have been landed. Closing bug.

Committed r75520: <http://trac.webkit.org/changeset/75520>

Committed r75583: <http://trac.webkit.org/changeset/75583>