Bug 76594 - run-perf-tests should support Skipped list
Summary: run-perf-tests should support Skipped list
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: 10266
  Show dependency treegraph
 
Reported: 2012-01-18 17:53 PST by Ryosuke Niwa
Modified: 2012-01-19 00:27 PST (History)
7 users (show)

See Also:


Attachments
Patch (14.09 KB, patch)
2012-01-18 19:48 PST, Ryosuke Niwa
no flags 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-01-18 17:53:07 PST
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.
Comment 1 Adam Barth 2012-01-18 17:59:10 PST
Do we want a whitelist or a blacklist?
Comment 2 Ryosuke Niwa 2012-01-18 19:26:09 PST
(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.
Comment 3 Ryosuke Niwa 2012-01-18 19:48:08 PST
Created attachment 123056 [details]
Patch
Comment 4 Ryosuke Niwa 2012-01-18 20:38:57 PST
Comment on attachment 123056 [details]
Patch

Great! Thanks for the review.
Comment 5 Dirk Pranke 2012-01-18 21:08:10 PST
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 :).
Comment 6 Ryosuke Niwa 2012-01-18 21:59:20 PST
(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.
Comment 7 Ryosuke Niwa 2012-01-18 22:00:44 PST
Committed r105381: <http://trac.webkit.org/changeset/105381>
Comment 8 Dirk Pranke 2012-01-18 22:19:00 PST
Thanks! I'm definitely open to better ways to document / require / test those things. It's probably too subtle as-is.
Comment 9 Ryosuke Niwa 2012-01-18 22:29:06 PST
(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.