Bug 93904 - old-run-webkit-tests should skip reference tests with all file extensions, not just .html.
Summary: old-run-webkit-tests should skip reference tests with all file extensions, no...
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: Roger Fong
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-13 14:56 PDT by Roger Fong
Modified: 2012-08-14 14:16 PDT (History)
3 users (show)

See Also:


Attachments
patch (1.71 KB, patch)
2012-08-13 15:05 PDT, Roger Fong
no flags Details | Formatted Diff | Diff
patch (1.66 KB, patch)
2012-08-14 11:28 PDT, Roger Fong
dpranke: review+
dpranke: commit-queue+
Details | Formatted Diff | Diff
perl fix (2.32 KB, patch)
2012-08-14 11:58 PDT, Roger Fong
thorton: review-
thorton: commit-queue-
Details | Formatted Diff | Diff
doh (1.74 KB, patch)
2012-08-14 12:50 PDT, Roger Fong
no flags Details | Formatted Diff | Diff
Rei (1.81 KB, patch)
2012-08-14 13:05 PDT, Roger Fong
no flags Details | Formatted Diff | Diff
Mee (1.80 KB, patch)
2012-08-14 13:12 PDT, Roger Fong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2012-08-13 14:56:02 PDT
old-run-webkit-tests ref test only skipped .html test.
They need to skip .html, .shtml, .xml, .xhtml, .pl, .htm, .php, .svg, .mht.
Comment 1 Tim Horton 2012-08-13 14:56:28 PDT
(In reply to comment #0)
> old-run-webkit-tests ref test only skipped .html test.
> They need to skip .html, .shtml, .xml, .xhtml, .pl, .htm, .php, .svg, .mht.

You should mention in the bug where you got this list from, probably.
Comment 2 Roger Fong 2012-08-13 15:05:11 PDT
Created attachment 158116 [details]
patch
Comment 3 Roger Fong 2012-08-13 15:05:29 PDT
Hey Dirk,
Could I get you to review this? 
Thanks,
Roger
Comment 4 Tim Horton 2012-08-13 15:06:42 PDT
Comment on attachment 158116 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158116&action=review

> Tools/Scripts/old-run-webkit-tests:2574
> +    foreach (@extensions) {
> +        if (-f "$base-$expectedTag.$_" || -f "$base-$expectedTag-$mismatchTag.$_") {
> +            return 1;
> +        }
> +    }

I wonder if you should match only the extension that was used in $filename.
Comment 5 Dirk Pranke 2012-08-14 10:58:06 PDT
Comment on attachment 158116 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158116&action=review

change basically looks fine.

> Tools/Scripts/old-run-webkit-tests:2563
> +    if ($filename =~ /-$expectedTag(-$mismatchTag)?\.(html|shtml|xml|xhtml|htm|php|svg|mht)$/) {

Are all of these actually used in reftests? That seems unlikely.

> Tools/Scripts/old-run-webkit-tests:2568
> +    my @extensions = ('html','shtml','xml','xhtml','htm','php','svg','mht');

it would be good if we could share the same list here and above

>> Tools/Scripts/old-run-webkit-tests:2574
>> +    }
> 
> I wonder if you should match only the extension that was used in $filename.

I'm not actually sure that matching extensions is (or should be) a requirement.
Comment 6 Tim Horton 2012-08-14 11:00:48 PDT
(In reply to comment #5)
> (From update of attachment 158116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158116&action=review
> 
> change basically looks fine.
> 
> > Tools/Scripts/old-run-webkit-tests:2563
> > +    if ($filename =~ /-$expectedTag(-$mismatchTag)?\.(html|shtml|xml|xhtml|htm|php|svg|mht)$/) {
> 
> Are all of these actually used in reftests? That seems unlikely.

Probably not, but this is the list that's supported by NRWT, so it'll prevent surprises.

> > Tools/Scripts/old-run-webkit-tests:2568
> > +    my @extensions = ('html','shtml','xml','xhtml','htm','php','svg','mht');
> 
> it would be good if we could share the same list here and above

Agreed.

> >> Tools/Scripts/old-run-webkit-tests:2574
> >> +    }
> > 
> > I wonder if you should match only the extension that was used in $filename.
> 
> I'm not actually sure that matching extensions is (or should be) a requirement.

Sure, I suppose that doesn't make any sense. Ignore me :)
Comment 7 Dirk Pranke 2012-08-14 11:03:33 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 158116 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158116&action=review
> > 
> > change basically looks fine.
> > 
> > > Tools/Scripts/old-run-webkit-tests:2563
> > > +    if ($filename =~ /-$expectedTag(-$mismatchTag)?\.(html|shtml|xml|xhtml|htm|php|svg|mht)$/) {
> > 
> > Are all of these actually used in reftests? That seems unlikely.
> 
> Probably not, but this is the list that's supported by NRWT, so it'll prevent surprises.
> 

Sure. NRWT also has '.pl' :) - webkitpy.layout_tests.port.base.Port._supported_file_extensions ...
Comment 8 Roger Fong 2012-08-14 11:28:18 PDT
Created attachment 158381 [details]
patch

Made that array fix,
Added .pl
Comment 9 Roger Fong 2012-08-14 11:30:54 PDT
I'm also not a perl expert...as in I've never used it...so one sec on that patch...
Comment 10 Roger Fong 2012-08-14 11:58:58 PDT
Created attachment 158388 [details]
perl fix

That should do it.
Ran it on the whole svg animations test folder (lots of ref tests there) and none of them ran.
I'll get Tim to OK the perl but I'm pretty sure it's right now.
Comment 11 Tim Horton 2012-08-14 12:46:02 PDT
Comment on attachment 158388 [details]
perl fix

View in context: https://bugs.webkit.org/attachment.cgi?id=158388&action=review

> Source/WebCore/platform/graphics/win/FontCGWin.cpp:309
> +        shouldUseFontSmoothing = true;

Hmm?

> Tools/Scripts/old-run-webkit-tests:2563
> +    my @extensions = ('html','shtml','xml','xhtml','htm','php','svg','mht','.pl');

No period.
Comment 12 Roger Fong 2012-08-14 12:50:41 PDT
Created attachment 158395 [details]
doh
Comment 13 Tim Horton 2012-08-14 12:55:25 PDT
Comment on attachment 158395 [details]
doh

View in context: https://bugs.webkit.org/attachment.cgi?id=158395&action=review

> Tools/Scripts/old-run-webkit-tests:2565
> +    my $extensionStr = join("|", @extensions);
> +    my $regexStr = "-$expectedTag(-$mismatchTag)?\\.(".$extensionStr.")\$";

This looks suspiciously like hungarian notation, which we don't do :)

> Tools/Scripts/old-run-webkit-tests:2572
> +    foreach (@extensions) {

you can do foreach my $extension (@extensions) and then use the nice, named $extension instead of the meaningless $_ below.
Comment 14 Dirk Pranke 2012-08-14 13:01:46 PDT
all fine comments; my r+ was way too sloppy :(.
Comment 15 Roger Fong 2012-08-14 13:05:50 PDT
Created attachment 158399 [details]
Rei
Comment 16 Tim Horton 2012-08-14 13:06:40 PDT
Comment on attachment 158399 [details]
Rei

View in context: https://bugs.webkit.org/attachment.cgi?id=158399&action=review

> Tools/Scripts/old-run-webkit-tests:2565
> +    my $extensions_joined = join("|", @extensions);
> +    my $extension_expression = "-$expectedTag(-$mismatchTag)?\\.(".$extensions_joined.")\$";

These should be camel case, not underscored :D
Comment 17 Roger Fong 2012-08-14 13:12:40 PDT
Created attachment 158400 [details]
Mee
Comment 18 WebKit Review Bot 2012-08-14 14:16:23 PDT
Comment on attachment 158400 [details]
Mee

Clearing flags on attachment: 158400

Committed r125604: <http://trac.webkit.org/changeset/125604>
Comment 19 WebKit Review Bot 2012-08-14 14:16:27 PDT
All reviewed patches have been landed.  Closing bug.