[webkit-reviews] review denied: [Bug 38693] cleanup json_results_generator dependencies so that non-layout-tests can also use it safely : [Attachment 59169] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jun 19 17:36:33 PDT 2010
Adam Barth <abarth at webkit.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 38693: cleanup json_results_generator dependencies so that non-layout-tests
can also use it safely
https://bugs.webkit.org/show_bug.cgi?id=38693
Attachment 59169: Patch
https://bugs.webkit.org/attachment.cgi?id=59169&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
Mostly an r- because it looks like your unit test is talking to the network,
which is bad news bears.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:1
+ #!/usr/bin/env python
We don't need this line. This file isn't runnable on its own.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:83
+ def __init__(self, builder_name, build_name, build_number,
Wow, that's a lot of arguments. Maybe we'd be better off with some sort of
object?
WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:141
+ def _get_svn_revision(self, in_directory):
This function shouldn't exist.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:147
+ if os.path.exists(os.path.join(in_directory, '.svn')):
scm.py does this work.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:149
+ output = subprocess.Popen(["svn", "info", "--xml"],
Please don't use subprocess directly. Use executive.py instead.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:151
+ shell=(sys.platform == 'win32'),
I'm not sure we want to use the shell here.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:45
+ class TestJSONGeneration(unittest.TestCase):
We usually use Test as a suffix instead of a prefix.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:42
+ BUILDER_BASE_URL = "http://build.chromium.org/buildbot/gtest-results/"
Is this really going to talk to the network? We should use a Mock network
instead so the unit tests are self-contained.
WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:47
+ self.results_directory = tempfile.mkdtemp()
We should also abstract away access to the file system so we don't need to
touch the disk to test this code (which isn't really about manipulating the
disk).
WebKitTools/Scripts/webkitpy/common/test/test_results.py:30
+ # Test results and modifier constants.
I don't understand what role this file places.
More information about the webkit-reviews
mailing list