Bug 14740 – Hierarchical layout tests and platform organization

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Matt Lilek]

Bugzilla for the current plan for better platform-specific layout test support.
Corresponding wiki page: <http://trac.webkit.org/projects/webkit/wiki/RunWebKitTestsDesign>

---

Each platform configuration should define one variable, $platformTestDirectory, that specifies the most specific directory under LayoutTests/platform/ that applies to that platform. For instance, on Mac OS X Leopard, mac-leopard should be specified.

This name is broken into all possible hyphenated prefixes. For example, if a platform specifies a-b-c as its $platformTestDirectory, the directories in its platform hierarchy become (in order of decreasing specificity): a-b-c -> a-b -> a

run-webkit-tests should search recursively through all the subdirectories of LayoutTests/ to find test files, just as it does currently. However, it should not search beneath platform/. It should then search through each of the directories in the platform hierarchy to find additional platform-specific tests to run.

[Matt Lilek]

Created attachment 15723 [details]
wip

Work in progress that does platform-specific results and such but not platform-specific tests

[Adam Roben (:aroben)]

Comment on attachment 15723 [details]
wip

This looks like a great start! A few comments:

+my $platform;

I think it would be good to define $platform to its default setting before parsing the options. Then you can show the default in the usage output.

+my $platformTestDirectory = $testDirectory . "/platform/";

It would be nice to use File::Spec->catdir() instead of building up these paths manually.

