Bug 78012 - run-perf-tests doesn't recognize paths that start with PerformanceTests
Summary: run-perf-tests doesn't recognize paths that start with PerformanceTests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 77037
  Show dependency treegraph
 
Reported: 2012-02-07 11:45 PST by Ryosuke Niwa
Modified: 2012-02-08 01:51 PST (History)
7 users (show)

See Also:


Attachments
Fixes the bug (4.88 KB, patch)
2012-02-07 15:18 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per Adam's comment (3.34 KB, patch)
2012-02-08 01:10 PST, Ryosuke Niwa
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-02-07 11:45:04 PST
run-perf-tests PerformanceTests/Parser runs exactly 0 tests because run-perf-tests recognizes that as PerformanceTests/PerformanceTests/Parser. It's annoying as hell.
Comment 1 Adam Barth 2012-02-07 12:35:19 PST
run-webkit-tests has some code to remove the LayoutTests from command line arguments.  Presumably you want something similar here.
Comment 2 Ryosuke Niwa 2012-02-07 13:49:12 PST
(In reply to comment #1)
> run-webkit-tests has some code to remove the LayoutTests from command line arguments.  Presumably you want something similar here.

Something like that. I just looked at its implementation in manager.py and it seems way too complicated for run-perf-test. I'm just going to use filesystem.relpath instead.
Comment 3 Ryosuke Niwa 2012-02-07 15:18:29 PST
Created attachment 125942 [details]
Fixes the bug
Comment 4 Ryosuke Niwa 2012-02-07 15:19:33 PST
I feel like we should be able to use the same approach in run-webkit-tests but there's some special treatment for / and \ there so not sure if that's possible.
Comment 5 Adam Barth 2012-02-07 16:14:59 PST
Comment on attachment 125942 [details]
Fixes the bug

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

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:111
> +            if self._host.filesystem.exists(arg):

It's strange that this checks the real file system for existence.  I would expect this to be a syntactic transformation that doesn't query the local file system.
Comment 6 Ryosuke Niwa 2012-02-07 16:28:31 PST
Comment on attachment 125942 [details]
Fixes the bug

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

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:111
>> +            if self._host.filesystem.exists(arg):
> 
> It's strange that this checks the real file system for existence.  I would expect this to be a syntactic transformation that doesn't query the local file system.

Otherwise how would I figure out whether we should use relpath or not? An alternative is to add both since find_files.find only finds existing files.
Comment 7 Adam Barth 2012-02-07 16:33:01 PST
> Otherwise how would I figure out whether we should use relpath or not? An alternative is to add both since find_files.find only finds existing files.

That could work.  You can also look whether the string PerformanceTests appears in the path.
Comment 8 Ryosuke Niwa 2012-02-07 16:35:31 PST
(In reply to comment #7)
> > Otherwise how would I figure out whether we should use relpath or not? An alternative is to add both since find_files.find only finds existing files.
> 
> That could work.  You can also look whether the string PerformanceTests appears in the path.

I want to avoid hard-coding "PerformanceTests" since I'm hoping that I can also replace the equivalent function in manager.py by this one as a followup.
Comment 9 Hajime Morrita 2012-02-07 21:47:12 PST
(In reply to comment #8)
> I want to avoid hard-coding "PerformanceTests" since I'm hoping that I can also replace the equivalent function in manager.py by this one as a followup.
You can just pass the "base directory name" as an argument to the (coming shared) function.
Avoiding file system access is good for performance reason. In this case, startup speed.

Anyway, I like this change. I'm tired to remove "PerformanceTest" part
from my completed-by-the-shell path ;-)
Comment 10 Ryosuke Niwa 2012-02-08 01:10:41 PST
Created attachment 126024 [details]
Updated per Adam's comment
Comment 11 Adam Barth 2012-02-08 01:15:56 PST
Comment on attachment 126024 [details]
Updated per Adam's comment

That certainly looks nicer.  :)
Comment 12 Ryosuke Niwa 2012-02-08 01:51:25 PST
Committed r107053: <http://trac.webkit.org/changeset/107053>