Bug 93904 – old-run-webkit-tests should skip reference tests with all file extensions, not just .html.

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

[Roger Fong]

old-run-webkit-tests ref test only skipped .html test.
They need to skip .html, .shtml, .xml, .xhtml, .pl, .htm, .php, .svg, .mht.

[Tim Horton]

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

[Roger Fong]

Created attachment 158116 [details]
patch

[Roger Fong]

Hey Dirk,
Could I get you to review this? 
Thanks,
Roger

[Tim Horton]

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.

[Dirk Pranke]

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.

[Tim Horton]

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

[Dirk Pranke]

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

[Roger Fong]

Created attachment 158381 [details]
patch

Made that array fix,
Added .pl

[Roger Fong]

I'm also not a perl expert...as in I've never used it...so one sec on that patch...

[Roger Fong]

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.

[Tim Horton]

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.

[Roger Fong]

Created attachment 158395 [details]
doh

[Tim Horton]

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.

[Dirk Pranke]

all fine comments; my r+ was way too sloppy :(.

[Roger Fong]

Created attachment 158399 [details]
Rei

[Tim Horton]

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

[Roger Fong]

Created attachment 158400 [details]
Mee

[WebKit Review Bot]

Comment on attachment 158400 [details]
Mee

Clearing flags on attachment: 158400

Committed r125604: <http://trac.webkit.org/changeset/125604>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.