Bug 37645 - [chromium] new-run-webkit-tests should be able to use chromium DRT
Summary: [chromium] new-run-webkit-tests should be able to use chromium DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 37668
Blocks: 37793
  Show dependency treegraph
 
Reported: 2010-04-15 02:03 PDT by Tony Chang
Modified: 2010-04-19 01:23 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.77 KB, patch)
2010-04-15 02:03 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (6.75 KB, patch)
2010-04-15 23:43 PDT, Tony Chang
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-04-15 02:03:08 PDT
[chromium] new-run-webkit-tests should be able to use chromium DRT
Comment 1 Tony Chang 2010-04-15 02:03:36 PDT
Created attachment 53418 [details]
Patch
Comment 2 Tony Chang 2010-04-15 02:14:05 PDT
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.
Comment 3 Dirk Pranke 2010-04-15 10:20:05 PDT
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 @@
> +2010-04-15  Tony Chang  <tony@chromium.org>
> +
> +        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  <dpranke@chromium.org>
>  
>          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.
Comment 4 Dirk Pranke 2010-04-15 10:22:18 PDT
(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
Comment 5 Ojan Vafai 2010-04-15 10:28:15 PDT
(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.
Comment 6 Dirk Pranke 2010-04-15 10:43:17 PDT
(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.
Comment 7 Tony Chang 2010-04-15 23:43:02 PDT
Created attachment 53516 [details]
Patch
Comment 8 Adam Barth 2010-04-17 13:30:01 PDT
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.
Comment 9 Tony Chang 2010-04-18 20:27:13 PDT
Committed r57800: <http://trac.webkit.org/changeset/57800>