[webkit-reviews] review granted: [Bug 34511] refactor run-chromium-webkit-tests, make work for webkit mac platform : [Attachment 48273] patch 4 of 4 - add WebKit Mac implementation, test implementation, and a simple test driver

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 34511: refactor run-chromium-webkit-tests, make work for webkit mac
platform
https://bugs.webkit.org/show_bug.cgi?id=34511

Attachment 48273: patch 4 of 4 - add WebKit Mac implementation, test
implementation, and a simple test driver
https://bugs.webkit.org/attachment.cgi?id=48273&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
It's slightly confusing to me what driver_test.py is for.  You explained in
person that it's a testing tool used to short-cut all the 5000 lines of python
you normally would go through to call into the basic driver code.

Seems it's only useful as an intermediate step towards unit tests.  We might
want to add a FIXME to the file to suggest that it shoudl be replaced by real
unit tests some day.

I think that we probably would have called "TestPort" "MockPort" (not to be
confused with MachPort! :p)  But is also OK as is and can be re-worked more
later.


Why "is None" here?
 50	    if port_name is None:
is that better than just "not port_name"?
I'm not sure what "port.version()" is actually going to use for "port" or how
it would compile?
 51		port_name = 'mac' + port.version()

Seems we'll want different ports for the different versions eventually?
4     def baseline_search_path(self):
 55	    dirs = []
 56	    if self._name == 'mac-tiger':
 57		dirs.append(self._webkit_baseline_path(self._name))
 58	    if self._name in ('mac-tiger', 'mac-leopard'):
 59		dirs.append(self._webkit_baseline_path('mac-leopard'))
 60	    if self._name in ('mac-tiger', 'mac-leopard', 'mac-snowleopard'):
 61		dirs.append(self._webkit_baseline_path('mac-snowleopard'))
 62	    dirs.append(self._webkit_baseline_path('mac'))
 63	    return dirs

Not sure.

I think we should consider making this an optional override method (i.e. don't
bother to raise NotImplemented()):
76     def setup_test_run(self):
 77	    # This port doesn't require any specific configuration.
 78	    pass
 
We have user.open_webpage for this:
 82	    webbrowser.open(uri, new=1)

Again, we should probably make these non-required in the base class:
88     def start_helper(self):
 89	    # This port doesn't use a helper process.
 90	    pass
 91 
 92	def stop_helper(self):
 93	    # This port doesn't use a helper process.
 94	    pass

I think the skipped-list translator shoudl be a separate class.  But that also
could be done in a later change:
 101	 def test_expectations(self):

Again, the default port shoudl just pass here:
     def _path_to_helper(self):
 210	     return None

There is a special class for parsing editor variables, which probably would
work right for this:
63	       # This split() isn't really what we want -- it incorrectly will
 264		 # split quoted strings within the wrapper argument -- but in
 265		 # practice it shouldn't come up and the --help output warns
 266		 # about it anyway.
 267		 cmd += self._options.wrapper.split()

Seems we'll want to share more code with other ports over time.  SEems to be
some copy/paste here.