Bug 78127

Summary: REGRESSION (NRWT): Results for new non-text-only tests are always put in the most-specific platform directory
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, dpranke, eric, hyatt, mjs, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 88680    
Attachments:
Description Flags
Patch rniwa: review+

Adam Roben (:aroben)
Reported 2012-02-08 09:55:09 PST
To reproduce: 1. cat > LayoutTests/fast/new-test.html Hi! ^D 2. run-webkit-tests fast/new-test.html NRWT will put the new results in platform/mac-lion/fast, while ORWT will put the results in platform/mac/fast. NRWT always puts results for new non-text-only tests in the most-specific platform directory. This doesn't match ORWT, which had the following policy: > For cross-platform tests, text-only results should go in the cross-platform directory, > while render tree dumps should go in the least-specific platform directory. http://trac.webkit.org/browser/trunk/Tools/Scripts/old-run-webkit-tests?rev=105162#L1728 ORWT's policy was to assume that new tests were as cross-platform as possible (i.e., that the results would be valid on all variants of the base platform), while NRWT's policy seems to assume that new tests are as platform-specific as possible. We should be encouraging people to write cross-platform tests, so I think ORWT's policy is better.
Attachments
Patch (15.38 KB, patch)
2012-06-18 17:08 PDT, Dirk Pranke 		rniwa: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Roben (:aroben)
Comment 1 2012-02-08 09:56:28 PST
The guilty code in NRWT: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py?rev=106224#L163 port.baseline_path() ends up returning the most-specific platform directory.
Dirk Pranke
Comment 2 2012-02-08 10:05:26 PST
Hm. There seem to be plusses and minuses to ORWT's logic. I agree that tests should be as generic as possible. However, is there no way to put baselines in the most-specific directory with ORWT? If not, that would mean baseline locations would have to be managed by hand, right? The advantage to NRWT's behavior is that all platforms will put baselines in safe locations, and then a simple 'optimize' pass can eliminate duplicates. That said, I don't feel that strongly about it one way or another, and it's certainly easy to change.
Eric Seidel (no email)
Comment 3 2012-02-08 10:12:30 PST
We could include the optimize logic in NRWT. Requiring people to run webkit-patch optimize-baselines after running NRWT seems cumbersome.
Dirk Pranke
Comment 4 2012-02-08 10:27:59 PST
(In reply to comment #3) > We could include the optimize logic in NRWT. Requiring people to run webkit-patch optimize-baselines after running NRWT seems cumbersome. I'm not sure how useful that would be, since NRWT will only generate one port's results, as opposed to something like garden-o-matic or webkit-patch rebaseline, which is likely to pull all of the baselines for a test. I would be somewhat concerned that optimizing the baselines might produce some amount of thrashing if we're doing one port at a time. Maybe not, though. I do agree that running a separate optimize step is a hassle, though. Maybe we should default to least-specific, and just have a separate flag for most-specific for the rare times when you need it? (Although I'm not sure when you would know it was needed and yet not just optimize or move things by hand).
Adam Roben (:aroben)
Comment 5 2012-02-08 10:43:43 PST
(In reply to comment #4) > Maybe we should default to least-specific, and just have a separate flag for most-specific for the rare times when you need it? (Although I'm not sure when you would know it was needed and yet not just optimize or move things by hand). ORWT has such a flag: --add-platform-exceptions
Dirk Pranke
Comment 6 2012-02-08 12:04:19 PST
(In reply to comment #5) > (In reply to comment #4) > > Maybe we should default to least-specific, and just have a separate flag for most-specific for the rare times when you need it? (Although I'm not sure when you would know it was needed and yet not just optimize or move things by hand). > > ORWT has such a flag: --add-platform-exceptions Ah. Alrighty then :).
Dirk Pranke
Comment 7 2012-06-18 17:08:50 PDT
Created attachment 148204 [details] Patch
Ryosuke Niwa
Comment 8 2012-06-19 01:06:27 PDT
Comment on attachment 148204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148204&action=review > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:284 > + optparse.make_option("--add-platform-exceptions", action="store_true", default=False, I would have named this flag --use-most-specific-directory or --use-version-directory. I don't understand what "exceptions" you're referring here. > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:977 > + # Test that we update the platform expectations in the version-specific dirs Nit: Please spell out directories.
Dirk Pranke
Comment 9 2012-06-19 11:25:05 PDT
(In reply to comment #8) > (From update of attachment 148204 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148204&action=review > > > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:284 > > + optparse.make_option("--add-platform-exceptions", action="store_true", default=False, > > I would have named this flag --use-most-specific-directory or --use-version-directory. I don't understand what "exceptions" you're referring here. > Yeah, it's not a great name. I'm using it for compatibility w/ ORWT. We should probably file a separate bug to rename --reset-results, --new-baseline, and --add-platform-exceptions all at once to something saner. > > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:977 > > + # Test that we update the platform expectations in the version-specific dirs > > Nit: Please spell out directories. Will do.
Dirk Pranke
Comment 10 2012-06-19 11:27:41 PDT
Committed r120740: <http://trac.webkit.org/changeset/120740>
Note You need to log in before you can comment on or make changes to this bug.