Bug 141152 – [webkitpy] Add platform specific Skipped file mechanism for performance tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

Now there is only one skipped file for performance tests: PerformanceTests/Skipped.
But EFL needs a platform specific one to be able to skip Canvas/reuse.html - bug139812

[Csaba Osztrogonác]

Created attachment 245867 [details]
Patch

With this patch we can skip tests for platform ios,mac,win,gtk and efl. I don't want to add nested platforms as LayoutTests has.

[Csaba Osztrogonác]

Created attachment 245868 [details]
Patch

Moved _expectations_from_skipped_files function into skipped_perf_tests, because it isn't used anywhere else.

[Ryosuke Niwa]

Comment on attachment 245868 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:692
> +        skipped_file_paths = [self.perf_tests_dir(), self._filesystem.join(self.perf_tests_dir(), 'platform', self.port_name)]
>          for search_path in skipped_file_paths:

Instead of adding a seprate file, can we add a modifier like [EFL] test-name as done in test expectations?

[Csaba Osztrogonác]

(In reply to comment #3)
> Comment on attachment 245868 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245868&action=review
> 
> > Tools/Scripts/webkitpy/port/base.py:692
> > +        skipped_file_paths = [self.perf_tests_dir(), self._filesystem.join(self.perf_tests_dir(), 'platform', self.port_name)]
> >          for search_path in skipped_file_paths:
> 
> Instead of adding a seprate file, can we add a modifier like [EFL] test-name
> as done in test expectations?

Unfortunately no, because PerformanceTests/Skipped file has the simple
old style Skipped file syntax, which doesn't support any modifier.
_tests_from_skipped_file_contents omits only commented lines, other
lines are interpreted as test names:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/base.py?rev=179134#L678

I don't want to implement a sophisticated mechanism for performance tests
similar to TestExpectations. I would like to have a workaround until
bug139812 is fixed not to have to swith off the EFL performance bot.

[Ryosuke Niwa]

(In reply to comment #4)
>
> Unfortunately no, because PerformanceTests/Skipped file has the simple
> old style Skipped file syntax, which doesn't support any modifier.
> _tests_from_skipped_file_contents omits only commented lines, other
> lines are interpreted as test names:
> http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/base.
> py?rev=179134#L678

There’s no need to limit ourselves to that format.  

> I don't want to implement a sophisticated mechanism for performance tests
> similar to TestExpectations. I would like to have a workaround until
> bug139812 is fixed not to have to swith off the EFL performance bot.

I don’t like this approach because it introdues “platform” directory, which is otherwise useless.

[Ryosuke Niwa]

Created attachment 245951 [details]
Adds the support for platform modifier

[Csaba Osztrogonác]

Comment on attachment 245951 [details]
Adds the support for platform modifier

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

LGTM, r=me with some nits.

> Tools/Scripts/webkitpy/port/base.py:710
> +        line_number = 1
> +        tests_to_skip = []
> +        for line in skipped_file_contents.split('\n'):

We don't need explicit line numbering, enumerate solves this task:
for line_number, line in enumerate(skipped_file_contents.split('\n')):

> Tools/Scripts/webkitpy/port/base.py:713
> +                _log.error("Synatx error at line %d in %s: %s" % (line_number, filename, line))

typo: Synatx -> Syntax

[Csaba Osztrogonác]

(In reply to comment #7)
> We don't need explicit line numbering, enumerate solves this task:
> for line_number, line in enumerate(skipped_file_contents.split('\n')):

+ start=1 argument for enumerate

[Ryosuke Niwa]

Committed r179610: <http://trac.webkit.org/changeset/179610>