+} elsif (isTiger()) {
+    $platformTestDirectory .= "mac-leopard";
+} elsif (isLeopard()) {
+    $platformTestDirectory .= "mac";

I think these are reversed.

+    foreach my $level (@platformHierarchy) {
+        return $level if (-f "$level/$base-$expectedTag.txt");
+    }

I believe this will traverse the platform hierarchy from least- to most-specific, but we want it to go the other way, so that results in, for example, `mac-leopard` will override results in `mac`

[Adam Roben (:aroben)]

(In reply to comment #2)
> (From update of attachment 15723 [details] [edit])
> +    foreach my $level (@platformHierarchy) {
> +        return $level if (-f "$level/$base-$expectedTag.txt");
> +    }
> 
> I believe this will traverse the platform hierarchy from least- to
> most-specific, but we want it to go the other way, so that results in, for
> example, `mac-leopard` will override results in `mac`

   Ah, I guess I was wrong about this one.

[Matt Lilek]

Created attachment 15753 [details]
wip v2

> +my $platform;
>
> I think it would be good to define $platform to its default setting before
> parsing the options. Then you can show the default in the usage output.

Done - though I feel dirty the way I did this (but maybe that's just me).

> +my $platformTestDirectory = $testDirectory . "/platform/";
>
> It would be nice to use File::Spec->catdir() instead of building up these paths
> manually.
> 
> +} elsif (isTiger()) {
> +    $platformTestDirectory .= "mac-leopard";
> +} elsif (isLeopard()) {
> +    $platformTestDirectory .= "mac";
> 
> I think these are reversed.

Done and done.

I'm sure there are nitpicks left in here which is why I'm not putting it up for review.  Another reason is that platform specific tests do not use the resources of lower levels if applicable. So say you had a platform/mac-leopard/css1/basic test that relied on one of the resources in the css1/resources folder - that platform-specific test would require it's own copy of those resources in platform/mac-leopard/css1/resources - is that an issue and if so, how big?

[Adam Roben (:aroben)]

Comment on attachment 15753 [details]
wip v2

+sub buildTestHierarchy();

I think buildTestHierarchy is a misleading name, because this isn't building up an hierarchy of tests. Maybe buildPlatformHierarchy would be better?

+} elsif (isTiger()) {
+    $platform = "mac";

We should probably just say isOSX() here, since we want OS X v. 10.6 to match this, once it exists.

+if ($platformOverride) {
+    $platform = $platformOverride;

There's no need for a separate $platformOverride variable. The --platform option can just set $platform directly.

This is looking really close! I think all we have left is generating new results, right?

[Adam Roben (:aroben)]

(In reply to comment #4)
> Created an attachment (id=15753) [edit]
> wip v2
> 
> > +my $platform;
> >
> > I think it would be good to define $platform to its default setting before
> > parsing the options. Then you can show the default in the usage output.
> 
> Done - though I feel dirty the way I did this (but maybe that's just me).
> 
> > +my $platformTestDirectory = $testDirectory . "/platform/";
> >
> > It would be nice to use File::Spec->catdir() instead of building up these paths
> > manually.
> > 
> > +} elsif (isTiger()) {
> > +    $platformTestDirectory .= "mac-leopard";
> > +} elsif (isLeopard()) {
> > +    $platformTestDirectory .= "mac";
> > 
> > I think these are reversed.
> 
> Done and done.
> 
> I'm sure there are nitpicks left in here which is why I'm not putting it up for
> review.  Another reason is that platform specific tests do not use the
> resources of lower levels if applicable. So say you had a
> platform/mac-leopard/css1/basic test that relied on one of the resources in the
> css1/resources folder - that platform-specific test would require it's own copy
> of those resources in platform/mac-leopard/css1/resources - is that an issue
> and if so, how big?

   Well, the test could hard-code the path to the shared resource (e.g., "../../../../css1/basic/resources/whatever.jpg"). We could eventually decide to move all resources to some shared location, though I imagine there will be some platform-specific resources at some point.

> 

[Matt Lilek]

Created attachment 15892 [details]
wip v3

*almost* ready for full review - want to test it a bit more

[Adam Roben (:aroben)]

Comment on attachment 15892 [details]
wip v3

+if (isTiger()) {
+    $platform = "mac-leopard";
+} elsif (isLeopard()) {
+    $platform = "mac";

I think it should be:

if (isLeopard()) {
   $platform = "mac-leopard";
} elsif (isOSX()) {
   $platform = "mac";
}...

+my $platformTestDirectory = catdir($testDirectory, 'platform');
+$platformTestDirectory = catdir($platformTestDirectory, $platform);

You can combine these lines into one:

my $platformTestDirectory = catdir($textDirectory, 'platform', $platform);

+sub platformDirForTest($)

I think this could use a better name -- perhaps "directoryForNewResults" or something like that. Then we might want to rename "expectedDirectoryForText" to "directoryForExistingResults".

+    my @paths = split('/', $test);

You should use File::Spec->splitpath here:

my (undef, @paths, undef) = File::Spec->splitpath($test);

(I'm not sure if the leading '/' is still needed in this case).

[Adam Roben (:aroben)]

Comment on attachment 15892 [details]
wip v3

I just realized that platformDirForTest shouldn't be needed at all. It can all be encapsulated into expectedDirectoryForTest, like so:

 sub expectedDirectoryForTest($)
 {
     my ($base) = @_;

    foreach my $level (@platformHierarchy) {
        return $level if (-f "$level/$base-$expectedTag.txt");
    }

    return $expectedDirectory if -f "$expectedDirectory/$base-$expectedTag.txt";

    return $platformHierarchy[$#platformHierarchy];
 }

This should work correctly for both new and old tests, and for both generating and comparing results.

[Matt Lilek]

Created attachment 15915 [details]
reviewable v1

[Adam Roben (:aroben)]

Comment on attachment 15915 [details]
reviewable v1

+my $platformDefault = $platform ? $platform : "none";

I wonder if we should die if $platform is undefined at this point. Seems like bad things might happen down the road if we keep going.

+        my $resultDir = expectedDirectoryForTest($test);

I think you can just use $expectedDir instead (this happens in 2 places).

r=me!

[Matt Lilek]

Comment on attachment 15915 [details]
reviewable v1

clearing review flag so I can add a skipped file override

[Matt Lilek]

Created attachment 15925 [details]
reviewable v2

[Adam Roben (:aroben)]

Comment on attachment 15925 [details]
reviewable v2

+    # FIXME: Get rid of the $useWinSkipped hack once Windows doesn't need a Skipped file anymore
+    # or stops using the mac result directory or we come up with something better.

I think this should say something clearer like:

# FIXME: The two lines below are a hack to represent that on Windows we
# currently want to
# 1) use the Mac expected results
# 2) use the Windows Skipped file
# Once Windows has its own results and/or we come up with a better way of
# sharing results with Mac, we should set $platform to "win" and get rid of
# $useWinSkipped entirely.

+            print "new (result generated in $expectedDir)\n";

I think it should be "results" with an "s".

+    if (!defined($platform) || $platform eq "none") {
+        return $testDirectory;
+    }

I think you'd better return ($testDirectory) so that the caller still gets a list.

r=me

[Adam Roben (:aroben)]

Comment on attachment 15925 [details]
reviewable v2

+    if (!defined($platform) || $platform eq "none") {
+        return $testDirectory;
+    }

Hm, won't this mean that we'll run all the tests twice if we hit this code? We first add all the tests from $testDirectory, then all the tests in @platformHierarchy, but in this case they're the same...

[Matt Lilek]

Created attachment 15930 [details]
reviewable v3

The only real change is if there's no platform specified, don't get confused and run the tests twice, and create a "undefined" directory in platform/ and stick the results in there as well.

[Adam Roben (:aroben)]

Comment on attachment 15930 [details]
reviewable v3

+    mkpath($platformTestDirectory) if ($platform eq "undefined" && !-d "$platformTestDirectory");

This behavior seems fine, though it will be a little less confusing for people if you print out a message along the lines of "Your platform is not recognized. Any new results will be generated in platform/undefined." Or you could just abort the script with a call to die.

Either way, r=me (again :-)

[Matt Lilek]

(In reply to comment #17)
> (From update of attachment 15930 [details] [edit])
> +    mkpath($platformTestDirectory) if ($platform eq "undefined" && !-d
> "$platformTestDirectory");
> 
> This behavior seems fine, though it will be a little less confusing for people
> if you print out a message along the lines of "Your platform is not recognized.
> Any new results will be generated in platform/undefined." Or you could just
> abort the script with a call to die.
> 
> Either way, r=me (again :-)
> 

I had that in the last patch, but I tweaked the wording before I landed it in r25015 :)