Bug 37393 - Sketch out the win port for new-run-webkit-tests
Summary: Sketch out the win port for new-run-webkit-tests
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:
Blocks:
 
Reported: 2010-04-10 16:12 PDT by Adam Barth
Modified: 2010-04-10 17:25 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.84 KB, patch)
2010-04-10 16:14 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (8.85 KB, patch)
2010-04-10 16:15 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-04-10 16:12:57 PDT
Sketch out the win port for new-run-webkit-tests
Comment 1 Adam Barth 2010-04-10 16:14:08 PDT
Created attachment 53056 [details]
Patch
Comment 2 Adam Barth 2010-04-10 16:15:32 PDT
Created attachment 53057 [details]
Patch
Comment 3 Eric Seidel (no email) 2010-04-10 16:31:21 PDT
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.
Comment 4 Adam Barth 2010-04-10 16:38:19 PDT
I accept all of your comments.  I will fix them in followup patches.
Comment 5 Adam Barth 2010-04-10 16:38:36 PDT
Well, except the 80c thing.  :)
Comment 6 Adam Barth 2010-04-10 16:39:51 PDT
Committed r57425: <http://trac.webkit.org/changeset/57425>
Comment 7 Dirk Pranke 2010-04-10 17:04:12 PDT
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().
Comment 8 Dirk Pranke 2010-04-10 17:13:03 PDT
(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.
Comment 9 Adam Barth 2010-04-10 17:15:31 PDT
> 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.
Comment 10 Dirk Pranke 2010-04-10 17:25:29 PDT
(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.).