Bug 37645 – [chromium] new-run-webkit-tests should be able to use chromium DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

[chromium] new-run-webkit-tests should be able to use chromium DRT

[Tony Chang]

Created attachment 53418 [details]
Patch

[Tony Chang]

This patch is for discussion.

Very soon now, we'll be able to build DRT on Mac.  It's missing lots of stuff, but with the attached patch, I can run layout tests (kind of).  Here are my questions:

1) What's the best way to switch to DRT?  This patch adds a command line flag, but we could also just auto-detect based on whether a chromium checkout exists.

2) I suppose we still want to build the helper binary and run them.  Do we want to use Chromium's image_diff or WebKit's ImageDiff?

3) Where should I write results to?  /tmp/layout-test-results to match other platforms?

That's enough to start.

[Dirk Pranke]

Comment on attachment 53418 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 5d80a22..21859ac 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,14 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] new-run-webkit-tests should be able to use chromium DRT
> +        https://bugs.webkit.org/show_bug.cgi?id=37645
> +
> +        * Scripts/webkitpy/layout_tests/port/chromium.py:
> +        * Scripts/webkitpy/layout_tests/port/chromium_mac.py:
> +        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
> +
>  2010-04-15  Dirk Pranke  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> index 5b90b85..4372d14 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> @@ -130,8 +130,11 @@ class ChromiumPort(base.Port):
>              'chromium', 'test_expectations.txt')
>  
>      def results_directory(self):
> -        return self.path_from_chromium_base('webkit', self._options.configuration,
> -                                            self._options.results_directory)
> +        try:
> +            return self.path_from_chromium_base('webkit',
> +                self._options.configuration, self._options.results_directory)
> +        except AssertionError:
> +            return os.path.join('/tmp', self._options.results_directory)
>

I wouldn't change this. I think the upstream folks are actually leaning to Chromium's approach, and the layout test results should stay under the build directories.

Otherwise, LGTM.

[Dirk Pranke]

(In reply to comment #2)
> 1) What's the best way to switch to DRT?  This patch adds a command line flag,
> but we could also just auto-detect based on whether a chromium checkout exists.

I think command line switches are the way to go. Whether or not I have a chromium checkout is orthogonal to whether or not I'll want to use DRT or TestShell. I could see how it might make picking a default easier, but I wouldn't bother with that.

> 2) I suppose we still want to build the helper binary and run them.  Do we want
> to use Chromium's image_diff or WebKit's ImageDiff?

We should merge the tools and only have one.

> 
> 3) Where should I write results to?  /tmp/layout-test-results to match other
> platforms?

See my comment in the review - I think this should still live in the build directory (and I think we want other platforms to change to match this, as well).

-- Dirk

[Ojan Vafai]

(In reply to comment #4)
> (In reply to comment #2)
> > 1) What's the best way to switch to DRT?  This patch adds a command line flag,
> > but we could also just auto-detect based on whether a chromium checkout exists.
> 
> I think command line switches are the way to go. Whether or not I have a
> chromium checkout is orthogonal to whether or not I'll want to use DRT or
> TestShell. I could see how it might make picking a default easier, but I
> wouldn't bother with that.

Down with command line switches! Lets at least make it default to TestShell iff a chromium checkout exists and DRT otherwise. The only reason I'm not 100% against adding the commandline option is for people who have hybrid chromium/webkit checkouts.

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > 1) What's the best way to switch to DRT?  This patch adds a command line flag,
> > > but we could also just auto-detect based on whether a chromium checkout exists.
> > 
> > I think command line switches are the way to go. Whether or not I have a
> > chromium checkout is orthogonal to whether or not I'll want to use DRT or
> > TestShell. I could see how it might make picking a default easier, but I
> > wouldn't bother with that.
> 
> Down with command line switches! Lets at least make it default to TestShell iff
> a chromium checkout exists and DRT otherwise. The only reason I'm not 100%
> against adding the commandline option is for people who have hybrid
> chromium/webkit checkouts.

Umm, yeah, I'm that person :) Given that I spend half my day flipping between webkit tests and chromium tests and am very happy that I've managed to do it all in a single checkout, please don't do things this way. Besides, the switches will only be there temporarily.

If you want to smarten-up the default detection, that's fine w/ me. I wouldn't bother, but I won't object.

[Tony Chang]

Created attachment 53516 [details]
Patch

[Adam Barth]

Comment on attachment 53516 [details]
Patch

This looks fine.

+ path_from_chromium_base

We need to remove this function call from every file that doesn't have the word "chromium" in its name.

[Tony Chang]

Committed r57800: <http://trac.webkit.org/changeset/57800>