Bug 34511 – refactor run-chromium-webkit-tests, make work for webkit mac platform

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

This is a tracking bug to follow the work being done to genericize run-chromium-webkit-tests and make it work with the base Mac platform.

[Dirk Pranke]

Created attachment 47989 [details]
Patch

[Dirk Pranke]

Hmm. Okay, 290KB is a pretty big patch. I'll see if I can split this up into more reasonable chunks tomorrow.

Note that this does work as-is, and we get a 3.5x speedup over run-webkit-tests on my Mac Pro; the tests complete in just over 2 minutes.

[Tony Chang]

Comment on attachment 47989 [details]
Patch

I think changing the name from platform to port is confusing.  I think of Chromium as a single WebKit port, but you're talking about the difference between Chromium platforms.

Also, it's impossible to tell from this diff what is a copy from an existing file and what is a new implementation.  I skipped a lot of stuff because it kind of felt like a copy.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/driver_test.py b/WebKitTools/Scripts/webkitpy/layout_tests/driver_test.py
> +import os
> +import port
> +import optparse

Nit: alphabetize imports

> +def run_tests(port, options, tests):
> +    image_path = 'foo.png'

image_path appears unused.

> +    driver = port.start_driver('foo.png', None)

What is foo.png?

> +    for t in tests:
> +        uri = port.filename_to_uri(os.path.join(port.layout_tests_dir(), t))
> +        print "uri: " + uri
> +        (crash, timeout, checksum, output, err) = \

Nit: no () on the left side of the equals.

> +if __name__ == '__main__':
> +    optparser = optparse.OptionParser()
> +    optparser.add_option('-p', '--port', action='store', default='mac')

Please add help text for all the flags.  Also, can you use type=choice to limit the possible values for port and target?

> +    optparser.add_option('-t', '--target', action='store', default='Release')
> +    optparser.add_option('', '--timeout', action='store', default='2000')

type=int?  Help sound mention that timeout is in ms.




> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py
> +        if hasattr(self._port, 'path_from_base'):

Why would _port not have this attribute?  Can we force a common interface for each Port?  E.g., a method that returns the directories needed here.


> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py
> +    def __init__(self, port, tests, expectations, platform, is_debug_mode,
> +                  is_lint_mode, tests_are_present=True):

Nit: Seems we lost the docstring.  Would be nice to document the args.

> @@ -320,27 +309,13 @@ class TestExpectationsFile:
>          """Returns an object that can be iterated over. Allows for not caring
>          about whether we're iterating over a file or a new-line separated
>          string."""

Nit: This should probably be a generator function.

> +        # Strip final entry if it's empty to avoid added in an extra
> +        # newline.
> +        if iterable[len(iterable) - 1] == "\n":
> +            return iterable[:len(iterable) - 1]

Nit: 
if iterable[-1] == "\n"
    return iterable[:-1]

> @@ -398,6 +373,9 @@ class TestExpectationsFile:
> +        # TODO(dpranke) - update this

Nit: WebKit seems to use FIXME instead of TODO.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py
> +        (crash, timeout, actual_checksum, output, error) =  \

Nit: No ()s.

> @@ -454,32 +403,23 @@ class TestShellThread(threading.Thread):
> +        (crash, timeout, actual_checksum, output, error) = \

Nit: No ()s

> +        end = time.time()
> +
> +        stats = process_output(self._port, test_info, self._test_types,
> +                               self._test_args, self._options.target,
> +                               self._options.results_directory, crash,
> +                               timeout, end - start, actual_checksum,
> +                               output, error)

Nit: Having so many args seems like a recipe for hard to maintain code.  Maybe we can refactor into some objects in a separate change.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py
> +class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
> +
> +    def __init__(self, port_obj, output_dir):

Everywhere else you call this "port" rather than "port_obj".  Would be nice to be consistent.

Would also be nice if this file had a way to start/stop the server from the command line like the other http server implementations.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> +class Driver:

Nit: Should the file docstring make mention of the Driver class?

