Bug 57662 – new-run-webkit-tests: fix feature detection, skipped platform lists on mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: fix feature detection, skipped platform lists on mac

[Dirk Pranke]

Created attachment 87922 [details]
Patch

[Adam Barth]

Comment on attachment 87922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87922&action=review

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:134
> +    def _path_to_webcore_library(self):
> +        return self._build_path('WebCore.framework/Versions/A/WebCore')

This method does not appear to be used in this patch.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186
> +        entries = self._filesystem.glob(self._webkit_baseline_path('*'))

Doesn't this return a super huge list?  This doesn't seem like the right algorithm.  You want to enumerate the directories and then ask whether they're in the baseline search path, not enumerate all the things in those directories.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:187
> +        platform_prefix = self.name().split('-', 1)[0]

This should be a helpful function instead of munging the string here.

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 87922 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87922&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:134
> > +    def _path_to_webcore_library(self):
> > +        return self._build_path('WebCore.framework/Versions/A/WebCore')
> 
> This method does not appear to be used in this patch.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186
> > +        entries = self._filesystem.glob(self._webkit_baseline_path('*'))
> 
> Doesn't this return a super huge list?  This doesn't seem like the right algorithm.  You want to enumerate the directories and then ask whether they're in the baseline search path, not enumerate all the things in those directories.
> 

The glob() returns LayoutTests/platform/*, nor platform/*/*. It's a small list and the isdir call filters for the directories.

> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:187
> > +        platform_prefix = self.name().split('-', 1)[0]
> 
> This should be a helpful function instead of munging the string here.

Will add.

[Dirk Pranke]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 87922 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87922&action=review
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:134
> > > +    def _path_to_webcore_library(self):
> > > +        return self._build_path('WebCore.framework/Versions/A/WebCore')
> > 
> > This method does not appear to be used in this patch.
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186
> > > +        entries = self._filesystem.glob(self._webkit_baseline_path('*'))
> > 
> > Doesn't this return a super huge list?  This doesn't seem like the right algorithm.  You want to enumerate the directories and then ask whether they're in the baseline search path, not enumerate all the things in those directories.
> > 
> 
> The glob() returns LayoutTests/platform/*, nor platform/*/*. It's a small list and the isdir call filters for the directories.
> 
> > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:187
> > > +        platform_prefix = self.name().split('-', 1)[0]
> > 
> > This should be a helpful function instead of munging the string here.
> 
> Will add.

Er, actually, this was left over from an earlier, incorrect patch and isn't even needed. Deleted.

[Dirk Pranke]

Created attachment 87941 [details]
update w/ feedback from abarth

[Adam Barth]

Comment on attachment 87941 [details]
update w/ feedback from abarth

View in context: https://bugs.webkit.org/attachment.cgi?id=87941&action=review

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186
> +            basename = self._filesystem.basename(entry)

Shouldn't this be inside the if since the variable is only used inside the if?

[Adam Barth]

Comment on attachment 87941 [details]
update w/ feedback from abarth

View in context: https://bugs.webkit.org/attachment.cgi?id=87941&action=review

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:134
> +    def _path_to_webcore_library(self):
> +        return self._build_path('WebCore.framework/Versions/A/WebCore')

I would have made this change in a separate patch, but i think it's fine to add it to this patch.

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 87941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87941&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186
> > +            basename = self._filesystem.basename(entry)
> 
> Shouldn't this be inside the if since the variable is only used inside the if?

Sure, it can be. I will fix this when I land it. 

Thanks for the review.

[Dirk Pranke]

Committed r82753: <http://trac.webkit.org/changeset/82753>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82759 might have broken SnowLeopard Intel Release (WebKit2 Tests)
The following tests are not passing:
inspector/styles/styles-add-blank-property.html
plugins/windowless_plugin_paint_test.html