Sketch out the win port for new-run-webkit-tests
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.).