[webkit-reviews] review granted: [Bug 52048] commit-queue should know how to upload archived results (for test flakes or general failures) : [Attachment 78367] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 52048: commit-queue should know how to upload archived results (for test
flakes or general failures)
https://bugs.webkit.org/show_bug.cgi?id=52048

Attachment 78367: Patch
https://bugs.webkit.org/attachment.cgi?id=78367&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?