Bug 76594 – run-perf-tests should support Skipped list

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

Unfortunately, run-perf-tests don't support all kinds of tests in PerformanceTests. In fact, only directories we can successfully run tests in are BIndings, Parser, and inspector at the moment.

We can mitigate the issue for now by adding Skipped list.

[Adam Barth]

Do we want a whitelist or a blacklist?

[Ryosuke Niwa]

(In reply to comment #1)
> Do we want a whitelist or a blacklist?

For now I use the same format as Skipped list for layout tests so blacklist.

[Ryosuke Niwa]

Created attachment 123056 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 123056 [details]
Patch

Great! Thanks for the review.

[Dirk Pranke]

Comment on attachment 123056 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/test.py:227
> +    LAYOUT_TEST_DIR = '/mock-checkout/LayoutTests'

Your fix ignores the comments on lines 220-222. I think Eric introduced the /mock-checkout directory in MockSCM at some point, and Adam (?) maybe added the path_to_module() to MockFileSystem to support something similar (although looking at the latter now, that's at least slightly broken because there's no guarantee that /mock-checkout/Tools/Scripts exists in the mock filesystem; we need to create the corresponding directories and files for consistency).

At any rate, it is (was?) a feature of NRWT that it could support LayoutTests being located outside of the webkit tree if necessary (as the chromium port once used to do), and this LAYOUT_TEST_DIR partially tested for that; it would concern me if there were tests failing because they assumed the tests lived under $webkit_root/LayoutTests, because they shouldn't (IMO) be assuming that.

It could be, of course, that there are better ways to test for the features/test cases I'm worried about. And, at the very least, if we do decide that my concerns above are more trouble than they're worth, you should update or delete the comments to comply. And I would like to document that discussion somewhere we can associate with such a CL so that if we have to do some spelunking in the future we might have a shot at finding it :).

[Ryosuke Niwa]

(In reply to comment #5)
> Your fix ignores the comments on lines 220-222. I think Eric introduced the /mock-checkout directory in MockSCM at some point, and Adam (?) maybe added the path_to_module() to MockFileSystem to support something similar (although looking at the latter now, that's at least slightly broken because there's no guarantee that /mock-checkout/Tools/Scripts exists in the mock filesystem; we need to create the corresponding directories and files for consistency).
> 
> At any rate, it is (was?) a feature of NRWT that it could support LayoutTests being located outside of the webkit tree if necessary (as the chromium port once used to do), and this LAYOUT_TEST_DIR partially tested for that; it would concern me if there were tests failing because they assumed the tests lived under $webkit_root/LayoutTests, because they shouldn't (IMO) be assuming that.
> 
> It could be, of course, that there are better ways to test for the features/test cases I'm worried about. And, at the very least, if we do decide that my concerns above are more trouble than they're worth, you should update or delete the comments to comply. And I would like to document that discussion somewhere we can associate with such a CL so that if we have to do some spelunking in the future we might have a shot at finding it :).

Okay. Thanks for the clarification. I've reverted that part of the change.

[Ryosuke Niwa]

Committed r105381: <http://trac.webkit.org/changeset/105381>

[Dirk Pranke]

Thanks! I'm definitely open to better ways to document / require / test those things. It's probably too subtle as-is.

[Ryosuke Niwa]

(In reply to comment #8)
> Thanks! I'm definitely open to better ways to document / require / test those things. It's probably too subtle as-is.

I think we need to simplify all these classes. Dependencies between host, filesystem, config, etc... are all too confusing.