[Webkit-unassigned] [Bug 50098] New webkit-patch rebaseline2 command.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 29 10:18:06 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=50098
Adam Barth <abarth at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #74983|review? |review-
Flag| |
--- Comment #6 from Adam Barth <abarth at webkit.org> 2010-11-29 10:18:05 PST ---
(From update of attachment 74983)
View in context: https://bugs.webkit.org/attachment.cgi?id=74983&action=review
This looks cool, but you're re-inventing a bunch of wheels instead of making the existing wheels more awesome. Also, you've siloed a bunch of your code in a separate package instead of putting it in the right place in the package hierarchy where it can be used by other parts of webkitpy.
In general, the effectiveness of webkitpy should grow like n^2. The way you've written this patch, the effectiveness is only growing like n because you're not making use of the network effects of sharing code.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/all_tests.py:7
> +unittest.main()
We have some nifty unittest auto-discovery code. Maybe we should use that here? It's a pay to register new unit tests with the harness.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/bucket.py:209
> + def b(self, bucket_name, *args):
This is kind of a short function name. Can we use something more descriptive?
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:34
> +class RemoteZip(object):
This class seems general and should be in webkitpy.common.net
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:42
> + def _load(self):
> + if self._zip_file is None:
> + self._zip_file = zipfile.ZipFile(urllib.urlretrieve(self._zip_url)[0])
Consider using NetworkTransaction to be robust to network connectivity issues.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:63
> +class ZipFileHandle:
same here.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:100
> +class DirectoryAsZip(object):
Also a general class that should be in webkit.common somewhere.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:121
> + f = open(os.path.join(self._path, filename))
> + contents = f.read()
> + f.close
We generally use "with" to avoid leaking file handles. Also, we have a FileSystem class we use to abstract these operations. Notice that you haven't specified a codec, which is something that FileSystem enforces.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:135
> + def delete(self, filename):
> + filename = os.path.join(self._path, filename)
> + os.remove(filename)
This seems like a security vulnerability waiting to happen... Maybe some sort of requirement that the final filename is actually a subdirectory of self._path?
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:139
> +class Builder(object):
> + """Retrieves results from zip files stored on a buildbot builder"""
We already have an abstraction for a BuildBot. Presumably we should add this functionality to that abstraction instead of creating a new abstraction.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:168
> + def results_for(self, name, target_type=None):
We already have code that does this for the non-zip case. We should merge this functionality into that code instead of having two silos.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:209
> +class AggregateBuilder(object):
This belongs in the buildbot package somewhere.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:234
> + return 'http://build.chromium.org/f/chromium/layout_test_results/' + builder_name + '/layout-test-results.zip'
General webkitpy code shouldn't refer to build.chromium.org. You probably need to add this to the port abstraction.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:264
> +class WebKitBuilder(object):
> + BUILDERS = {
> + 'mac-snowleopard': 'SnowLeopard%20Intel%20Release%20(Tests)',
> + 'mac-leopard': 'Leopard%20Intel%20Debug%20(Tests)',
> + 'win': 'Windows%207%20Release%20(WebKit2%20Tests)',
> + 'chromium-linux': 'GTK%20Linux%2032-bit%20Release',
> + }
webkitpy already knows about these things. Please look at how the rest of the system knows about this stuff.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/rebaseliner.py:61
> + def _make_bucket_tree(self):
> + b = bucket.BucketTree()
> + b.b(None,
> + b.b('mac',
> + b.b('mac-snowleopard',
> + b.b('mac-leopard',
> + b.b('mac-tiger'))),
> + b.b('win',
> + b.b('chromium',
> + b.b('chromium-mac'),
> + b.b('chromium-win',
> + b.b('chromium-win-vista',
> + b.b('chromium-win-xp')),
> + b.b('chromium-linux',
> + b.b('chromium-gpu',
> + b.b('chromium-gpu-linux'),
> + b.b('chromium-gpu-win'),
> + b.b('chromium-gpu-mac'))))))),
> + b.b('qt',
> + b.b('qt-linux'),
> + b.b('qt-mac'),
> + b.b('qt-win')),
> + b.b('gtk'))
Is this some kind of description of the structure of the expectations fallback tree? If so, it should be in code that's shareable with the rest of webkitpy instead of siloed away here.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/rebaseliner.py:63
> + AGGREGATE_BUCKETS = ['chromium', 'chromium-win', 'win', 'mac', 'qt', 'chromium-gpu']
Why is this magic list here? We shouldn't have magic constants like this in the code.
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/result.py:35
> +class Result(object):
> + """Represents the result of a single test on a single platform"""
We already have an abstraction for Result.
> WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:29
> +from webkitpy.layout_tests.port import factory
webkitpy.tool cannot import from webkitpy.layout_tests. Code that's shared between these two packages should go in webkitpy.common.
> WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:53
> + make_option("--chromium", action="store_true", dest="use_chromium_bots", help="Fetch results from the Chromium bots."),
Boo. We should use --port to specify the port instead of making a magic special case for Chromium.
> WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:54
> + make_option("--webkit", action="store_false", dest="use_chromium_bots", help="Fetch results from the WebKit bots (default)."),
It doesn't make sense to have a --webkit command line argument for webkit-patch. Everything webkit-patch does is related to WebKit!
> WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:57
> + make_option("--list-builders", action="store_true", dest="list_builders", help="List the builders for each platform."),
> + make_option("--list-archives", action="store_true", dest="list_archives", help="Show the URLs of the test result archives."),
> + make_option("--show-archives", action="store_true", dest="show_archives", help="Show the results stored in the specified archives."),
These seem like separate commands. Why are they arguments to this command?
> WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:63
> + builder = ChromiumBuilder() if options.use_chromium_bots else WebKitBuilder()
This line of code shouldn't be here. We should get this information by instantiating a Port object. How will this line of code scale when there are N contributors to WebKit with as much infrastructure as Chromium?
> WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:73
> + exit()
Whenever you call exit, you might consider whether what you really want is a separate command. Calling exit also makes this code difficult to test.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list