Bug 79956 - need to clone the GPU baselines and expectations into the new "virtual" dir
Summary: need to clone the GPU baselines and expectations into the new "virtual" dir
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: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 76501
  Show dependency treegraph
 
Reported: 2012-02-29 16:00 PST by Dirk Pranke
Modified: 2012-02-29 18:52 PST (History)
8 users (show)

See Also:


Attachments
Patch (14.92 KB, patch)
2012-02-29 18:01 PST, Dirk Pranke
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-02-29 16:00:52 PST
As part of the changes for bug 76501, I am adding the ability to run "virtual test suites" in NRWT. The way this is going to work is that you can declare a directory foo to mirror a directory bar (so that we run all the tests in bar), but run the tests with an additional set of command line flags, and support a new set of directories for baselines in the fallback path.

So, to be concrete, I'm adding a new directory 'LayoutTests/platform/chromium/virtual_gpu/fast/canvas' that will point to 'fast/canvas'. Any baselines that were originally in platform/chromium-gpu-mac/fast/canvas will now need to be copied into platform/chromium-mac/platform/chromium/virtual_gpu/fast/canvas according (and same for the win and linux ports).

We will also need to list any expectations lines twice; any test in fast/canvas that is expected to fail will also need a listing in platform/chromium/virtual_gpu/fast/canvas.

This is all straightforward; once we have the new virtual dirs working stably we can then delete the chromium-gpu directories and the "GPU" expectations.
Comment 1 Stephen White 2012-02-29 16:14:40 PST
How is platform/chromium-mac/platform/chromium/virtual_gpu/fast/canvas
better than platform/chromium-gpu-mac/fast/canvas?

AFAICT, we'll have the same number of test results files, and twice the number of test_expectations lines, and longer and more confusing directory paths to find them.

I'm not convinced this is actually better.   So... convince me?  :)
Comment 2 Adam Barth 2012-02-29 16:17:36 PST
The latter doesn't require gardeners to do any special GPU-specific work and therefore easier to maintain.
Comment 3 Adam Barth 2012-02-29 16:18:01 PST
Sorry, "the former" rather.
Comment 4 Dirk Pranke 2012-02-29 16:19:18 PST
We will no longer need the 'webkit_gpu_tests' step on the bot - the tests will run as part of the main test run. This allows us to delete a fair amount of complexity in the tools for dealing with additional "ports" that aren't really ports.

I think removing the 'gpu port' as a concept will remove a fair amount of cognitive overhead that we currently inflict on the team. I think the 'virtual suite' concept is less overhead and will actually be more useful down the road for other things as well.
Comment 5 Stephen White 2012-02-29 16:39:08 PST
(In reply to comment #4)
> We will no longer need the 'webkit_gpu_tests' step on the bot - the tests will run as part of the main test run. This allows us to delete a fair amount of complexity in the tools for dealing with additional "ports" that aren't really ports.

Oh right, that's a pretty important step.

It's a shame we can't keep the GPU modifier, though.  When I tried this experimentally (by copying) for canvas/philip, it added ~65 lines to test_expectations, which I suppose is a bullet we have to bite.

Just to bikeshed a little, I'm not too fond of "virtual_gpu" as a directory name.  It sounds like the GPU is virtual.  Could it just be "gpu" instead?  Or maybe "virtual/gpu"?

And platform/chromium-mac/platform/chromium/virtual_gpu/fast/canvas is way long.  Could it be
platform/chromium-mac/gpu/fast/canvas
instead?  As long as nobody adds a new "gpu" directory off of LayoutTests, there should be no conflicts.
Comment 6 Dirk Pranke 2012-02-29 16:55:30 PST
(In reply to comment #5)
> (In reply to comment #4)
> > We will no longer need the 'webkit_gpu_tests' step on the bot - the tests will run as part of the main test run. This allows us to delete a fair amount of complexity in the tools for dealing with additional "ports" that aren't really ports.
> 
> Oh right, that's a pretty important step.
> 
> It's a shame we can't keep the GPU modifier, though.  When I tried this experimentally (by copying) for canvas/philip, it added ~65 lines to test_expectations, which I suppose is a bullet we have to bite.
> 

Yeah. As I mentioned in my note on chromium-dev@, I think we need to move to checking in failing expectations and deleting lines in the expectations file, so this hopefully goes away soon.

An alternative, though, that occurred to me a while ago would be to add in code that could inherit the expectation just like we inherit the baselines. If you wanted to have different expectations for the same test and the virtual test, you'd still need two lines (just like you need two lines for cpu/gpu today, of course).

I'm not sure if this expectations-inheriting is worth it, though, or if we should just move to checking in the expectations.

> Just to bikeshed a little, I'm not too fond of "virtual_gpu" as a directory name.  It sounds like the GPU is virtual.  Could it just be "gpu" instead?  Or maybe "virtual/gpu"?

Bikeshedding welcome here :) virtual_gpu is the best JamesR and I came up with ... we thought it was important to indicate somehow that this was a virtual directory and not a regular one. virtual/gpu would be fine, though.

> 
> And platform/chromium-mac/platform/chromium/virtual_gpu/fast/canvas is way long.  Could it be
> platform/chromium-mac/gpu/fast/canvas
> instead?  As long as nobody adds a new "gpu" directory off of LayoutTests, there should be no conflicts.

That wouldn't be consistent with the way we handle platform-specific baselines for other (non-virtual) tests, and I would prefer not to have to special case the logic.
Comment 7 Dirk Pranke 2012-02-29 17:00:02 PST
James is not wedded to virtual_gpu, and I actually like the idea of having a convention that you can find all of the virtual tests in one place, so virtual/gpu it is.
Comment 8 Dirk Pranke 2012-02-29 18:01:01 PST
Created attachment 129569 [details]
Patch
Comment 9 Dirk Pranke 2012-02-29 18:03:56 PST
Note that there are a bunch of baselines I'll be adding with this, and a line to SKIP the tests for now in test_expectations.txt.
Comment 10 Dirk Pranke 2012-02-29 18:52:00 PST
Committed r109296: <http://trac.webkit.org/changeset/109296>