Bug 41776 – Need to have a way to specify different results for Windows XP and 7

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

[Brian Weinstein]

There are some international text tests that are failing on Windows 7, while passing on Windows XP due to differences in international fonts.

We need some way to have different results for Window XP and Windows 7, like we have for Tiger, Leopard, and Snow Leopard.

[Brian Weinstein]

Created attachment 60750 [details]
[PATCH] Fix

[Adam Roben (:aroben)]

Comment on attachment 60750 [details]
[PATCH] Fix

It looks like you've made it so XP will look in win-xp and win, and Vista will look in win-vista and win, and 7 will look in win.

I think it would be better and less confusing to match Mac by having it be:

XP: win-xp, win-vista, win-7, win
Vista: win-vista, win-7, win
7: win-7, win

You could basically adapt the code that uses @macPlatforms in old-run-webkit-tests.

I think "isWindowsXP" and "isWindowsVista" and "isWindows7" are better than using the "Win" nickname in the function names.

> +sub determineWinVersion()
> +{
> +    return if $winVersion;
> +
> +    if (!isCygwin()) {
> +        $winVersion = -1;
> +        return;
> +    }
> +
> +    my $versionString = `uname -s`;
> +    $versionString =~ /(\d.\d)/;

This will allow any character between the two digits. You should escape the . with a backslash.

> +
> +    my @splitVersion = split(/\./, $1);
> +    @splitVersion >= 2 or die "Invalid version $versionString";
> +    $winVersion = {
> +            "major" => $splitVersion[0],
> +            "minor" => $splitVersion[1]
> +    };
> +}

I don't think there's any reason to split the major and minor version. Just keep it as a string and do things like (winVersion() eq "6.1") in the is* functions.

[Brian Weinstein]

(In reply to comment #2)
> (From update of attachment 60750 [details])
> It looks like you've made it so XP will look in win-xp and win, and Vista will look in win-vista and win, and 7 will look in win.
> 
> I think it would be better and less confusing to match Mac by having it be:
> 
> XP: win-xp, win-vista, win-7, win
> Vista: win-vista, win-7, win
> 7: win-7, win

Fixed, and updated the ChangeLog to describe this.

> 
> You could basically adapt the code that uses @macPlatforms in old-run-webkit-tests.

Done, refactored that code a little bit.

> 
> I think "isWindowsXP" and "isWindowsVista" and "isWindows7" are better than using the "Win" nickname in the function names.

Fixed.

> 
> > +sub determineWinVersion()
> > +{
> > +    return if $winVersion;
> > +
> > +    if (!isCygwin()) {
> > +        $winVersion = -1;
> > +        return;
> > +    }
> > +
> > +    my $versionString = `uname -s`;
> > +    $versionString =~ /(\d.\d)/;
> 
> This will allow any character between the two digits. You should escape the . with a backslash.

Done.

> 
> > +
> > +    my @splitVersion = split(/\./, $1);
> > +    @splitVersion >= 2 or die "Invalid version $versionString";
> > +    $winVersion = {
> > +            "major" => $splitVersion[0],
> > +            "minor" => $splitVersion[1]
> > +    };
> > +}
> 
> I don't think there's any reason to split the major and minor version. Just keep it as a string and do things like (winVersion() eq "6.1") in the is* functions.

Done. Thanks!

[Brian Weinstein]

Created attachment 60753 [details]
[PATCH] Fix - Take 2

[Adam Roben (:aroben)]

Comment on attachment 60753 [details]
[PATCH] Fix - Take 2

> +my @winPlatforms = ("win-xp", "win-vista", "win-7", "win");

If you add "mac-snowleopard" and "mac" as the last two items in this list, you can get rid of the code that appends them to the list in expectedDirectoryForTest.

> @@ -1852,13 +1861,14 @@ sub buildPlatformResultHierarchy()
>      mkpath($platformTestDirectory) if ($platform eq "undefined" && !-d "$platformTestDirectory");
>  
>      my @platforms;
> -    if ($platform =~ /^mac-/) {
> +    if ($platform =~ /^mac-/ || $platform =~ /^win-/) {
> +        my @platformList = $platform =~  /^mac-/ ? @macPlatforms : @winPlatforms;

You have an extra space after =~ here.

I think it would be better to do something like this:

my $isMac = $platform =~ /^mac-/;
my $isWin = $platform =~ /^win-/;

if ($isMac || $isWin) {
    my @platformList = $isMac ? @macPlatforms : @winPlatforms;

> +sub isWindows7()
> +{
> +    return isCygwin() && winVersion() eq "6.1";
> +}
> +
> +sub isWindowsVista()
> +{
> +    return isCygwin() && winVersion() eq "6.0";
> +}
> +
> +sub isWindowsXP()
> +{
> +    return isCygwin() && winVersion() eq "5.1";
> +}

No need for the isCygwin() checks here, given the way that winVersion() works.

[Brian Weinstein]

(In reply to comment #5)
> (From update of attachment 60753 [details])
> > +my @winPlatforms = ("win-xp", "win-vista", "win-7", "win");
> 
> If you add "mac-snowleopard" and "mac" as the last two items in this list, you can get rid of the code that appends them to the list in expectedDirectoryForTest.
> 

Fixed.

> > @@ -1852,13 +1861,14 @@ sub buildPlatformResultHierarchy()
> >      mkpath($platformTestDirectory) if ($platform eq "undefined" && !-d "$platformTestDirectory");
> >  
> >      my @platforms;
> > -    if ($platform =~ /^mac-/) {
> > +    if ($platform =~ /^mac-/ || $platform =~ /^win-/) {
> > +        my @platformList = $platform =~  /^mac-/ ? @macPlatforms : @winPlatforms;
> 
> You have an extra space after =~ here.

Refactored, so not an issue anymore.

> 
> I think it would be better to do something like this:
> 
> my $isMac = $platform =~ /^mac-/;
> my $isWin = $platform =~ /^win-/;
> 
> if ($isMac || $isWin) {
>     my @platformList = $isMac ? @macPlatforms : @winPlatforms;
> 

Fixed.

> > +sub isWindows7()
> > +{
> > +    return isCygwin() && winVersion() eq "6.1";
> > +}
> > +
> > +sub isWindowsVista()
> > +{
> > +    return isCygwin() && winVersion() eq "6.0";
> > +}
> > +
> > +sub isWindowsXP()
> > +{
> > +    return isCygwin() && winVersion() eq "5.1";
> > +}
> 
> No need for the isCygwin() checks here, given the way that winVersion() works.

Fixed. Thanks!

[Brian Weinstein]

Created attachment 60780 [details]
[PATCH] Fix (one last pass)

[Adam Roben (:aroben)]

Comment on attachment 60780 [details]
[PATCH] Fix (one last pass)

> -    if ($platform =~ /^mac-/) {
> +    
> +    my $isSpecificMac = $platform =~ /^mac-/;
> +    my $isWin = $platform =~ /^win/;

Doing:

my $isMac = $platform =~ /^mac/;

would have the same behavior, I believe (only "mac" will get added to @platforms). It might be slightly easier to understand, too, since it would make Mac and Win the same.

r=me

[Brian Weinstein]

Created attachment 60783 [details]
[PATCH] Small nits picked, so I can set cq+

[Adam Roben (:aroben)]

Comment on attachment 60783 [details]
[PATCH] Small nits picked, so I can set cq+

r=me

[Brian Weinstein]

Landed in r62742.