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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jun 19 17:36:34 PDT 2010


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
  Attachment #59169|review?                     |review-
               Flag|                            |

--- Comment #16 from Adam Barth <abarth at webkit.org>  2010-06-19 17:36:33 PST ---
(From update of attachment 59169)
Mostly an r- because it looks like your unit test is talking to the network, which is bad news bears.

 +  #!/usr/bin/env python
We don't need this line.  This file isn't runnable on its own.

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

 +      def _get_svn_revision(self, in_directory):
This function shouldn't exist.

 +          if os.path.exists(os.path.join(in_directory, '.svn')):
scm.py does this work.

 +              output = subprocess.Popen(["svn", "info", "--xml"],
Please don't use subprocess directly.  Use executive.py instead.

 +                                        shell=(sys.platform == 'win32'),
I'm not sure we want to use the shell here.

 +  class TestJSONGeneration(unittest.TestCase):
We usually use Test as a suffix instead of a prefix.

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

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

 +  # Test results and modifier constants.
I don't understand what role this file places.

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