> +    def __init__(self, png_path, options, port_object):

Nit: Would be nice to be consistent between port_object and port.  Also, all the other classes seem to take port as the first arg and this class takes it as the last arg.

> +    def run_test(uri, timeout, checksum):

Nit: Should the first arg be self?  Probably doesn't matter since it's overridden by implementations.

> +        Returns a tuple of the following:
> +            crash - a boolean indicating whether the driver crashed on the test
> +            timeout - a boolean indicating whehter the test timed out
> +            checksum - a string containing the checksum of the image, if
> +                present
> +            output - any text output
> +            error - any unexpected or additional (or error) text output

This is the type of thing that would be nice to make a full fledged class out of rather than returning a 5-tuple.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> +    def setup_test_run(self):
> +        # Delete the disk cache if any to ensure a clean test run.

Is this necessary these days?  I thought we used the in memory cache (otherwise running multiple test shells would corrupt the cache).

> +    def start_helper(self):
> +        self._helper = None
> +        return
> +        helper_path = self._path_to_helper()

Can we delete this unreached code?

> +    def stop_helper(self):
> +        pass
> +        if self._helper:

This code too.

> +        return self._path_from_base('webkit', 'data', 'layout_tests',
> +                                   'platform', platform, 'LayoutTests')

Nit: indention seems off by one.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/http_server.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/http_server.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/http_server.py

Should this file be named to reflect that it's just for lighttpd?  Alternately, can we do without until we bring up Chromium Win?

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py
> +def remove_log_files(folder, starts_with):

I think I saw this method in a different file.  Would be nice to share.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 47989 [details])
> I think changing the name from platform to port is confusing.  I think of
> Chromium as a single WebKit port, but you're talking about the difference
> between Chromium platforms.

I agree that it is confusing. Ideally I would have used just 'platform', but that's a builtin module in python. I felt that 'platform_utils' was also not a great name and I thought that port was better.

Also the name is used to refer to truly different ports, like 'chromium' vs 'mac' vs 'gtk'.

> 
> Also, it's impossible to tell from this diff what is a copy from an existing
> file and what is a new implementation.  I skipped a lot of stuff because it
> kind of felt like a copy.
> 

Yeah, a lot of it is a copy, and there doesn't seem to be a good way to indicate that using git; plus even where it was a copy a lot of it got re-arranged.

> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/driver_test.py b/WebKitTools/Scripts/webkitpy/layout_tests/driver_test.py
> > +import os
> > +import port
> > +import optparse
> 
> Nit: alphabetize imports
> 

Done.

> > +def run_tests(port, options, tests):
> > +    image_path = 'foo.png'
> 
> image_path appears unused.
> 
> > +    driver = port.start_driver('foo.png', None)
> 
> What is foo.png?
>

Clarified the two. It was supposed to be start_driver(image_path). Added a comment to indicate that it's the path to write the image capture result to.
 
> > +    for t in tests:
> > +        uri = port.filename_to_uri(os.path.join(port.layout_tests_dir(), t))
> > +        print "uri: " + uri
> > +        (crash, timeout, checksum, output, err) = \
> 
> Nit: no () on the left side of the equals.
> 

Done.

> > +if __name__ == '__main__':
> > +    optparser = optparse.OptionParser()
> > +    optparser.add_option('-p', '--port', action='store', default='mac')
> 
> Please add help text for all the flags.  Also, can you use type=choice to limit
> the possible values for port and target?
> 
> > +    optparser.add_option('-t', '--target', action='store', default='Release')
> > +    optparser.add_option('', '--timeout', action='store', default='2000')
> 
> type=int?  Help sound mention that timeout is in ms.
> 

Done

> 
> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py
> > +        if hasattr(self._port, 'path_from_base'):
> 
> Why would _port not have this attribute?  Can we force a common interface for
> each Port?  E.g., a method that returns the directories needed here.
>

_port_from_base() is actually only defined in the Chromium implementations and points to the top of the Chromium source tree; it isn't meaningful in the WebKit mac port (or other ports).

