Summary: | Sketch out the win port for new-run-webkit-tests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dpranke, eric, ojan | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Adam Barth
2010-04-10 16:12:57 PDT
Created attachment 53056 [details]
Patch
Created attachment 53057 [details]
Patch
Comment on attachment 53057 [details]
Patch
43 if options and hasattr(options, 'chromium') and options.chromium:
should just be options.chromium, no?
What's this used for?
190191 def test_base_platform_names(self):
are you sure it shoudl return "win" as well as mac?
God I had 80c wrap:
245 return [os.path.join(self._webkit_baseline_path(self._name,
246 'Skipped'))]
What's this used for? Is this right?
84 def test_platform_name(self):
280 raise NotImplementedError('WebKitPort.test_platform_name')
285 return self._name + self.version()
Why?
48 def version(self):
49 return ''
Portname override is lame:
43 def __init__(self, port_name=None, options=None):
44 if port_name is None:
45 port_name = 'win'
63 def _tests_for_disabled_features(self):
is a copy/paste disaster. Can't that be pushed into WebKitPort?
Please respond to the comments. Otherwise this looks like an OK first start.
I accept all of your comments. I will fix them in followup patches. Well, except the 80c thing. :) Committed r57425: <http://trac.webkit.org/changeset/57425> Comment on attachment 53057 [details] Patch > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py > index f4c2433..a943b77 100644 > --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py > @@ -65,7 +65,11 @@ class WebKitPort(base.Port): > return self._webkit_baseline_path(self._name) > > def baseline_search_path(self): > - raise NotImplementedError('WebKitPort.baseline_search_path') > + return [self._webkit_baseline_path(self._name)] > + > + def path_to_test_expectations_file(self): > + return os.path.join(self._webkit_baseline_path(self._name), > + 'test_expectations.txt') Note that this'll put the test_expectations.txt file in LayoutTests/platform/{win,mac}, instead of having a common one shared by all of the WebKit variations. Are you sure you want this? If you do, you probably don't want to return multiple test_base_platform_names(). (In reply to comment #3) > (From update of attachment 53057 [details]) > 43 if options and hasattr(options, 'chromium') and > options.chromium: > > should just be options.chromium, no? No. options.chromium isn't guaranteed to be set, so you need the extra tests. > > What's this used for? > 190191 def test_base_platform_names(self): See the comments in base.py. > 84 def test_platform_name(self): > 280 raise NotImplementedError('WebKitPort.test_platform_name') > 285 return self._name + self.version() > Yes, it's as good a thing to return as any. Again, see the comments in the base class for what the function does. > Why? > 48 def version(self): > 49 return '' WebKit doesn't keep different versions of the baselines for different versions of windows, so there's no need for a version string here (just like chromium on the mac doesn't version, but webkit mac does). (In reply to comment #3) > (From update of attachment 53057 [details]) > 43 if options and hasattr(options, 'chromium') and > options.chromium: > > should just be options.chromium, no? > > What's this used for? > 190191 def test_base_platform_names(self): > > are you sure it shoudl return "win" as well as mac? > > God I had 80c wrap: > 245 return [os.path.join(self._webkit_baseline_path(self._name, > 246 'Skipped'))] > > What's this used for? Is this right? > 84 def test_platform_name(self): > 280 raise NotImplementedError('WebKitPort.test_platform_name') > 285 return self._name + self.version() > > Why? > 48 def version(self): > 49 return '' > > Portname override is lame: > 43 def __init__(self, port_name=None, options=None): > 44 if port_name is None: > 45 port_name = 'win' > > 63 def _tests_for_disabled_features(self): > is a copy/paste disaster. Can't that be pushed into WebKitPort? > > Please respond to the comments. Otherwise this looks like an OK first start. > Note that this'll put the test_expectations.txt file in
> LayoutTests/platform/{win,mac}, instead
> of having a common one shared by all of the WebKit variations. Are you sure you
> want this? If you
> do, you probably don't want to return multiple test_base_platform_names().
I'm not sure what we want to do here. Currently the test_expectation.txt file is in LayoutTests/platform/mac, so I went with that.
(In reply to comment #9) > > Note that this'll put the test_expectations.txt file in > > LayoutTests/platform/{win,mac}, instead > > of having a common one shared by all of the WebKit variations. Are you sure you > > want this? If you > > do, you probably don't want to return multiple test_base_platform_names(). > > I'm not sure what we want to do here. Currently the test_expectation.txt file > is in LayoutTests/platform/mac, so I went with that. Maybe I'm not following you, but the code now will return platform/mac/test_expectations.txt when running on a mac, and platform/win/test_expectations.txt when running on windows. I.e., you'll have two files to update, which defeats the point a bit. Perhaps we should put the "base" version in LayoutTests/test_expectations.txt and the various ports can decide if they want to pile into the base version or use their own like Chromium does. Alternatively, we could have a "platform/webkit" directory for this file and any baselines that produce webkit-specific output (e.g., for text errors specific to JSC, etc.). |