Bug 14740 - Hierarchical layout tests and platform organization
Summary: Hierarchical layout tests and platform organization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-23 19:26 PDT by Matt Lilek
Modified: 2007-08-11 13:48 PDT (History)
4 users (show)

See Also:


Attachments
wip (5.35 KB, text/plain)
2007-07-28 22:44 PDT, Matt Lilek
no flags Details
wip v2 (6.15 KB, text/plain)
2007-07-30 19:41 PDT, Matt Lilek
no flags Details
wip v3 (10.55 KB, text/plain)
2007-08-09 19:21 PDT, Matt Lilek
no flags Details
reviewable v1 (11.79 KB, patch)
2007-08-10 18:41 PDT, Matt Lilek
no flags Details | Formatted Diff | Diff
reviewable v2 (12.00 KB, patch)
2007-08-10 23:12 PDT, Matt Lilek
aroben: review+
Details | Formatted Diff | Diff
reviewable v3 (12.27 KB, patch)
2007-08-11 10:40 PDT, Matt Lilek
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lilek 2007-07-23 19:26:58 PDT
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.
Comment 1 Matt Lilek 2007-07-28 22:44:01 PDT
Created attachment 15723 [details]
wip

Work in progress that does platform-specific results and such but not platform-specific tests
Comment 2 Adam Roben (:aroben) 2007-07-28 22:55:12 PDT
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`
Comment 3 Adam Roben (:aroben) 2007-07-28 23:02:06 PDT
(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.
Comment 4 Matt Lilek 2007-07-30 19:41:42 PDT
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?
Comment 5 Adam Roben (:aroben) 2007-07-30 19:56:51 PDT
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?
Comment 6 Adam Roben (:aroben) 2007-07-30 20:43:58 PDT
(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.

> 

Comment 7 Matt Lilek 2007-08-09 19:21:43 PDT
Created attachment 15892 [details]
wip v3

*almost* ready for full review - want to test it a bit more
Comment 8 Adam Roben (:aroben) 2007-08-09 19:48:39 PDT
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).
Comment 9 Adam Roben (:aroben) 2007-08-09 19:53:15 PDT
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.
Comment 10 Matt Lilek 2007-08-10 18:41:29 PDT
Created attachment 15915 [details]
reviewable v1
Comment 11 Adam Roben (:aroben) 2007-08-10 19:18:41 PDT
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!
Comment 12 Matt Lilek 2007-08-10 19:22:21 PDT
Comment on attachment 15915 [details]
reviewable v1

clearing review flag so I can add a skipped file override
Comment 13 Matt Lilek 2007-08-10 23:12:08 PDT
Created attachment 15925 [details]
reviewable v2
Comment 14 Adam Roben (:aroben) 2007-08-10 23:33:13 PDT
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
Comment 15 Adam Roben (:aroben) 2007-08-10 23:35:54 PDT
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...
Comment 16 Matt Lilek 2007-08-11 10:40:16 PDT
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.
Comment 17 Adam Roben (:aroben) 2007-08-11 12:37:37 PDT
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 :-)
Comment 18 Matt Lilek 2007-08-11 13:48:19 PDT
(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 :)