[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