Bug 28907 – [Qt] The --strict switch of run-webkit-tests does not work

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Andras Becsi]

Created attachment 38922 [details]
Implement --without-metrics switch for the Qt port

Using run-webkit-tests --strict on the Qt port should compare the mac expected files with the actual files of the Qt port with the font size metrics removed from the files. In this way only the render tree is compared and so the obvious differences can be tracked, and the related bugs can be corrected.

The attached patch completes and corrects the implementation of --strict and renames the switch to --without-metrics.

[Andras Becsi]

Created attachment 38924 [details]
Add Changelog entry to my without-metrics patch

[Eric Seidel (no email)]

Comment on attachment 38924 [details]
Add Changelog entry to my without-metrics patch

Indentation issues in this patch.

[Andras Becsi]

(In reply to comment #2)
> (From update of attachment 38924 [details])
> Indentation issues in this patch.

Seems that there are indentation issues in the whole run-webkit-tests file.

Should there be the same policy applied to perl scripts as to the cpp files?

The check-webkit-style script only checks cpp and h files so I tried to apply the indentation of the context, which seemed to me to be the most consequent in the obviously messy script.

[Ariya Hidayat]

Comment on attachment 38924 [details]
Add Changelog entry to my without-metrics patch

As discussed over IRC, this patch should not remove --strict and it should just compare between test result and expected of the port (i.e. not mixing it with Qt vs Mac).

Also, a name like exclude-metrics or skip-metrics sounds like a better name than without-metrics.

r- until the issue is addressed.

[Andras Becsi]

Created attachment 39140 [details]
Change my patch as discussed on IRC

[Tor Arne Vestbø]

Comment on attachment 39140 [details]
Change my patch as discussed on IRC

Good stuff, but can you please split up the --strict fix and the new option into two separate patches?

A few comments:

>          print "\tThis allows the pixel tests to have consistent color values across all machines.\n";
> -        
> +
>          if (isPerianInstalled()) {

Don't add whitespace-hunks

> +    if ( !isAppleMacWebKit() && $strictTesting && !$isText ) {

No space in "if (something)"

> +        $excludeMetrics = 1;
> +        my $expectedFile = "$testDirectory/platform/mac/$expectedFileName";
> +        if ( !-s $expectedFile ) {
> +            $expectedFile = "$testDirectory/$expectedFileName";
> +        }

There can be several layers of results, see expectedDirectoryForTest() and @platformResultHierarchy

> +        $modBase = catfile( $simplifiedDir, basename($base));

Space after (

[Andras Becsi]

Created attachment 39149 [details]
Refactor --strict switch to --ignore-metrics

Refactor --strict switch to --ignore-metrics and correct the implementation to make the feature usable on all platforms. 
This is much simpler and straightforward then my previous patches and it can be easily extended to support advanced comparisons (eg. to other platform's expected files) later.

[Tor Arne Vestbø]

Comment on attachment 39149 [details]
Refactor --strict switch to --ignore-metrics

Nice. A few finishing touches:

> +my $simpleTag = "simple";

This is no longer needed.

> +    'ignore-metrics' => \$ignoreMetrics,

Should be postixed with a ! since it's a boolean flag, this will make GetOptions support --no-ignore-metrics, etc

>      'strip-editing-callbacks!' => \$stripEditingCallbacks,

Like this one

> +sub simplify($$)

Suggest renaming to stripMetrics()

> +    my $simpleExpected = $expected;
> +    my $simpleActual = $actual;

No need to assign these, you already have it in $actual and $expected.

> +    $simpleActual = $actual;

And def. no need to re-assign a second time ;)

> +    $simpleActual =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g;

Just use $actual here, it's passed by value

> +    $simpleExpected =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g;

Same, just use $expected directly

[Andras Becsi]

Created attachment 39155 [details]
Corrected copy-paste issues, and applied suggestions to previous patch

[Andras Becsi]

Created attachment 39157 [details]
Fixed my previous patch which had an error.

[Tor Arne Vestbø]

I'll land this tomorrow.

Also, I just realized the stripMetrics() function can be simplified to:

    my ($actual, $expected) = @_;

    foreach my $result ($actual, $expected) {
        $result =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g;
        ...
    }

But I'll do that change before landing.

[Andras Becsi]

Thank you very much for the extensive review, suggestions and help. 
I also noticed this possibility of simplification and I noticed a typo in the Changelog text too (succesfull -> succesful), so I created a fixed patch:

http://gist.github.com/182506

-----------
(In reply to comment #11)
> I'll land this tomorrow.
> 
> Also, I just realized the stripMetrics() function can be simplified to:
> 
>     my ($actual, $expected) = @_;
> 
>     foreach my $result ($actual, $expected) {
>         $result =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g;
>         ...
>     }
> 
> But I'll do that change before landing.

[Tor Arne Vestbø]

Landed in r48148