I will add more descriptive hooks to the Port interface for determining rev info.
 
> 
> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py
> > +    def __init__(self, port, tests, expectations, platform, is_debug_mode,
> > +                  is_lint_mode, tests_are_present=True):
> 
> Nit: Seems we lost the docstring.  Would be nice to document the args.
> 

Done.

> > @@ -320,27 +309,13 @@ class TestExpectationsFile:
> >          """Returns an object that can be iterated over. Allows for not caring
> >          about whether we're iterating over a file or a new-line separated
> >          string."""
> 
> Nit: This should probably be a generator function.
> 

You're probably right, but I'll leave it for now.

> > +        # Strip final entry if it's empty to avoid added in an extra
> > +        # newline.
> > +        if iterable[len(iterable) - 1] == "\n":
> > +            return iterable[:len(iterable) - 1]
> 
> Nit: 
> if iterable[-1] == "\n"
>     return iterable[:-1]
> 

Done

> > @@ -398,6 +373,9 @@ class TestExpectationsFile:
> > +        # TODO(dpranke) - update this
> 
> Nit: WebKit seems to use FIXME instead of TODO.
> 

Done

> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py
> > +        (crash, timeout, actual_checksum, output, error) =  \
> 
> Nit: No ()s.
> 
> > @@ -454,32 +403,23 @@ class TestShellThread(threading.Thread):
> > +        (crash, timeout, actual_checksum, output, error) = \
> 
> Nit: No ()s
> 
> > +        end = time.time()
> > +
> > +        stats = process_output(self._port, test_info, self._test_types,
> > +                               self._test_args, self._options.target,
> > +                               self._options.results_directory, crash,
> > +                               timeout, end - start, actual_checksum,
> > +                               output, error)
> 
> Nit: Having so many args seems like a recipe for hard to maintain code.  Maybe
> we can refactor into some objects in a separate change.
> 

Agree completely.

> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py
> > +class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
> > +
> > +    def __init__(self, port_obj, output_dir):
> 
> Everywhere else you call this "port" rather than "port_obj".  Would be nice to
> be consistent.
>

Unfortunately, the server files also use "port" to indicate the port to run the server on (e.g., 8000). I didn't want to use "port_obj" everywhere, but changing
the socket port here was awkward.
 
> Would also be nice if this file had a way to start/stop the server from the
> command line like the other http server implementations.
>

Yeah. I'll add that later.
 
> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> > +class Driver:
> 
> Nit: Should the file docstring make mention of the Driver class?
> 

done

> > +    def __init__(self, png_path, options, port_object):
> 
> Nit: Would be nice to be consistent between port_object and port.  Also, all
> the other classes seem to take port as the first arg and this class takes it as
> the last arg.
>

done. Not sure why I used port_object here, or flipped the order.
 
> > +    def run_test(uri, timeout, checksum):
> 
> Nit: Should the first arg be self?  Probably doesn't matter since it's
> overridden by implementations.
>

Yes. Good catch.
 
> > +        Returns a tuple of the following:
> > +            crash - a boolean indicating whether the driver crashed on the test
> > +            timeout - a boolean indicating whehter the test timed out
> > +            checksum - a string containing the checksum of the image, if
> > +                present
> > +            output - any text output
> > +            error - any unexpected or additional (or error) text output
> 
> This is the type of thing that would be nice to make a full fledged class out
> of rather than returning a 5-tuple.
> 

Yeah. See above in test_shell_thread.

> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> > +    def setup_test_run(self):
> > +        # Delete the disk cache if any to ensure a clean test run.
> 
> Is this necessary these days?  I thought we used the in memory cache (otherwise
> running multiple test shells would corrupt the cache).

I actually don't know; this was copied over from the existing code.

> 
> > +    def start_helper(self):
> > +        self._helper = None
> > +        return
> > +        helper_path = self._path_to_helper()
> 
> Can we delete this unreached code?
> 
> > +    def stop_helper(self):
> > +        pass
> > +        if self._helper:
> 
> This code too.
> 

