old-run-webkit-tests ref test only skipped .html test. They need to skip .html, .shtml, .xml, .xhtml, .pl, .htm, .php, .svg, .mht.
(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.
Created attachment 158116 [details] patch
Hey Dirk, Could I get you to review this? Thanks, Roger
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 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.
(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 :)
(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 ...
Created attachment 158381 [details] patch Made that array fix, Added .pl
I'm also not a perl expert...as in I've never used it...so one sec on that patch...
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 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.
Created attachment 158395 [details] doh
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.
all fine comments; my r+ was way too sloppy :(.
Created attachment 158399 [details] Rei
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
Created attachment 158400 [details] Mee
Comment on attachment 158400 [details] Mee Clearing flags on attachment: 158400 Committed r125604: <http://trac.webkit.org/changeset/125604>
All reviewed patches have been landed. Closing bug.