Bug 28907 - [Qt] The --strict switch of run-webkit-tests does not work
Summary: [Qt] The --strict switch of run-webkit-tests does not work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-02 04:36 PDT by Andras Becsi
Modified: 2009-09-08 05:36 PDT (History)
1 user (show)

See Also:


Attachments
Implement --without-metrics switch for the Qt port (7.96 KB, patch)
2009-09-02 04:36 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
Add Changelog entry to my without-metrics patch (9.15 KB, patch)
2009-09-02 04:54 PDT, Andras Becsi
ariya.hidayat: review-
Details | Formatted Diff | Diff
Change my patch as discussed on IRC (13.37 KB, patch)
2009-09-07 04:23 PDT, Andras Becsi
vestbo: review-
vestbo: commit-queue-
Details | Formatted Diff | Diff
Refactor --strict switch to --ignore-metrics (9.50 KB, patch)
2009-09-07 08:33 PDT, Andras Becsi
vestbo: review-
Details | Formatted Diff | Diff
Corrected copy-paste issues, and applied suggestions to previous patch (9.03 KB, patch)
2009-09-07 10:34 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
Fixed my previous patch which had an error. (9.03 KB, patch)
2009-09-07 10:42 PDT, Andras Becsi
vestbo: review+
vestbo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2009-09-02 04:36:34 PDT
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.
Comment 1 Andras Becsi 2009-09-02 04:54:43 PDT
Created attachment 38924 [details]
Add Changelog entry to my without-metrics patch
Comment 2 Eric Seidel (no email) 2009-09-02 13:58:27 PDT
Comment on attachment 38924 [details]
Add Changelog entry to my without-metrics patch

Indentation issues in this patch.
Comment 3 Andras Becsi 2009-09-03 01:51:15 PDT
(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.
Comment 4 Ariya Hidayat 2009-09-04 06:27:30 PDT
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.
Comment 5 Andras Becsi 2009-09-07 04:23:49 PDT
Created attachment 39140 [details]
Change my patch as discussed on IRC
Comment 6 Tor Arne Vestbø 2009-09-07 04:39:06 PDT
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 (
Comment 7 Andras Becsi 2009-09-07 08:33:23 PDT
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.
Comment 8 Tor Arne Vestbø 2009-09-07 08:42:22 PDT
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
Comment 9 Andras Becsi 2009-09-07 10:34:40 PDT
Created attachment 39155 [details]
Corrected copy-paste issues, and applied suggestions to previous patch
Comment 10 Andras Becsi 2009-09-07 10:42:47 PDT
Created attachment 39157 [details]
Fixed my previous patch which had an error.
Comment 11 Tor Arne Vestbø 2009-09-07 11:10:50 PDT
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.
Comment 12 Andras Becsi 2009-09-07 11:38:50 PDT
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.
Comment 13 Tor Arne Vestbø 2009-09-08 05:36:48 PDT
Landed in r48148