Fixed. This was broken.

> > +        return self._path_from_base('webkit', 'data', 'layout_tests',
> > +                                   'platform', platform, 'LayoutTests')
> 
> Nit: indention seems off by one.
> 

Fixed.

> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/http_server.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/http_server.py
> > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/http_server.py
> 
> Should this file be named to reflect that it's just for lighttpd?  Alternately,
> can we do without until we bring up Chromium Win?
>

Probably, but I"ll leave it for this CL. Chromium Win is already working.
 
> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py
> > +def remove_log_files(folder, starts_with):
> 
> I think I saw this method in a different file.  Would be nice to share.

Done - made a common method in http_server_base.

[Dirk Pranke]

Created attachment 48098 [details]
full patch revised w/ feedback from reviewers

attaching revised full patch.

[Eric Seidel (no email)]

The renames make it hard to see what's going on here.  rs=me on making any renames first w/o changing functionality and then we could review a patch which actually changes/adds functionality.

rs=me on doing the Chromium -> Google Inc changes in the licenses as well.

[Dirk Pranke]

Created attachment 48106 [details]
patch 1 of 4 - rename files to the new dir structure

This was checked in as r54327 ("rubber-stamped" per eric seidel's instructions)

[Dirk Pranke]

Created attachment 48107 [details]
uploading patch 2 of 4 - this just fix a string in the copyright text for the files.

this was committed as r54328 ("rubber-stamped" per instructions from eric seidel)

[Dirk Pranke]

Created attachment 48109 [details]
patch 3 of 4 - refactor the port package into an object-oriented style

[Dirk Pranke]

Okay, this latest patch shows the substantive changes involved in the refactoring. Please review.

(There will be one more patch that adds in the WebKit mac port plus some test code).

[Evan Martin]

I glanced at the patch and I still see copyright tweaks -- did you forget to merge in your other committed changes before making the diff?

[Eric Seidel (no email)]

3 of 4 seems to include copyright text changes too.  Did you upload the right patch?  The bots also don't think that 3 of 4 applies to trunk...

[Dirk Pranke]

(In reply to comment #12)
> 3 of 4 seems to include copyright text changes too.  Did you upload the right
> patch?  The bots also don't think that 3 of 4 applies to trunk...

The copyright diffs to the first two files were files that were missed in patch 2. There was one more copyright diff in chromium_win.py; I'm not sure what happened there.

Apart from that, it was the right patch, but I created it using svn diff and posting it by hand. I'll re-upload it using webkit-patch and hopefully that'll be happier.

[Dirk Pranke]

Created attachment 48161 [details]
resubmitted patch 3 of 4; not sure why the bots didn't like the last one

[Tony Chang]

Comment on attachment 48161 [details]
resubmitted patch 3 of 4; not sure why the bots didn't like the last one

This diff is much easier to read.  Thanks.

LGTM.

> Index: WebKitTools/Scripts/webkitpy/layout_tests/run_chromium_webkit_tests.py
> -    def __init__(self, options, meter):
> +    def __init__(self, options, meter, port):

Nit: Can we pass in port as the first arg here?

> -        return (test_args, shell_args)
> +        return (test_args, png_path, shell_args)

Nit: You don't need parens around this.

> Index: WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py
> +        (crash, timeout, actual_checksum, output, error) =  \
> +            driver.run_test(test_info.uri.strip(), test_info.timeout,
> +                            test_info.image_hash)

Nit: You don't need the parens on the left side of this assignment.

[Eric Seidel (no email)]

Comment on attachment 48161 [details]
resubmitted patch 3 of 4; not sure why the bots didn't like the last one

Seems a bit strange that the port has a path_from_chromium_base method.

Maybe instead we should use a "add_repository_paths" method.  or maybe a "repostiory_paths" method which returns a list that we loop over.

 57             port: reference to platform-specific interface
Seems like you mean something like "a webkit port, e.g. Chromium Mac, or Apple Windows"

This will probably become clear to me later:
 60             platform: test platform name to use for parsing expectations
but I thought that "port" encompassed platform?

I'm not clear what this means:
 391         # FIXME(dpranke) - update this

Generally # FIXME in webkit don't have a speicifc person assigned, they're supposed to be detailed enough to be generally actionable.

I'm not sure this is accurate:
 58       port: port-specific hooks

This is starting to get to the point where we might wnat to return a real class object. :)
152         (crash, timeout, actual_checksum, output, error) =  \
 153             driver.run_test(test_info.uri.strip(), test_info.timeout,
 154                             test_info.image_hash)

I have to catch a shuttle, but I'll read more on the shuttle and we can talk about this in person this afternoon.

Looks good so far.

[Dirk Pranke]

(In reply to comment #15)
> (From update of attachment 48161 [details])
> This diff is much easier to read.  Thanks.
> 
> LGTM.
> 
> > Index: WebKitTools/Scripts/webkitpy/layout_tests/run_chromium_webkit_tests.py
> > -    def __init__(self, options, meter):
> > +    def __init__(self, options, meter, port):
> 
> Nit: Can we pass in port as the first arg here?
> 

Done.

> > -        return (test_args, shell_args)
> > +        return (test_args, png_path, shell_args)
> 
> Nit: You don't need parens around this.
>

Done. I actually find it a bit easier to follow when we return multiple arguments to wrap them in parens, but maybe that's just me.
 
> > Index: WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py
> > +        (crash, timeout, actual_checksum, output, error) =  \
> > +            driver.run_test(test_info.uri.strip(), test_info.timeout,
> > +                            test_info.image_hash)
> 
> Nit: You don't need the parens on the left side of this assignment.

Done. ibid.

[Eric Seidel (no email)]

Comment on attachment 48161 [details]
resubmitted patch 3 of 4; not sure why the bots didn't like the last one

expected_baselines could have just always returned a list, and the callers that only care about the first, could have only looked at the first?  But perhaps all_baselines is a performance optimization.  Either way, not something we need to change before landing this patch. :)

We should probably hold the ports as contants somewhere, but again, doesn't matter for this patch:
 210         if relative_path.startswith(LAYOUTTEST_HTTP_DIR):
 211             # http/tests/ run off port 8000 and ssl/ off 8443
 212             relative_path = relative_path[len(LAYOUTTEST_HTTP_DIR):]
 213             port = 8000
 214         elif relative_path.startswith(LAYOUTTEST_WEBSOCKET_DIR):
 215             # websocket/tests/ run off port 8880 and 9323
 216             # Note: the root is /, not websocket/tests/
 217             port = 8880

Interesting.  Will this re-raise e?:
 251         except OSError, e:
 252             if e.errno != errno.EEXIST:
 253                 raise

Looks functional, and better than what we have.

It's hard to give an in-depth review of a change this large.  But I'm willing to rubber-stamp this for sure!

I looked at every line.  I'm just not sure that my little brain could actually hold it all in at once. :)

Thanks for the patch!

[Dirk Pranke]

(In reply to comment #16)
> (From update of attachment 48161 [details])
> Seems a bit strange that the port has a path_from_chromium_base method.
> 
> Maybe instead we should use a "add_repository_paths" method.  or maybe a
> "repostiory_paths" method which returns a list that we loop over.
>

Yeah, it's a bit unclear what the best way to handle this is. I'll talk to Ojan about what he'd like to do here.
 
>  57             port: reference to platform-specific interface
> Seems like you mean something like "a webkit port, e.g. Chromium Mac, or Apple
> Windows"

Clarified a bit in the comments. 

> 
> This will probably become clear to me later:
>  60             platform: test platform name to use for parsing expectations
> but I thought that "port" encompassed platform?
> 
> I'm not clear what this means:
>  391         # FIXME(dpranke) - update this
> 
> Generally # FIXME in webkit don't have a speicifc person assigned, they're
> supposed to be detailed enough to be generally actionable.
> 
> I'm not sure this is accurate:
>  58       port: port-specific hooks
> 
> This is starting to get to the point where we might wnat to return a real class
> object. :)
> 152         (crash, timeout, actual_checksum, output, error) =  \
>  153             driver.run_test(test_info.uri.strip(), test_info.timeout,
>  154                             test_info.image_hash)
> 
> I have to catch a shuttle, but I'll read more on the shuttle and we can talk
> about this in person this afternoon.
> 
> Looks good so far.

[Dirk Pranke]

Created attachment 48271 [details]
patch 3 of 4 - revised w/ feedback from tony and eric

[Dirk Pranke]

uploaded patch w/ mods per tony and eric's feedback.

[Eric Seidel (no email)]

Comment on attachment 48271 [details]
patch 3 of 4 - revised w/ feedback from tony and eric

LGTM.

[Dirk Pranke]

Created attachment 48273 [details]
patch 4 of 4 - add WebKit Mac implementation, test implementation, and a simple test driver

[Eric Seidel (no email)]

Comment on attachment 48273 [details]
patch 4 of 4 - add WebKit Mac implementation, test implementation, and a simple test driver

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.

[Dirk Pranke]

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

Agreed.

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

Perhaps. They're not technically Mock units and so I didn't want to confusing things overly.

> 
> Why "is None" here?
>  50         if port_name is None:
> is that better than just "not port_name"?

I think it's more idiomatic, but I could be wrong.

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

That's a bug. Fixed.
 
> 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.
>


There's not enough variance to justify multiple files or classes for different versions (IMO).

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

I kind of like it this way because it explicitly makes the implementor look at something and indicate what the right thing to do is. But, I could be convinced otherwise.
 
> We have user.open_webpage for this:
>  82         webbrowser.open(uri, new=1)
> 

Okay.

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

Perhaps a better option would be to allow port-specific command line arguments, so that we don't need to try and cram them into a single --wrapper argument.
 
> Seems we'll want to share more code with other ports over time.  SEems to be
> some copy/paste here.

Yeah, especially in the interrupt handling. I think we probably want to rework that code completely since the way the driver handles ctrl-C is clunky at best (there's a bug filed somewhere about this).

[Dirk Pranke]

Committed r54452: <http://trac.webkit.org/changeset/54452>

[Dirk Pranke]

(In reply to comment #18)
> (From update of attachment 48161 [details])
> expected_baselines could have just always returned a list, and the callers that
> only care about the first, could have only looked at the first?  But perhaps
> all_baselines is a performance optimization.  Either way, not something we need
> to change before landing this patch. :)
> 

all_baselines is used by the scripts that crawl the baseline dirs looking for duplicate expectations. There is no indication that one baseline dir is more important than the others. I suppose we could restructure it as the most important one being first. It might actually be better to split this into two different files.

> We should probably hold the ports as contants somewhere, but again, doesn't
> matter for this patch:
>  210         if relative_path.startswith(LAYOUTTEST_HTTP_DIR):
>  211             # http/tests/ run off port 8000 and ssl/ off 8443
>  212             relative_path = relative_path[len(LAYOUTTEST_HTTP_DIR):]
>  213             port = 8000
>  214         elif relative_path.startswith(LAYOUTTEST_WEBSOCKET_DIR):
>  215             # websocket/tests/ run off port 8880 and 9323
>  216             # Note: the root is /, not websocket/tests/
>  217             port = 8880
> 

Agreed. There's a lot of overlap between the three server implementations that can probably be cleaned up.

> Interesting.  Will this re-raise e?:
>  251         except OSError, e:
>  252             if e.errno != errno.EEXIST:
>  253                 raise
> 

Yes. raise by itself will re-raise the exception (if it exists; if not, you get a different exception being thrown).

> Looks functional, and better than what we have.
> 
> It's hard to give an in-depth review of a change this large.  But I'm willing
> to rubber-stamp this for sure!
> 
> I looked at every line.  I'm just not sure that my little brain could actually
> hold it all in at once. :)
> 
> Thanks for the patch!