Summary: | [chromium] new-run-webkit-tests should be able to use chromium DRT | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Chang <tony> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, dpranke, ojan, tkent | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | 37668 | ||||||||
Bug Blocks: | 37793 | ||||||||
Attachments: |
|
Description
Tony Chang
2010-04-15 02:03:08 PDT
Created attachment 53418 [details]
Patch
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 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. (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 (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. (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. Created attachment 53516 [details]
Patch
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.
Committed r57800: <http://trac.webkit.org/changeset/57800> |