[Webkit-unassigned] [Bug 38693] cleanup json_results_generator dependencies so that non-layout-tests can also use it safely

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38693





--- Comment #19 from Kinuko Yasuda <kinuko at chromium.org>  2010-06-28 15:49:36 PST ---
Updated the patch with some additional refactoring changes.
(This time the patch doesn't include directory move/rename as combining update+move generally makes the patch hard to read)

(In reply to comment #16)
> (From update of attachment 59169 [details])
> 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.

Removed.

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

Refactored a bit to use a new TestResult class.

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

For now I've put an additional comment to explain why it is kept as is.
(I've once replaced this part (_get_svn_revision()) to use scm.py but it caused some run-time
errors on chromium Windows buildbot.  I've been failing to reproduce it on my env for now.)

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

Fixed.

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

Fixed.  It won't talk to the network.

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

Fixed.  It won't touch the disk either.

> WebKitTools/Scripts/webkitpy/common/test/test_results.py:30
>  +  # Test results and modifier constants.
> I don't understand what role this file places.

Removed the file as there wasn't a strong reason to expose those constants there.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.