Bug 37393 – Sketch out the win port for new-run-webkit-tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Sketch out the win port for new-run-webkit-tests

[Adam Barth]

Created attachment 53056 [details]
Patch

[Adam Barth]

Created attachment 53057 [details]
Patch

[Eric Seidel (no email)]

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.

[Adam Barth]

I accept all of your comments.  I will fix them in followup patches.

[Adam Barth]

Well, except the 80c thing.  :)

[Adam Barth]

Committed r57425: <http://trac.webkit.org/changeset/57425>

[Dirk Pranke]

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().

[Dirk Pranke]

(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.

[Adam Barth]

> 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.

[Dirk Pranke]

(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.).