Bug 83944 – Extract PerfTestFactory

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Extract PerfTestFactory

[Ryosuke Niwa]

Created attachment 137148 [details]
cleanup

[Eric Seidel (no email)]

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?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 137160 [details]
Renamed pattern_map to _pattern_map

[Dirk Pranke]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r115410: <http://trac.webkit.org/changeset/115410>