Bug 78127 – REGRESSION (NRWT): Results for new non-text-only tests are always put in the most-specific platform directory

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

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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]

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)]

We could include the optimize logic in NRWT.  Requiring people to run webkit-patch optimize-baselines after running NRWT seems cumbersome.

[Dirk Pranke]

(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)]

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

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

Created attachment 148204 [details]
Patch

[Ryosuke Niwa]

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]

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

Committed r120740: <http://trac.webkit.org/changeset/120740>