Bug 83944 - Extract PerfTestFactory
Summary: Extract PerfTestFactory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-13 14:07 PDT by Ryosuke Niwa
Modified: 2012-04-27 00:04 PDT (History)
6 users (show)

See Also:


Attachments
cleanup (11.06 KB, patch)
2012-04-13 14:11 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Renamed pattern_map to _pattern_map (11.06 KB, patch)
2012-04-13 15:08 PDT, Ryosuke Niwa
dpranke: 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-04-13 14:07:13 PDT
Extract PerfTestFactory
Comment 1 Ryosuke Niwa 2012-04-13 14:11:04 PDT
Created attachment 137148 [details]
cleanup
Comment 2 Eric Seidel (no email) 2012-04-13 14:19:53 PDT
Comment on attachment 137148 [details]
cleanup

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:186
> +    pattern_map = [

_pattern_map?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:192
> +    def create_perf_test(cls, test_name, path):

Do you need to assert() that the path is relative?  absolute paths would do the wrong thing here. :)

> Tools/Scripts/webkitpy/performance_tests/perftest_unittest.py:62
> +        test = PerfTest('some-test', '/path/some-dir/some-test')

Seems like a bad test case, since you don't use absolute paths, normally.

> Tools/Scripts/webkitpy/performance_tests/perftest_unittest.py:128
> +        test = PerfTestFactory.create_perf_test('inspector/some-test', '/path/inspector/some-test')

I don't see how these pass?  That doesn't match ^inspector ??

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:131
> +            tests.append(PerfTestFactory.create_perf_test(relative_path.replace('\\', '/'), path))

What's with the windows hack here??  Don't we have a nicer way to ensure unix style paths?
Comment 3 Ryosuke Niwa 2012-04-13 14:28:45 PDT
(In reply to comment #2)
> (From update of attachment 137148 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137148&action=review
> 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:186
> > +    pattern_map = [
> 
> _pattern_map?

Will do.

> > Tools/Scripts/webkitpy/performance_tests/perftest.py:192
> > +    def create_perf_test(cls, test_name, path):
> 
> Do you need to assert() that the path is relative?  absolute paths would do the wrong thing here. :)

No, they need to be absolute paths.

> > Tools/Scripts/webkitpy/performance_tests/perftest_unittest.py:128
> > +        test = PerfTestFactory.create_perf_test('inspector/some-test', '/path/inspector/some-test')
> 
> I don't see how these pass?  That doesn't match ^inspector ??

The regex matches the test name, not the path.

> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:131
> > +            tests.append(PerfTestFactory.create_perf_test(relative_path.replace('\\', '/'), path))
> 
> What's with the windows hack here??  Don't we have a nicer way to ensure unix style paths?

I'd love to know if there's one. The only reason I have that hack is so that test names match between different platforms.
Comment 4 Ryosuke Niwa 2012-04-13 15:08:28 PDT
Created attachment 137160 [details]
Renamed pattern_map to _pattern_map
Comment 5 Dirk Pranke 2012-04-26 17:07:15 PDT
Comment on attachment 137160 [details]
Renamed pattern_map to _pattern_map

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

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:131
> +            tests.append(PerfTestFactory.create_perf_test(relative_path.replace('\\', '/'), path))

I think you need to convert the \ to / before you call skips_perf_test on line 129, right?
Comment 6 Ryosuke Niwa 2012-04-27 00:02:33 PDT
Thanks for the review.

(In reply to comment #5)
> (From update of attachment 137160 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137160&action=review
> 
> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:131
> > +            tests.append(PerfTestFactory.create_perf_test(relative_path.replace('\\', '/'), path))
> 
> I think you need to convert the \ to / before you call skips_perf_test on line 129, right?

Yes.
Comment 7 Ryosuke Niwa 2012-04-27 00:04:40 PDT
Committed r115410: <http://trac.webkit.org/changeset/115410>